Fix integer overflows in internal memalign and malloc functions [BZ #22343]

When posix_memalign is called with an alignment less than MALLOC_ALIGNMENT
and a requested size close to SIZE_MAX, it falls back to malloc code
(because the alignment of a block returned by malloc is sufficient to
satisfy the call).  In this case, an integer overflow in _int_malloc leads
to posix_memalign incorrectly returning successfully.

Upon fixing this and writing a somewhat thorough regression test, it was
discovered that when posix_memalign is called with an alignment larger than
MALLOC_ALIGNMENT (so it uses _int_memalign instead) and a requested size
close to SIZE_MAX, a different integer overflow in _int_memalign leads to
posix_memalign incorrectly returning successfully.

Both integer overflows affect other memory allocation functions that use
_int_malloc (one affected malloc in x86) or _int_memalign as well.

This commit fixes both integer overflows.  In addition to this, it adds a
regression test to guard against false successful allocations by the
following memory allocation functions when called with too-large allocation
sizes and, where relevant, various valid alignments:
malloc, realloc, calloc, reallocarray, memalign, posix_memalign,
aligned_alloc, valloc, and pvalloc.

ChangeLog:

2018-01-04  Arjun Shankar  <arjun@redhat.com>

	[BZ #22343]
	* malloc/malloc.c (checked_request2size): call REQUEST_OUT_OF_RANGE
	after padding.
	(_int_memalign): check for integer overflow before calling
	_int_malloc.
	* malloc/tst-malloc-too-large.c: New test.
	* malloc/Makefile: Add tst-malloc-too-large.
---
Two notes:

1. This bug has been flagged as security relevant and needs a CVE ID.

2. Although this patch stands on its own, a previous version of the test
was reviewed by Florian. I dropped it when committing the earlier (related)
patch pending this new change. Here is that review:
https://sourceware.org/ml/libc-alpha/2017-11/msg00325.html
Since then, I have expanded the scope of the test a little.

 malloc/Makefile               |   1 +
 malloc/malloc.c               |  32 ++++--
 malloc/tst-malloc-too-large.c | 253 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+), 8 deletions(-)
 create mode 100644 malloc/tst-malloc-too-large.c

On Thu, Jan 4, 2018 at 12:02 PM, Arjun Shankar <arjun.is@lostca.se> wrote:
> When posix_memalign is called with an alignment less than MALLOC_ALIGNMENT
> and a requested size close to SIZE_MAX, it falls back to malloc code
> (because the alignment of a block returned by malloc is sufficient to
> satisfy the call).  In this case, an integer overflow in _int_malloc leads
> to posix_memalign incorrectly returning successfully.

I'm not qualified to review malloc patches, but I want to point out
that if this patch is approved it should be backported to all active
release branches.

zw

Arjun Shankar wrote:
> +      (sz) = request2size (req);        \
> +      if (((sz) < (req))

This does not work correctly if sz is signed, which is allowed by the current 
API: INTERNAL_SIZE_T is allowed to be signed, and sz might be INTERNAL_SIZE_T.

Perhaps the best fix would be to remove INTERNAL_SIZE_T and replace it with 
size_t everywhere; there's no real reason for INTERNAL_SIZE_T. Alternatively, we 
could change the documentation for INTERNAL_SIZE_T to say that it must be an 
unsigned type; this would be a simpler patch.

> +  /* Check for overflow.  */
> +  if (nb > SIZE_MAX - alignment - MINSIZE)
> +    {
> +      __set_errno (ENOMEM);
> +      return 0;
> +    }

This causes the code to do an unnecessary overflow check, as it already checked 
nb against a (wrong) upper bound earlier.

How about something like the attached patch for the code, instead? (I haven't 
tested this at all.)
From 0c48d38733ba40c9f0f6561c9736e6b98da930de Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 4 Jan 2018 12:25:14 -0800
Subject: [PATCH] [BZ #22343]

* malloc/malloc.c (usize2tidx): Remove; unused.
(requestalign2size): New macro.  Define request2size in its terms.
(checked_requestalign2size): New macro.
Define checked_request2size in its terms.
(_int_memalign): Use it.
---
 ChangeLog                |  8 ++++++++
 malloc/malloc-internal.h | 15 +++++----------
 malloc/malloc.c          | 31 ++++++++++++++++++-------------
 3 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5be91cf978..0b422836bb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2018-01-04  Paul Eggert  <eggert@cs.ucla.edu>
+
+	* malloc/malloc.c (usize2tidx): Remove; unused.
+	(requestalign2size): New macro.  Define request2size in its terms.
+	(checked_requestalign2size): New macro.
+	Define checked_request2size in its terms.
+	(_int_memalign): Use it.
+
 2018-01-04  Florian Weimer  <fweimer@redhat.com>
 
 	[BZ #22667]
diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
index ad054508ee..8d0a68b78b 100644
--- a/malloc/malloc-internal.h
+++ b/malloc/malloc-internal.h
@@ -27,9 +27,7 @@
 
    The default version is the same as size_t.
 
-   While not strictly necessary, it is best to define this as an
-   unsigned type, even if size_t is a signed type. This may avoid some
-   artificial size limitations on some systems.
+   This type must be unsigned.
 
    On a 64-bit machine, you may be able to reduce malloc overhead by
    defining INTERNAL_SIZE_T to be a 32 bit `unsigned int' at the
@@ -40,17 +38,14 @@
    potential advantages of decreasing size_t word size.
 
    Implementors: Beware of the possible combinations of:
-     - INTERNAL_SIZE_T might be signed or unsigned, might be 32 or 64 bits,
+     - INTERNAL_SIZE_T might be 32 or 64 bits,
        and might be the same width as int or as long
      - size_t might have different width and signedness as INTERNAL_SIZE_T
      - int and long might be 32 or 64 bits, and might be the same width
 
-   To deal with this, most comparisons and difference computations
-   among INTERNAL_SIZE_Ts should cast them to unsigned long, being
-   aware of the fact that casting an unsigned int to a wider long does
-   not sign-extend. (This also makes checking for negative numbers
-   awkward.) Some of these casts result in harmless compiler warnings
-   on some systems.  */
+   Take care when comparing INTERNAL_SIZE_T to signed integer values,
+   as the comparisons might (or might not) treat negative signed
+   values as if they were large unsigned values.  */
 #ifndef INTERNAL_SIZE_T
 # define INTERNAL_SIZE_T size_t
 #endif
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 48106f9bd4..9e8a10c9f5 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -312,8 +312,6 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
 
 /* When "x" is from chunksize().  */
 # define csize2tidx(x) (((x) - MINSIZE + MALLOC_ALIGNMENT - 1) / MALLOC_ALIGNMENT)
-/* When "x" is a user-provided size.  */
-# define usize2tidx(x) csize2tidx (request2size (x))
 
 /* With rounding and alignment, the bins are...
    idx 0   bytes 0..24 (64-bit) or 0..12 (32-bit)
@@ -1220,18 +1218,27 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 /* pad request bytes into a usable size -- internal version */
 
 #define request2size(req)                                         \
-  (((req) + SIZE_SZ + MALLOC_ALIGN_MASK < MINSIZE)  ?             \
-   MINSIZE :                                                      \
-   ((req) + SIZE_SZ + MALLOC_ALIGN_MASK) & ~MALLOC_ALIGN_MASK)
+  requestalign2size(req, MALLOC_ALIGNMENT)
+#define requestalign2size(req, align)				  \
+  (((req) + SIZE_SZ + (align) - 1 < MINSIZE)			  \
+   ? MINSIZE							  \
+   : ((req) + SIZE_SZ + (align) - 1) & ~((align) - 1))
 
 /*  Same, except also perform argument check */
 
 #define checked_request2size(req, sz)                             \
-  if (REQUEST_OUT_OF_RANGE (req)) {					      \
-      __set_errno (ENOMEM);						      \
-      return 0;								      \
-    }									      \
-  (sz) = request2size (req);
+  checked_requestalign2size(req, MALLOC_ALIGNMENT, sz)
+#define checked_requestalign2size(req, align, sz)			\
+  do									\
+    {									\
+      (sz) = requestalign2size (req, align);				\
+      if ((sz) < (req) || REQUEST_OUT_OF_RANGE (sz))			\
+	{								\
+	  __set_errno (ENOMEM);						\
+	  return 0;							\
+	}								\
+    }									\
+  while (0)
 
 /*
    --------------- Physical chunk operations ---------------
@@ -4662,9 +4669,7 @@ _int_memalign (mstate av, size_t alignment, size_t bytes)
   unsigned long remainder_size;   /* its size */
   INTERNAL_SIZE_T size;
 
-
-
-  checked_request2size (bytes, nb);
+  checked_requestalign2size (bytes, alignment, nb);
 
   /*
      Strategy: find a spot within that chunk that meets the alignment

On 01/04/2018 12:32 PM, Paul Eggert wrote:
> Arjun Shankar wrote:
>> +      (sz) = request2size (req);        \
>> +      if (((sz) < (req))
> 
> This does not work correctly if sz is signed, which is allowed by the
> current API: INTERNAL_SIZE_T is allowed to be signed, and sz might be
> INTERNAL_SIZE_T.

Good catch. I had not noticed that malloc-internals.h could override
INTERNAL_SIZE_T and make the above check against a signed type, for
which such overflow is undefined behaviour.

The reason I didn't think this is that we have *tons* of checks that
would break if we actually made this signed, so the fix you request
is correct.

> Perhaps the best fix would be to remove INTERNAL_SIZE_T and replace
> it with size_t everywhere; there's no real reason for
> INTERNAL_SIZE_T. Alternatively, we could change the documentation for
> INTERNAL_SIZE_T to say that it must be an unsigned type; this would
> be a simpler patch.

I agree.

>> +  /* Check for overflow.  */
>> +  if (nb > SIZE_MAX - alignment - MINSIZE)
>> +    {
>> +      __set_errno (ENOMEM);
>> +      return 0;
>> +    }
> 
> This causes the code to do an unnecessary overflow check, as it
> already checked nb against a (wrong) upper bound earlier.

You have potential overflow from two sources, the minimum size,
and the alignment. There is some computational overlap in both
cases.

> How about something like the attached patch for the code, instead? (I
> haven't tested this at all.)

That looks pretty good, and is a good suggestion.

It would allow further cleanups, and with a constant MALLOC_ALIGNMENT
the compiler can optimize as required.

I would suggest the following next steps:

* Arjun, give Paul's patch a try, with my suggestions below and see
  if it passes your regression test.
* Repost v2, please include Paul's name in the ChangeLog for all the suggestions
  e.g. your name and Paul's.

> 0001-BZ-22343.patch
> 
> 
> From 0c48d38733ba40c9f0f6561c9736e6b98da930de Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 4 Jan 2018 12:25:14 -0800
> Subject: [PATCH] [BZ #22343]
> 
> * malloc/malloc.c (usize2tidx): Remove; unused.
> (requestalign2size): New macro.  Define request2size in its terms.
> (checked_requestalign2size): New macro.
> Define checked_request2size in its terms.
> (_int_memalign): Use it.
> ---
>  ChangeLog                |  8 ++++++++
>  malloc/malloc-internal.h | 15 +++++----------
>  malloc/malloc.c          | 31 ++++++++++++++++++-------------
>  3 files changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 5be91cf978..0b422836bb 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2018-01-04  Paul Eggert  <eggert@cs.ucla.edu>
> +
> +	* malloc/malloc.c (usize2tidx): Remove; unused.
> +	(requestalign2size): New macro.  Define request2size in its terms.
> +	(checked_requestalign2size): New macro.
> +	Define checked_request2size in its terms.
> +	(_int_memalign): Use it.
> +
>  2018-01-04  Florian Weimer  <fweimer@redhat.com>
>  
>  	[BZ #22667]
> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
> index ad054508ee..8d0a68b78b 100644
> --- a/malloc/malloc-internal.h
> +++ b/malloc/malloc-internal.h
> @@ -27,9 +27,7 @@
>  
>     The default version is the same as size_t.
>  
> -   While not strictly necessary, it is best to define this as an
> -   unsigned type, even if size_t is a signed type. This may avoid some
> -   artificial size limitations on some systems.
> +   This type must be unsigned.

OK.

>  
>     On a 64-bit machine, you may be able to reduce malloc overhead by
>     defining INTERNAL_SIZE_T to be a 32 bit `unsigned int' at the
> @@ -40,17 +38,14 @@
>     potential advantages of decreasing size_t word size.
>  
>     Implementors: Beware of the possible combinations of:
> -     - INTERNAL_SIZE_T might be signed or unsigned, might be 32 or 64 bits,
> +     - INTERNAL_SIZE_T might be 32 or 64 bits,

OK.

>         and might be the same width as int or as long
>       - size_t might have different width and signedness as INTERNAL_SIZE_T
>       - int and long might be 32 or 64 bits, and might be the same width
>  
> -   To deal with this, most comparisons and difference computations
> -   among INTERNAL_SIZE_Ts should cast them to unsigned long, being
> -   aware of the fact that casting an unsigned int to a wider long does
> -   not sign-extend. (This also makes checking for negative numbers
> -   awkward.) Some of these casts result in harmless compiler warnings
> -   on some systems.  */
> +   Take care when comparing INTERNAL_SIZE_T to signed integer values,
> +   as the comparisons might (or might not) treat negative signed
> +   values as if they were large unsigned values.  */
>  #ifndef INTERNAL_SIZE_T
>  # define INTERNAL_SIZE_T size_t
>  #endif

OK.

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 48106f9bd4..9e8a10c9f5 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -312,8 +312,6 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
>  
>  /* When "x" is from chunksize().  */
>  # define csize2tidx(x) (((x) - MINSIZE + MALLOC_ALIGNMENT - 1) / MALLOC_ALIGNMENT)
> -/* When "x" is a user-provided size.  */
> -# define usize2tidx(x) csize2tidx (request2size (x))

OK.

>  
>  /* With rounding and alignment, the bins are...
>     idx 0   bytes 0..24 (64-bit) or 0..12 (32-bit)
> @@ -1220,18 +1218,27 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  /* pad request bytes into a usable size -- internal version */
>  
>  #define request2size(req)                                         \
> -  (((req) + SIZE_SZ + MALLOC_ALIGN_MASK < MINSIZE)  ?             \
> -   MINSIZE :                                                      \
> -   ((req) + SIZE_SZ + MALLOC_ALIGN_MASK) & ~MALLOC_ALIGN_MASK)
> +  requestalign2size(req, MALLOC_ALIGNMENT)
> +#define requestalign2size(req, align)				  \
> +  (((req) + SIZE_SZ + (align) - 1 < MINSIZE)			  \
> +   ? MINSIZE							  \
> +   : ((req) + SIZE_SZ + (align) - 1) & ~((align) - 1))

Bespoke alignment code. Use ALIGN_UP or PTR_ALIGN_UP from libc-pointer-arith.h.

>  
>  /*  Same, except also perform argument check */
>  
>  #define checked_request2size(req, sz)                             \
> -  if (REQUEST_OUT_OF_RANGE (req)) {					      \
> -      __set_errno (ENOMEM);						      \
> -      return 0;								      \
> -    }									      \
> -  (sz) = request2size (req);
> +  checked_requestalign2size(req, MALLOC_ALIGNMENT, sz)
> +#define checked_requestalign2size(req, align, sz)			\
> +  do									\
> +    {									\
> +      (sz) = requestalign2size (req, align);				\
> +      if ((sz) < (req) || REQUEST_OUT_OF_RANGE (sz))			\
> +	{								\
> +	  __set_errno (ENOMEM);						\
> +	  return 0;							\
> +	}								\
> +    }									\
> +  while (0)

OK.

>  
>  /*
>     --------------- Physical chunk operations ---------------
> @@ -4662,9 +4669,7 @@ _int_memalign (mstate av, size_t alignment, size_t bytes)
>    unsigned long remainder_size;   /* its size */
>    INTERNAL_SIZE_T size;
>  
> -
> -
> -  checked_request2size (bytes, nb);
> +  checked_requestalign2size (bytes, alignment, nb);

OK.

>  
>    /*
>       Strategy: find a spot within that chunk that meets the alignment
> -- 2.14.3

Carlos wrote:

> * Arjun, give Paul's patch a try, with my suggestions below and see
>   if it passes your regression test.
> * Repost v2, please include Paul's name in the ChangeLog for all the suggestions
>   e.g. your name and Paul's.

I tried this and it failed with the new test case in my original patch
because it caused alignment to be added twice: first with the new use of
checked_requestalign2size then immediately after in the call to _int_malloc
around line 4688 in _int_memalign.

#1:
-  checked_request2size (bytes, nb);
+  checked_requestalign2size (bytes, alignment, nb);

#2:
> m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));

I thought this was a simple matter of removing the unnecessary addition...

> m = (char *) (_int_malloc (av, nb + MINSIZE));

...but that triggered several testsuite failures:

> FAIL: malloc/tst-malloc-thread-fail
> FAIL: malloc/tst-memalign
> FAIL: malloc/tst-posix_memalign
> FAIL: malloc/tst-pvalloc
> FAIL: malloc/tst-valloc

Before this change, this code was using checked_request2size which adds
MALLOC_ALIGNMENT and then aligns up which - while it makes no sense (to me)
in cases where the alignment is isn't MALLOC_ALIGNMENT - somehow seems to
work. It seems to me that cleaning this up might not be simple. In addition,
Paul has already said:

> Perhaps the best fix would be to remove INTERNAL_SIZE_T and replace
> it with size_t everywhere; there's no real reason for
> INTERNAL_SIZE_T. Alternatively, we could change the documentation for
> INTERNAL_SIZE_T to say that it must be an unsigned type; this would
> be a simpler patch.

Given this and given that, as Carlos says:

> The reason I didn't think this is that we have *tons* of checks that
> would break if we actually made this signed, so the fix you request
> is correct.

I vote that we consider reviewing and checking in my original attempt at a
fix prior to the 2.27 release, and in lieu of this we file a separate bug to
track the removal of technical debt that already exists in the code. I'll be
happy to take Paul's patch and run with it once master opens for development
again.

Cheers,
Arjun

On 01/16/2018 08:27 AM, Arjun Shankar wrote:
> I vote that we consider reviewing and checking in my original attempt at a
> fix prior to the 2.27 release, and in lieu of this we file a separate bug to
> track the removal of technical debt that already exists in the code. I'll be
> happy to take Paul's patch and run with it once master opens for development
> again.

I think an immediate fix is the right solution for 2.27, these overflows in
malloc are bad.

On 01/04/2018 09:02 AM, Arjun Shankar wrote:
> When posix_memalign is called with an alignment less than MALLOC_ALIGNMENT
> and a requested size close to SIZE_MAX, it falls back to malloc code
> (because the alignment of a block returned by malloc is sufficient to
> satisfy the call).  In this case, an integer overflow in _int_malloc leads
> to posix_memalign incorrectly returning successfully.
> 
> Upon fixing this and writing a somewhat thorough regression test, it was
> discovered that when posix_memalign is called with an alignment larger than
> MALLOC_ALIGNMENT (so it uses _int_memalign instead) and a requested size
> close to SIZE_MAX, a different integer overflow in _int_memalign leads to
> posix_memalign incorrectly returning successfully.
> 
> Both integer overflows affect other memory allocation functions that use
> _int_malloc (one affected malloc in x86) or _int_memalign as well.
> 
> This commit fixes both integer overflows.  In addition to this, it adds a
> regression test to guard against false successful allocations by the
> following memory allocation functions when called with too-large allocation
> sizes and, where relevant, various valid alignments:
> malloc, realloc, calloc, reallocarray, memalign, posix_memalign,
> aligned_alloc, valloc, and pvalloc.

High level:

You alter checked_request2size such that it performs a new overflow
check on the request->size conversion, and you add a new alignment-based
overflow check in _int_memalign to catch overflows with alignment greater
than MALLOC_ALIGNMENT.

Overall what you have done makes sense, but as Paul Eggert points out,
this code needs a real once-over to make it more readable and refactor
to minimize the number of times we compute overflow and alignments. This
however is outside the scope of minimally fixing this for 2.27.

This patch is OK with me if you chance the brace style as suggested below.
Please check this fix in as a fix for 2.27, since I'd like to see this fixed.

Design:

The test case here amazing, exactly what we needed.

The code in question is probably the minimal amount of code we want to change
to fix this. It makes sure that all checked_request2size calls make sure that
MALLOC_ALIGNMENT does not cause overflows, and that with that internal padding
taken into account that we do not get too close to SIZE_MAX (within 2*MINSIZE).

Implementation:

The test case looks perfect to me, the right blend of testing the corner cases
and covering some other regions of interest.

I would like to see the macros in malloc use ({...}) because it's easier to
read and simpler.

> ChangeLog:
> 
> 2018-01-04  Arjun Shankar  <arjun@redhat.com>
> 
> 	[BZ #22343]
> 	* malloc/malloc.c (checked_request2size): call REQUEST_OUT_OF_RANGE
> 	after padding.
> 	(_int_memalign): check for integer overflow before calling
> 	_int_malloc.
> 	* malloc/tst-malloc-too-large.c: New test.
> 	* malloc/Makefile: Add tst-malloc-too-large.
> ---
> Two notes:
> 
> 1. This bug has been flagged as security relevant and needs a CVE ID.
> 
> 2. Although this patch stands on its own, a previous version of the test
> was reviewed by Florian. I dropped it when committing the earlier (related)
> patch pending this new change. Here is that review:
> https://sourceware.org/ml/libc-alpha/2017-11/msg00325.html
> Since then, I have expanded the scope of the test a little.
> 
>  malloc/Makefile               |   1 +
>  malloc/malloc.c               |  32 ++++--
>  malloc/tst-malloc-too-large.c | 253 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 278 insertions(+), 8 deletions(-)
>  create mode 100644 malloc/tst-malloc-too-large.c
> 
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 4266c2b66b..17873e67c4 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -36,6 +36,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>  	 tst-alloc_buffer \
>  	 tst-malloc-tcache-leak \
>  	 tst-malloc_info \
> +	 tst-malloc-too-large \

OK. Add test.

>  
>  tests-static := \
>  	 tst-interpose-static-nothread \
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 48106f9bd4..2bcf64cdb6 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1224,14 +1224,23 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     MINSIZE :                                                      \
>     ((req) + SIZE_SZ + MALLOC_ALIGN_MASK) & ~MALLOC_ALIGN_MASK)
>  
> -/*  Same, except also perform argument check */
> -
> -#define checked_request2size(req, sz)                             \
> -  if (REQUEST_OUT_OF_RANGE (req)) {					      \
> -      __set_errno (ENOMEM);						      \
> -      return 0;								      \
> -    }									      \
> -  (sz) = request2size (req);
> +/* Same, except also perform an argument and result check.  First, we check
> +   that the padding done by request2size didn't result in an integer
> +   overflow.  Then we check (using REQUEST_OUT_OF_RANGE) that the resulting
> +   size isn't so large that a later alignment would lead to another integer
> +   overflow.  */
> +#define checked_request2size(req, sz)   \
> +  do                                    \
> +    {                                   \
> +      (sz) = request2size (req);        \

OK, this rounds req up to an aligned minimum chunk size first.

> +      if (((sz) < (req))                \

OK, look for overflow in request2size (new check).

> +          || REQUEST_OUT_OF_RANGE (sz)) \

OK, still look for request that is too big.

> +        {                               \
> +          __set_errno (ENOMEM);         \
> +          return 0;                     \
> +        }                               \
> +    }                                   \
> +    while (0)

Please rewrite this as:

#define foo(...) \
({			\
	...		\
})

The nested braces are less verbose IMHO.

>  
>  /*
>     --------------- Physical chunk operations ---------------
> @@ -4672,6 +4681,13 @@ _int_memalign (mstate av, size_t alignment, size_t bytes)
>     */
>  
>  
> +  /* Check for overflow.  */
> +  if (nb > SIZE_MAX - alignment - MINSIZE)

OK, look for alignment requested overflow in _int_memalign.

> +    {
> +      __set_errno (ENOMEM);
> +      return 0;
> +    }
> +
>    /* Call malloc with worst case padding to hit alignment. */
>  
>    m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
> diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
> new file mode 100644
> index 0000000000..705a07d850
> --- /dev/null
> +++ b/malloc/tst-malloc-too-large.c
> @@ -0,0 +1,253 @@
> +/* Test and verify that too-large memory allocations fail with ENOMEM.

OK.

> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Bug 22375 reported a regression in malloc where if after malloc'ing then
> +   free'ing a small block of memory, malloc is then called with a really
> +   large size argument (close to SIZE_MAX): instead of returning NULL and
> +   setting errno to ENOMEM, malloc incorrectly returns the previously
> +   allocated block instead.  Bug 22343 reported a similar case where
> +   posix_memalign incorrectly returns successfully when called with an with
> +   a really large size argument.
> +
> +   Both of these were caused by integer overflows in the allocator when it
> +   was trying to pad the requested size to allow for book-keeping or
> +   alignment.  This test guards against such bugs by repeatedly allocating
> +   and freeing small blocks of memory then trying to allocate various block
> +   sizes larger than the memory bus width of 64-bit targets, or almost
> +   as large as SIZE_MAX on 32-bit targets supported by glibc.  In each case,
> +   it verifies that such impossibly large allocations correctly fail.  */
> +
> +
> +#include <stdlib.h>
> +#include <malloc.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <sys/resource.h>
> +#include <libc-diag.h>
> +#include <support/check.h>
> +#include <unistd.h>
> +#include <sys/param.h>
> +
> +
> +/* This function prepares for each 'too-large memory allocation' test by
> +   performing a small successful malloc/free and resetting errno prior to
> +   the actual test.  */

OK.

> +static void
> +test_setup (void)
> +{
> +  void *volatile ptr = malloc (16);
> +  TEST_VERIFY_EXIT (ptr != NULL);
> +  free (ptr);
> +  errno = 0;
> +}

OK.

> +
> +
> +/* This function tests each of:
> +   - malloc (SIZE)
> +   - realloc (PTR_FOR_REALLOC, SIZE)
> +   - for various values of NMEMB:
> +    - calloc (NMEMB, SIZE/NMEMB)
> +    - calloc (SIZE/NMEMB, NMEMB)
> +    - reallocarray (PTR_FOR_REALLOC, NMEMB, SIZE/NMEMB)
> +    - reallocarray (PTR_FOR_REALLOC, SIZE/NMEMB, NMEMB)
> +   and precedes each of these tests with a small malloc/free before it.  */
> +static void
> +test_large_allocations (size_t size)
> +{
> +  void * ptr_to_realloc;
> +
> +  test_setup ();
> +  TEST_VERIFY (malloc (size) == NULL);
> +  TEST_VERIFY (errno == ENOMEM);
> +
> +  ptr_to_realloc = malloc (16);
> +  TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
> +  test_setup ();
> +  TEST_VERIFY (realloc (ptr_to_realloc, size) == NULL);
> +  TEST_VERIFY (errno == ENOMEM);
> +  free (ptr_to_realloc);
> +
> +  for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2)
> +    if ((size % nmemb) == 0)
> +      {
> +        test_setup ();
> +        TEST_VERIFY (calloc (nmemb, size / nmemb) == NULL);
> +        TEST_VERIFY (errno == ENOMEM);
> +
> +        test_setup ();
> +        TEST_VERIFY (calloc (size / nmemb, nmemb) == NULL);
> +        TEST_VERIFY (errno == ENOMEM);
> +
> +        ptr_to_realloc = malloc (16);
> +        TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
> +        test_setup ();
> +        TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
> +        TEST_VERIFY (errno == ENOMEM);
> +        free (ptr_to_realloc);
> +
> +        ptr_to_realloc = malloc (16);
> +        TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
> +        test_setup ();
> +        TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
> +        TEST_VERIFY (errno == ENOMEM);
> +        free (ptr_to_realloc);
> +      }
> +    else
> +      break;
> +}

OK.

> +
> +
> +static long pagesize;
> +
> +/* This function tests the following aligned memory allocation functions
> +   using several valid alignments and precedes each allocation test with a
> +   small malloc/free before it:
> +   memalign, posix_memalign, aligned_alloc, valloc, pvalloc.  */
> +static void
> +test_large_aligned_allocations (size_t size)
> +{
> +  /* PTR stores the result of posix_memalign but since all those calls
> +     should fail, posix_memalign should never touch PTR.  We set it to
> +     NULL here and later on we check that it remains NULL after each
> +     posix_memalign call.  */
> +  void * ptr = NULL;
> +
> +  size_t align;
> +
> +  /* All aligned memory allocation functions expect an alignment that is a
> +     power of 2.  Given this, we test each of them with every valid
> +     alignment from 1 thru PAGESIZE.  */
> +  for (align = 1; align <= pagesize; align *= 2)
> +    {
> +      test_setup ();
> +      TEST_VERIFY (memalign (align, size) == NULL);
> +      TEST_VERIFY (errno == ENOMEM);
> +
> +      /* posix_memalign expects an alignment that is a power of 2 *and* a
> +         multiple of sizeof (void *).  */
> +      if ((align % sizeof (void *)) == 0)
> +        {
> +          test_setup ();
> +          TEST_VERIFY (posix_memalign (&ptr, align, size) == ENOMEM);
> +          TEST_VERIFY (ptr == NULL);
> +        }
> +
> +      /* aligned_alloc expects a size that is a multiple of alignment.  */
> +      if ((size % align) == 0)
> +        {
> +          test_setup ();
> +          TEST_VERIFY (aligned_alloc (align, size) == NULL);
> +          TEST_VERIFY (errno == ENOMEM);
> +        }
> +    }
> +
> +  /* Both valloc and pvalloc return page-aligned memory.  */
> +
> +  test_setup ();
> +  TEST_VERIFY (valloc (size) == NULL);
> +  TEST_VERIFY (errno == ENOMEM);
> +
> +  test_setup ();
> +  TEST_VERIFY (pvalloc (size) == NULL);
> +  TEST_VERIFY (errno == ENOMEM);
> +}
> +
> +
> +#define FOURTEEN_ON_BITS ((1UL << 14) - 1)
> +#define FIFTY_ON_BITS ((1UL << 50) - 1)
> +
> +
> +static int
> +do_test (void)
> +{
> +
> +#if __WORDSIZE >= 64
> +
> +  /* This test assumes that none of the supported targets have an address
> +     bus wider than 50 bits, and that therefore allocations for sizes wider
> +     than 50 bits will fail.  Here, we ensure that the assumption continues
> +     to be true in the future when we might have address buses wider than 50
> +     bits.  */
> +
> +  struct rlimit alloc_size_limit
> +    = {
> +        .rlim_cur = FIFTY_ON_BITS,
> +        .rlim_max = FIFTY_ON_BITS
> +      };
> +

OK.

> +  setrlimit (RLIMIT_AS, &alloc_size_limit);
> +
> +#endif /* __WORDSIZE >= 64 */
> +
> +  DIAG_PUSH_NEEDS_COMMENT;
> +#if __GNUC_PREREQ (7, 0)
> +  /* GCC 7 warns about too-large allocations; here we want to test
> +     that they fail.  */
> +  DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");

OK.

> +#endif
> +
> +  /* Aligned memory allocation functions need to be tested up to alignment
> +     size equivalent to page size, which should be a power of 2.  */
> +  pagesize = sysconf (_SC_PAGESIZE);
> +  TEST_VERIFY_EXIT (powerof2 (pagesize));
> +
> +  /* Loop 1: Ensure that all allocations with SIZE close to SIZE_MAX, i.e.
> +     in the range (SIZE_MAX - 2^14, SIZE_MAX], fail.
> +
> +     We can expect that this range of allocation sizes will always lead to
> +     an allocation failure on both 64 and 32 bit targets, because:
> +
> +     1. no currently supported 64-bit target has an address bus wider than
> +     50 bits -- and (2^64 - 2^14) is much wider than that;
> +
> +     2. on 32-bit targets, even though 2^32 is only 4 GB and potentially
> +     addressable, glibc itself is more than 2^14 bytes in size, and
> +     therefore once glibc is loaded, less than (2^32 - 2^14) bytes remain
> +     available.  */
> +
> +  for (size_t i = 0; i <= FOURTEEN_ON_BITS; i++)
> +    {
> +      test_large_allocations (SIZE_MAX - i);
> +      test_large_aligned_allocations (SIZE_MAX - i);
> +    }

OK.

> +
> +#if __WORDSIZE >= 64
> +  /* On 64-bit targets, we need to test a much wider range of too-large
> +     sizes, so we test at intervals of (1 << 50) that allocation sizes
> +     ranging from SIZE_MAX down to (1 << 50) fail:
> +     The 14 MSBs are decremented starting from "all ON" going down to 1,
> +     the 50 LSBs are "all ON" and then "all OFF" during every iteration.  */

OK.

> +  for (size_t msbs = FOURTEEN_ON_BITS; msbs >= 1; msbs--)
> +    {
> +      size_t size = (msbs << 50) | FIFTY_ON_BITS;
> +      test_large_allocations (size);
> +      test_large_aligned_allocations (size);
> +
> +      size = msbs << 50;
> +      test_large_allocations (size);
> +      test_large_aligned_allocations (size);
> +    }
> +#endif /* __WORDSIZE >= 64 */
> +
> +  DIAG_POP_NEEDS_COMMENT;

OK.

> +
> +  return 0;
> +}
> +
> +
> +#include <support/test-driver.c>
>

On 01/16/2018 02:31 PM, Carlos O'Donell wrote:
> Please rewrite this as:
> 
> #define foo(...) \
> ({			\
> 	...		\
> })
> 
> The nested braces are less verbose IMHO.

Note that requeset2size (above) can't use nested braces because
it's involved in constant sized array expressions outside
of a normal function.

Carlos O'Donell wrote:
> I think an immediate fix is the right solution for 2.27, these overflows in
> malloc are bad.

Yes, that sounds right to me too. Though we really need to fix the more-general 
problem in the not-too-distant future.

On 01/16/2018 10:27 PM, Paul Eggert wrote:
> Carlos O'Donell wrote:
>> I think an immediate fix is the right solution for 2.27, these overflows in
>> malloc are bad.
> 
> Yes, that sounds right to me too. Though we really need to fix the
> more-general problem in the not-too-distant future.

Agreed. Thanks for your support Paul. As Arjun mentioned we'll refactor this
in 2.28.

These checks should be done at the entry points, and a good cleanup moves
them all outward to such entry points and avoids duplicating the work in
each internal function.