Re: [RFC][PATCH v2] Add reallocarray function.

["Rüdiger Sonderfeld" <ruediger@c-plusplus.de>]

Oh, I think I've introduced a mistake.  The prototype should be

static inline bool
check_mul_overflow(size_t l, size_t r,
                   INTERNAL_SIZE_T *result)

since INTENRAL_SIZE_T apparently can be signed.

Regards,
Rüdiger

On Sunday 18 May 2014 23:43:05 Rüdiger Sonderfeld wrote:
> Hello Paul,
> 
> > Instead of cut and pasting this code from calloc, please refactor so
> > that the code is present only once, with the goal of optimizing it when
> > the GCC folks get their act together and have a function like the
> > __builtin_umul_overflow function that Clang has had since January.  This
> > will let calloc and reallocarray do the unsigned multiplication and
> > inspect the hardware's overflow bit directly, which is nicer than the
> > above hackery.
> 
> thanks for your feedback.  I've changed the patch to add a
> `check_mul_overflow' function.
> 
> Regards,
> Rüdiger
> 
> ---- 8< ------------------------------------------------------ >8 ----
> 
> The reallocarray function is an extension from OpenBSD.  It is an
> integer-overflow-safe replacement for realloc(p, X*Y) and
> malloc(X*Y) (realloc(NULL, X*Y)).  It can therefore help in preventing
> certain security issues in code.
> 
> See
> http://www.openbsd.org/cgi-bin/man.cgi?query=reallocarray&sektion=3&manpath=
> OpenBSD+Current ---
>  ChangeLog                 |  11 ++++
>  malloc/Makefile           |   2 +-
>  malloc/Versions           |   4 ++
>  malloc/malloc.c           |  45 ++++++++++---
>  malloc/malloc.h           |   8 +++
>  malloc/tst-reallocarray.c | 160
> ++++++++++++++++++++++++++++++++++++++++++++++
>  stdlib/stdlib.h           |   7 ++
>  7 files changed, 226 insertions(+), 11 deletions(-)
>  create mode 100644 malloc/tst-reallocarray.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index c606b0d..1e142e1 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,14 @@
> +2014-05-18  Rüdiger Sonderfeld  <ruediger@c-plusplus.de>
> +
> +	* malloc/Versions: Add reallocarray and __libc_rallocarray.
> +	* malloc/Makefile (tests): Add tst-reallocarray.c.
> +	* malloc/tst-reallocarray.c: New test file.
> +	* malloc/malloc.h (reallocarray): New declaration.
> +	* stdlib/stdlib.h (reallocarray): Likewise.
> +	* malloc/malloc.c (check_mul_overflow): New inline function.
> +	(__libc_reallocarray): New function.
> +	(__libc_calloc): Use `check_mul_overflow'.
> +
>  2014-05-17  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
>  	[BZ #16958]
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 7a716f9..1b415ae 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -26,7 +26,7 @@ dist-headers := malloc.h
>  headers := $(dist-headers) obstack.h mcheck.h
>  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>  	 tst-mallocstate tst-mcheck tst-mallocfork tst-trim1 \
> -	 tst-malloc-usable tst-realloc tst-posix_memalign \
> +	 tst-malloc-usable tst-realloc tst-reallocarray tst-posix_memalign \
>  	 tst-pvalloc tst-memalign tst-mallopt
>  test-srcs = tst-mtrace
> 
> diff --git a/malloc/Versions b/malloc/Versions
> index 7ca9bdf..64fade5 100644
> --- a/malloc/Versions
> +++ b/malloc/Versions
> @@ -61,6 +61,10 @@ libc {
>    GLIBC_2.16 {
>      aligned_alloc;
>    }
> +  GLIBC_2.20 {
> +    __libc_reallocarray;
> +    reallocarray;
> +  }
>    GLIBC_PRIVATE {
>      # Internal startup hook for libpthread.
>      __libc_malloc_pthread_startup;
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 1120d4d..822e400 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2943,6 +2943,36 @@ void *weak_variable (*__memalign_hook)
>  }
>  libc_hidden_def (__libc_free)
> 
> +static inline bool
> +check_mul_overflow(INTERNAL_SIZE_T l, INTERNAL_SIZE_T r,
> +                   INTERNAL_SIZE_T *result)
> +{
> +  /* size_t is unsigned so the behavior on overflow is defined.  */
> +  *result = l * r;
> +#define HALF_INTERNAL_SIZE_T                                    \
> +  (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2))
> +  if (__glibc_unlikely ((l | r) >= HALF_INTERNAL_SIZE_T))
> +    {
> +      if (r != 0 && *result / r != l)
> +        return true;
> +    }
> +  return false;
> +#undef HALF_INTERNAL_SIZE_T
> +}
> +
> +void *
> +__libc_reallocarray(void *optr, size_t nmemb, size_t elem_size)
> +{
> +  INTERNAL_SIZE_T bytes;
> +  if (check_mul_overflow(nmemb, elem_size, &bytes))
> +    {
> +      __set_errno (ENOMEM);
> +      return 0;
> +    }
> +  else
> +    return __libc_realloc (optr, bytes);
> +}
> +
>  void *
>  __libc_realloc (void *oldmem, size_t bytes)
>  {
> @@ -3153,17 +3183,10 @@ void *weak_variable (*__memalign_hook)
>    unsigned long nclears;
>    INTERNAL_SIZE_T *d;
> 
> -  /* size_t is unsigned so the behavior on overflow is defined.  */
> -  bytes = n * elem_size;
> -#define HALF_INTERNAL_SIZE_T \
> -  (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2))
> -  if (__builtin_expect ((n | elem_size) >= HALF_INTERNAL_SIZE_T, 0))
> +  if (check_mul_overflow(n, elem_size, &bytes))
>      {
> -      if (elem_size != 0 && bytes / elem_size != n)
> -        {
> -          __set_errno (ENOMEM);
> -          return 0;
> -        }
> +      __set_errno (ENOMEM);
> +      return 0;
>      }
> 
>    void *(*hook) (size_t, const void *) =
> @@ -5171,6 +5194,8 @@ struct mallinfo
>  strong_alias (__libc_malloc, __malloc) strong_alias (__libc_malloc, malloc)
> strong_alias (__libc_memalign, __memalign)
>  weak_alias (__libc_memalign, memalign)
> +strong_alias (__libc_reallocarray, __reallocarray)
> +  strong_alias (__libc_reallocarray, reallocarray)
>  strong_alias (__libc_realloc, __realloc) strong_alias (__libc_realloc,
> realloc)
>  strong_alias (__libc_valloc, __valloc) weak_alias (__libc_valloc, valloc)
>  strong_alias (__libc_pvalloc, __pvalloc) weak_alias (__libc_pvalloc,
> pvalloc) diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 30bb91a..db7e5a9 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -49,6 +49,14 @@ extern void *calloc (size_t __nmemb, size_t __size)
>  extern void *realloc (void *__ptr, size_t __size)
>  __THROW __attribute_warn_unused_result__;
> 
> +/* Re-allocate the previously allocated block in PTR, making the new
> +   block large enough for NMEMB elements of SIZE bytes each.  */
> +/* __attribute_malloc__ is not used, because if realloc returns
> +   the same pointer that was passed to it, aliasing needs to be allowed
> +   between objects pointed by the old and new pointers.  */
> +extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
> +     __THROW __attribute_warn_unused_result__;
> +
>  /* Free a block allocated by `malloc', `realloc' or `calloc'.  */
>  extern void free (void *__ptr) __THROW;
> 
> diff --git a/malloc/tst-reallocarray.c b/malloc/tst-reallocarray.c
> new file mode 100644
> index 0000000..b85825e
> --- /dev/null
> +++ b/malloc/tst-reallocarray.c
> @@ -0,0 +1,160 @@
> +/* Copyright (C) 2014 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/>.  */
> +
> +#include <errno.h>
> +#include <malloc.h>
> +#include <stdio.h>
> +#include <math.h>
> +#include <string.h>
> +
> +static int errors = 0;
> +
> +static void
> +merror (const char *msg)
> +{
> +  ++errors;
> +  printf ("Error: %s.\n", msg);
> +}
> +
> +static int
> +do_test (void)
> +
> +{
> +  void *ptr = NULL;
> +  void *ptr2 = NULL;
> +  unsigned char *c;
> +  size_t i;
> +  int ok;
> +  const size_t max = ~(size_t)0;
> +  size_t a, b;
> +
> +  /* Test overflow detection.  */
> +  errno = 0;
> +  ptr = reallocarray (NULL, max, 2);
> +  if (ptr)
> +    {
> +      merror ("Overflow for size_t MAX * 2 not detected");
> +      free(ptr);
> +    }
> +  else if (errno != ENOMEM)
> +    merror ("errno is not set correctly");
> +
> +  errno = 0;
> +  ptr = reallocarray (NULL, 2, max);
> +  if (ptr)
> +    {
> +      merror ("Overflow for 2 * size_t MAX not detected");
> +      free(ptr);
> +    }
> +  else if (errno != ENOMEM)
> +    merror ("errno is not set correctly");
> +
> +  a = 65537;
> +  b = max/65537 + 1;
> +  errno = 0;
> +  ptr = reallocarray (NULL, a, b);
> +  if (ptr)
> +    {
> +      merror ("Overflow for (size_t MAX/65537 + 1) * 65537 not detected");
> +      free(ptr);
> +    }
> +  else if (errno != ENOMEM)
> +    merror ("errno is not set correctly");
> +
> +  errno = 0;
> +  ptr = reallocarray (NULL, b, a);
> +  if (ptr)
> +    {
> +      merror ("Overflow for 65537 * (size_t MAX/65537 + 1)  not detected");
> +      free(ptr);
> +    }
> +  else if (errno != ENOMEM)
> +    merror ("errno is not set correctly");
> +
> +  /* Test realloc-like behavior.  */
> +  /* Allocate memory like malloc.  */
> +  ptr = reallocarray(NULL, 10, 2);
> +  if (!ptr)
> +    merror ("realloc(NULL, 10, 2) failed");
> +
> +  memset (ptr, 0xAF, 10*2);
> +
> +  /* Enlarge buffer.   */
> +  ptr2 = reallocarray(ptr, 20, 2);
> +  if (!ptr2)
> +    merror ("realloc(ptr, 20, 2) failed (enlarge)");
> +  else
> +    ptr = ptr2;
> +
> +  c = ptr;
> +  ok = 1;
> +  for (i = 0; i < 10*2; ++i)
> +    {
> +      if (c[i] != 0xAF)
> +        ok = 0;
> +    }
> +  if (!ok)
> +    merror ("Enlarging changed buffer content (10*2)");
> +
> +  /* Decrease buffer size.  */
> +  ptr2 = reallocarray(ptr, 5, 3);
> +  if (!ptr2)
> +    merror ("realloc(ptr, 5, 3) failed (decrease)");
> +  else
> +    ptr = ptr2;
> +
> +  c = ptr;
> +  ok = 1;
> +  for (i = 0; i < 5*3; ++i)
> +    {
> +      if (c[i] != 0xAF)
> +        ok = 0;
> +    }
> +  if (!ok)
> +    merror ("Reducing changed buffer content (5*3)");
> +
> +  /* Overflow should leave buffer untouched.  */
> +  errno = 0;
> +  ptr2 = reallocarray(ptr, 2, ~(size_t)0);
> +  if (ptr2)
> +    merror ("realloc(ptr, 2, size_t MAX) failed to detect overflow");
> +  if (errno != ENOMEM)
> +    merror ("errno not set correctly");
> +
> +  c = ptr;
> +  ok = 1;
> +  for (i = 0; i < 5*3; ++i)
> +    {
> +      if (c[i] != 0xAF)
> +        ok = 0;
> +    }
> +  if (!ok)
> +    merror ("Overflow changed buffer content (5*3)");
> +
> +  /* Free buffer (glibc).  */
> +  errno = 0;
> +  ptr2 = reallocarray (ptr, 0, 0);
> +  if (ptr2)
> +    merror ("reallocarray (ptr, 0, 0) returned non-NULL");
> +
> +  free (ptr2);
> +
> +  return errors != 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index 00329a2..b75c28f 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -479,6 +479,13 @@ extern void *calloc (size_t __nmemb, size_t __size)
>     between objects pointed by the old and new pointers.  */
>  extern void *realloc (void *__ptr, size_t __size)
>       __THROW __attribute_warn_unused_result__;
> +/* Re-allocate the previously allocated block in PTR, making the new
> +   block large enough for NMEMB elements of SIZE bytes each.  */
> +/* __attribute_malloc__ is not used, because if realloc returns
> +   the same pointer that was passed to it, aliasing needs to be allowed
> +   between objects pointed by the old and new pointers.  */
> +extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
> +     __THROW __attribute_warn_unused_result__;
>  /* Free a block allocated by `malloc', `realloc' or `calloc'.  */
>  extern void free (void *__ptr) __THROW;
>  __END_NAMESPACE_STD