Re: [PATCH v2] [BZ #15855] malloc: Check for integer overflow in pvalloc.

[Rich Felker <dalias at aerifal dot cx>]

On Tue, Sep 10, 2013 at 08:10:44AM +0530, Siddhesh Poyarekar wrote:
> > diff --git a/malloc/malloc.c b/malloc/malloc.c
> > index be472b2..7468758 100644
> > --- a/malloc/malloc.c
> > +++ b/malloc/malloc.c
> > @@ -3082,6 +3082,10 @@ __libc_pvalloc(size_t bytes)
> >    size_t page_mask = GLRO(dl_pagesize) - 1;
> >    size_t rounded_bytes = (bytes + page_mask) & ~(page_mask);
> > 
> > +  /* Check for overflow.  */
> > +  if (bytes + 2*pagesz + MINSIZE < bytes)
> > +    return 0;
> > +
> 
> It might be safer to use SIZE_MAX instead of relying on overflow
> behaviour of the processor:

This code does not "rely on overflow behavior of the processor".
Unsigned arithmetic cannot overflow. It's performed modulo one plus
the maximum value of the type.

In general, expressions of the form "a+b+c < a" are not safe to check
for overflow; since even if a+b<a, a+b+c>=a is possible. However,
since b+c (i.e. 2*pagesz+MINSIZE) cannot overflow in the above
expression, the check is safe.

>     if (SIZE_MAX - 2 * pagesz - MINSIZE < bytes)
>       return 0;

I do prefer this form, but you got it backwards in the process of
Yoda-fying it. It should be:

     if (bytes > SIZE_MAX - 2 * pagesz - MINSIZE)

and the "value being tested" should be on the left-hand side of a
comparison (i.e. non-Yoda-fied).

By the way, I think the code is failing to set errno, too.

Rich