Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

On Fri, Aug 1, 2014 at 3:49 PM, Jakub Jelinek <jakub@redhat.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  <jakub@redhat.com>
>
>         * 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
do."

Ok for trunk/4.9/4.8. If you choose to leave the __builtin_expect
there, please explain why?

-- 
Janne Blomqvist