Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
Jakub Jelinek wrote: On Sat, Aug 02, 2014 at 12:09:24AM +0300, Janne Blomqvist wrote: --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.0 +0200 +++ libgfortran/runtime/memory.c2014-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 The reason for __builtin_expect here was to make already the nmemb SIZE_MAX / size computation as unlikely, the noreturn predictor will of course DTRT with the {} block. But there is a difference in probability between __builtin_expect and noreturn. __builtin_expect had until two years ago a probability of 99%, now it has a probability of only 90% (which is tunable with a -param)* – while noreturn has a higher probability. Thus, at least if you had used else if (unlikely(... ...)) os_error you would have made the basic block with os_error more likely than without the unlikely (alias __builtin_expect). However, I don't know what happens with using unlikely(cond1) cond2. Tobias * Or internally in the compiler only, by passing a third argument.
Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
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 ja...@redhat.com * runtime/memory.c (xmallocarray): Avoid division for the common case. --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.0 +0200 +++ libgfortran/runtime/memory.c2014-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); Jakub
Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
On Fri, Aug 1, 2014 at 3:49 PM, Jakub Jelinek ja...@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 ja...@redhat.com * runtime/memory.c (xmallocarray): Avoid division for the common case. --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.0 +0200 +++ libgfortran/runtime/memory.c2014-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
Re: [PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
On Sat, Aug 02, 2014 at 12:09:24AM +0300, Janne Blomqvist wrote: --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.0 +0200 +++ libgfortran/runtime/memory.c2014-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 The reason for __builtin_expect here was to make already the nmemb SIZE_MAX / size computation as unlikely, the noreturn predictor will of course DTRT with the {} block. Jakub