This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] [BZ #15855] malloc: Check for integer overflow in pvalloc.
- From: Rich Felker <dalias at aerifal dot cx>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: Will Newton <will dot newton at linaro dot org>, libc-alpha at sourceware dot org, patches at linaro dot org
- Date: Tue, 10 Sep 2013 00:46:00 -0400
- Subject: Re: [PATCH v2] [BZ #15855] malloc: Check for integer overflow in pvalloc.
- Authentication-results: sourceware.org; auth=none
- References: <521327C7 dot 3020501 at linaro dot org> <20130910024043 dot GA4306 at spoyarek dot pnq dot redhat dot com>
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