This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
- From: Janne Blomqvist <blomqvist dot janne at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Fortran List <fortran at gcc dot gnu dot org>
- Date: Sat, 2 Aug 2014 00:09:24 +0300
- Subject: Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
- Authentication-results: sourceware.org; auth=none
- References: <CAO9iq9G3HyYRY1ObbTB81Gfb4oU7Aj4izc58oF3tdLcRnw6XMA at mail dot gmail dot com> <20140801124904 dot GG7393 at tucnak dot redhat dot com>
On Fri, Aug 1, 2014 at 3:49 PM, Jakub Jelinek <email@example.com> wrote:
> On Thu, Jul 31, 2014 at 11:32:12PM +0300, Janne Blomqvist wrote:
>> a while ago I committed a patch to trunk adding a function
>> xmallocarray to libgfortran, which is a malloc wrapper like xmalloc
>> but has two arguments and does an overflow check before multiplying
>> them together.
> That seems to be unnecessarily expensive for the common case where both
> nmemb and size are small.
> calloc in glibc uses something like following to avoid the division most of
> the time, if both nmemb and size are small enough then nmemb * size can't
> overflow. At least for 64-bit architectures small is smaller than 4GB
> for both numbers.
> 2014-08-01 Jakub Jelinek <firstname.lastname@example.org>
> * runtime/memory.c (xmallocarray): Avoid division for the common case.
> --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.000000000 +0200
> +++ libgfortran/runtime/memory.c 2014-08-01 14:41:08.385856116 +0200
> @@ -56,7 +56,9 @@ xmallocarray (size_t nmemb, size_t size)
> if (!nmemb || !size)
> size = nmemb = 1;
> - else if (nmemb > SIZE_MAX / size)
> +#define HALF_SIZE_T (((size_t) 1) << (__CHAR_BIT__ * sizeof (size_t) / 2))
> + else if (__builtin_expect ((nmemb | size) >= HALF_SIZE_T, 0)
> + && nmemb > SIZE_MAX / size)
> errno = ENOMEM;
> os_error ("Integer overflow in xmallocarray");
Nice, though as os_error() has the _Noreturn specifier the
__builtin_expect() is not necessary, right? In libgfortran.h we have
in the comment block above the likely/unlikely macros, which are
wrappers around __builtin_expect:
"...as __builtin_expect overrides the compiler
heuristic, do not use in conditions where one of the branches ends with a
call to a function with __attribute__((noreturn)): the compiler internal
heuristic will mark this branch as much less likely as unlikely() would
Ok for trunk/4.9/4.8. If you choose to leave the __builtin_expect
there, please explain why?