Date: Tue, 13 May 2014 15:32:31 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andrey Chernov <ache@freebsd.org> Cc: src-committers@freebsd.org, svn-src-stable-10@freebsd.org, Jilles Tjoelker <jilles@stack.nl>, svn-src-stable@freebsd.org, Don Lewis <truckman@freebsd.org>, svn-src-all@freebsd.org Subject: Re: svn commit: r265901 - stable/10/sys/kern Message-ID: <20140513150452.R2958@besplex.bde.org> In-Reply-To: <5371081A.2070703@freebsd.org> References: <201405120427.s4C4RAZf093033@svn.freebsd.org> <5370F110.5050502@freebsd.org> <20140512170322.GA2479@stack.nl> <5371081A.2070703@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 12 May 2014, Andrey Chernov wrote:
> On 12.05.2014 21:03, Jilles Tjoelker wrote:
>> On Mon, May 12, 2014 at 08:04:32PM +0400, Andrey Chernov wrote:
>>> On 12.05.2014 8:27, Don Lewis wrote:
>>>> + if (start + amask < start) {
>>>> + DPRINTF(("start+amask wrapped around\n"));
>>>> + goto out;
>>>> + }
>>
>>> Checking for overflow _after_ it happens is unportable and dangerous,
>>> since wrapping around is not only one possible result. They should be
>>> rewritten like that:
>>
>>> if (start > ULONG_MAX - amask)
>>
>> Unsigned types wrap around per the C standard. Overflow checking after
>> it happens is fine.
>>
>> You are right for signed types.
>
> You are right. The C Standard, 6.2.5, paragraph 9 [ISO/IEC 9899:2011],
> states:
Actually, there is a problem with promotions. The rman code is correct,
since all the types are hard-coded as u_long (even correctly spelled as
that), so we know that the promotions don't expand them. (Expansion would
allow start + amask to hold a larger value, and would also change the
type to signed.)
Similar code in vm is technically broken, since the types are are
usually vm_offset_t and that can be anything in theory. It might be
u_char or u_short. Both normally promote to int. POSIX requires
u_char to be 8 bits, so it is always too small for vm_offset_t, but
u_short can be large enough (e.g., u_short 64 bits, u_int 65 bits,
u_long 130 bits, u_long_long 260 bits, uintmax_t 520 bits for a
machine with a 64-bit address space but 65-bit registers :-).
> "A computation involving unsigned operands can never overflow, because a
> result that cannot be represented by the resulting unsigned integer type
> is reduced modulo the number that is one greater than the largest value
> that can be represented by the resulting type."
This is bad wording, since the "resulting type" isn't always unsigned
unless "unsigned operands" is interpreted as "operands, which are
unsigned after promotion".
Similar code in vm:
% int
% sys_mmap(td, uap)
% ...
% vm_offset_t addr;
% ...
% vm_size_t size, pageoff;
% ...
% if (addr + size < addr)
% return (EINVAL);
Here addr and size have logically different types, so they might have
quite different promotitions (largest wins). Typedefs are too hard
for me. Rewriting this using (__maxof(__typeof(foo)) - foo) (where foo
is one of the operands in the addition) works well here too.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140513150452.R2958>