Re: [PATCH v6] Add reallocarray function.

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 22/05/2017 17:25, Dennis Wölfing wrote:
> 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.
> 
> This is an updated version of a patch originally submitted by Rüdiger
> Sonderfeld in May 2014.
> See <https://sourceware.org/ml/libc-alpha/2014-05/msg00481.html>.
> 
> Tested on i686 and x86_64.
> 
> 2017-05-22  Dennis Wölfing  <denniswoelfing@gmx.de>
>             Rüdiger Sonderfeld  <ruediger@c-plusplus.de>
> 
> 	* include/stdlib.h (__libc_reallocarray): New declaration.
> 	* malloc/Makefile (routines): Add reallocarray.
> 	(tests): Add tst-reallocarray.c.
> 	* malloc/Versions: Add reallocarray and __libc_reallocarray.
> 	* malloc/malloc-internal.h (check_mul_overflow_size_t): New inline
> 	function.
> 	* malloc/malloc.h (reallocarray): New declaration.
> 	* stdlib/stdlib.h (reallocarray): Likewise.
> 	* malloc/reallocarray.c: New file.
> 	* malloc/tst-reallocarray.c: New test file.
> 	* manual/memory.texi: Document reallocarray.
> 	* sysdeps/unix/sysv/linux/aarch64/libc.abilist: Add reallocarray.
> 	* sysdeps/unix/sysv/linux/alpha/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/arm/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/hppa/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/i386/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/ia64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/microblaze/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/nios2/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist:
> 	Likewise.
> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist:
> 	Likewise.
> 	* sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/sh/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/tile/tilegx/tilegx64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/tilepro/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/x86_64/64/libc.abilist: Likewise.
> 	* sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist: Likewise.

LGTM with some nits below.

> diff --git a/NEWS b/NEWS
> index b4ecd6201d..e70bff01db 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -66,6 +66,8 @@ Version 2.26
>  * The port to Native Client running on ARMv7-A (--host=arm-nacl) has been
>    removed.
>  
> +* The reallocarray function has been added to libc.
> +
>  Security related changes:

I would extend it a bit by describing what reallocarray is intended to
(maybe something as 'It is a realloc replacement with a check for integer
overflow when calculating total allocation size').

> diff --git a/malloc/tst-reallocarray.c b/malloc/tst-reallocarray.c
> new file mode 100644
> index 0000000000..e914e2938b
> --- /dev/null
> +++ b/malloc/tst-reallocarray.c
> @@ -0,0 +1,119 @@
> +/* Test for reallocarray.
> +   Copyright (C) 2014-2017 Free Software Foundation, Inc.

It is a new file, so use 2017 instead.

> +   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 <string.h>
> +#include <support/check.h>
> +
> +static int
> +do_test (void)
> +

Extra line.

> diff --git a/manual/memory.texi b/manual/memory.texi
> index a256ca07b2..fb6b594ef1 100644
> --- a/manual/memory.texi
> +++ b/manual/memory.texi
> @@ -751,8 +751,8 @@ be a buffer that you use to hold a line being read from a file; no matter
>  how long you make the buffer initially, you may encounter a line that is
>  longer.
>  
> -You can make the block longer by calling @code{realloc}.  This function
> -is declared in @file{stdlib.h}.
> +You can make the block longer by calling @code{realloc} or
> +@code{reallocarray}.  These functions are declared in @file{stdlib.h}.
>  @pindex stdlib.h
>  
>  @comment malloc.h stdlib.h
> @@ -816,9 +816,29 @@ behavior, and will probably crash when @code{realloc} is passed a null
>  pointer.
>  @end deftypefun
>  
> -Like @code{malloc}, @code{realloc} may return a null pointer if no
> -memory space is available to make the block bigger.  When this happens,
> -the original block is untouched; it has not been modified or relocated.
> +@comment malloc.h stdlib.h
> +@comment BSD
> +@deftypefun {void *} reallocarray (void *@var{ptr}, size_t @var{nmemb}, size_t @var{size})
> +@safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@aculock{} @acsfd{} @acsmem{}}}
> +
> +The @code{reallocarray} function changes the size of the block whose address
> +is @var{ptr} to be long enough to contain a vector of @var{nmemb} elements,
> +each of size @var{size}.  It is equivalent to @samp{realloc (@var{ptr},
> +@var{nmemb} * @var{size})}, except that @code{reallocarray} fails safely if
> +the multiplication overflows, by setting @code{errno} to @code{ENOMEM},
> +returning a null pointer, and leaving the original block unchanged.
> +
> +@code{reallocarray} should be used instead of @code{realloc} when the new size
> +of the allocated block is the result of a multiplication that might overflow.
> +
> +@strong{Portability Note:} This function is not part of any standard.  It was
> +first introduced in OpenBSD 5.6.
> +@end deftypefun

I think it is worth to add FreeBSD 11.0 also supports it.