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
[PATCH, libfortran] Backport xmallocarray to 4.8/4.9 (CVE-2014-5044)
Hi, 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. https://gcc.gnu.org/viewcvs/gcc?limit_changes=0view=revisionrevision=211721 Originally I had no intentions of backporting this functionality to release branches, but subsequently Florian Weimer thought it was important enough to warrant a CVE number (CVE-2014-5044), so after some private discussions with Tobias and Jerry we agreed to backport. http://seclists.org/oss-sec/2014/q3/230 https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg01136.html https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg01135.html -- Janne Blomqvist
