* [RFC][PATCH] Add reallocarray function. @ 2014-05-18 21:05 Rüdiger Sonderfeld 2014-05-18 21:43 ` Paul Eggert 2014-09-01 15:48 ` [RFC][PATCH] " Florian Weimer 0 siblings, 2 replies; 19+ messages in thread From: Rüdiger Sonderfeld @ 2014-05-18 21:05 UTC (permalink / raw) To: libc-alpha 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 I think this would be a useful addition to the glibc as well. The patch currently contains no documentation. I will add it to the patch if there is interest in including the function. I have signed the FSF papers. The code reuses the integer overflow check from __libc_calloc (that's why it is using __builtin_expect instead of the recommended __glibc_unlikely) and on success simply calls __libc_realloc. Please CC me in replies because I'm not subscribed to libc-alpha. --- ChangeLog | 9 +++ malloc/Makefile | 2 +- malloc/Versions | 4 ++ malloc/malloc.c | 22 +++++++ malloc/malloc.h | 8 +++ malloc/tst-reallocarray.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++ stdlib/stdlib.h | 7 ++ 7 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 malloc/tst-reallocarray.c diff --git a/ChangeLog b/ChangeLog index c606b0d..d325cc4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +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 (__libc_reallocarray): New function. + 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..e86e10e 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -2944,6 +2944,26 @@ void *weak_variable (*__memalign_hook) libc_hidden_def (__libc_free) void * +__libc_reallocarray(void *optr, size_t nmemb, size_t elem_size) +{ + /* Overflow handling from __libc_calloc: */ + + /* size_t is unsigned so the behavior on overflow is defined. */ + INTERNAL_SIZE_T bytes = nmemb * elem_size; +#define HALF_INTERNAL_SIZE_T \ + (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2)) + if (__builtin_expect ((nmemb | elem_size) >= HALF_INTERNAL_SIZE_T, 0)) + { + if (elem_size != 0 && bytes / elem_size != nmemb) + { + __set_errno (ENOMEM); + return 0; + } + } + return __libc_realloc (optr, bytes); +} + +void * __libc_realloc (void *oldmem, size_t bytes) { mstate ar_ptr; @@ -5171,6 +5191,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..608ad64 --- /dev/null +++ b/malloc/tst-reallocarray.c @@ -0,0 +1,161 @@ +/* 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; + return 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 -- 1.9.3 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH] Add reallocarray function. 2014-05-18 21:05 [RFC][PATCH] Add reallocarray function Rüdiger Sonderfeld @ 2014-05-18 21:43 ` Paul Eggert 2014-05-18 21:50 ` [RFC][PATCH v2] " Rüdiger Sonderfeld 2014-09-01 15:48 ` [RFC][PATCH] " Florian Weimer 1 sibling, 1 reply; 19+ messages in thread From: Paul Eggert @ 2014-05-18 21:43 UTC (permalink / raw) To: Rüdiger Sonderfeld, libc-alpha Rüdiger Sonderfeld wrote: > + /* size_t is unsigned so the behavior on overflow is defined. */ > + INTERNAL_SIZE_T bytes = nmemb * elem_size; > +#define HALF_INTERNAL_SIZE_T \ > + (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2)) > + if (__builtin_expect ((nmemb | elem_size) >= HALF_INTERNAL_SIZE_T, 0)) > + { > + if (elem_size != 0 && bytes / elem_size != nmemb) 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC][PATCH v2] Add reallocarray function. 2014-05-18 21:43 ` Paul Eggert @ 2014-05-18 21:50 ` Rüdiger Sonderfeld 2014-05-19 4:27 ` Rüdiger Sonderfeld 2014-05-19 15:30 ` Joseph S. Myers 0 siblings, 2 replies; 19+ messages in thread From: Rüdiger Sonderfeld @ 2014-05-18 21:50 UTC (permalink / raw) To: Paul Eggert; +Cc: libc-alpha 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 -- 1.9.3 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-18 21:50 ` [RFC][PATCH v2] " Rüdiger Sonderfeld @ 2014-05-19 4:27 ` Rüdiger Sonderfeld 2014-05-19 15:30 ` Joseph S. Myers 1 sibling, 0 replies; 19+ messages in thread From: Rüdiger Sonderfeld @ 2014-05-19 4:27 UTC (permalink / raw) To: Paul Eggert; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-18 21:50 ` [RFC][PATCH v2] " Rüdiger Sonderfeld 2014-05-19 4:27 ` Rüdiger Sonderfeld @ 2014-05-19 15:30 ` Joseph S. Myers 2014-05-20 4:35 ` Rich Felker 2014-05-20 12:50 ` Rüdiger Sonderfeld 1 sibling, 2 replies; 19+ messages in thread From: Joseph S. Myers @ 2014-05-19 15:30 UTC (permalink / raw) To: Rüdiger Sonderfeld; +Cc: Paul Eggert, libc-alpha [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1206 bytes --] On Sun, 18 May 2014, Rüdiger Sonderfeld wrote: > 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__; How has this patch been tested? This function declaration needs conditioning on __USE_GNU, as no standard includes this function in <stdlib.h>; I'd have expected you to get failures of the conform/ tests for stdlib.h when you ran the testsuite. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-19 15:30 ` Joseph S. Myers @ 2014-05-20 4:35 ` Rich Felker 2014-05-20 8:17 ` Paul Eggert 2014-05-20 8:19 ` Andreas Schwab 2014-05-20 12:50 ` Rüdiger Sonderfeld 1 sibling, 2 replies; 19+ messages in thread From: Rich Felker @ 2014-05-20 4:35 UTC (permalink / raw) To: Joseph S. Myers; +Cc: Rüdiger Sonderfeld, Paul Eggert, libc-alpha On Mon, May 19, 2014 at 03:02:52PM +0000, Joseph S. Myers wrote: > On Sun, 18 May 2014, R?diger Sonderfeld wrote: > > > 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__; > > How has this patch been tested? This function declaration needs > conditioning on __USE_GNU, as no standard includes this function in > <stdlib.h>; I'd have expected you to get failures of the conform/ tests > for stdlib.h when you ran the testsuite. Not really related to this patch, but I think the comment about __attribute_malloc__ that was copied from realloc is wrong. The new and old pointers cannot alias because, if realloc succeeds (and I would expect reallocarray to behave the same) the _value_ of the old pointer is indeterminate and accessing it (e.g. comparing it with new) invokes undefined behavior. Rich ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 4:35 ` Rich Felker @ 2014-05-20 8:17 ` Paul Eggert 2014-05-20 8:19 ` Andreas Schwab 1 sibling, 0 replies; 19+ messages in thread From: Paul Eggert @ 2014-05-20 8:17 UTC (permalink / raw) To: Rich Felker, Joseph S. Myers; +Cc: Rüdiger Sonderfeld, libc-alpha Rich Felker wrote: > Not really related to this patch, but I think the comment about > __attribute_malloc__ that was copied from realloc is wrong. The new > and old pointers cannot alias You're correct that the new and old pointers cannot alias, but I fear that __attribute_malloc__ does not mean what you think it means. The GCC documentation is *really* confusing in this area, unfortunately. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56955 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 4:35 ` Rich Felker 2014-05-20 8:17 ` Paul Eggert @ 2014-05-20 8:19 ` Andreas Schwab 2014-05-20 15:45 ` Paul Eggert 1 sibling, 1 reply; 19+ messages in thread From: Andreas Schwab @ 2014-05-20 8:19 UTC (permalink / raw) To: Rich Felker Cc: Joseph S. Myers, Rüdiger Sonderfeld, Paul Eggert, libc-alpha Rich Felker <dalias@libc.org> writes: > Not really related to this patch, but I think the comment about > __attribute_malloc__ that was copied from realloc is wrong. The new > and old pointers cannot alias because, if realloc succeeds (and I > would expect reallocarray to behave the same) the _value_ of the old > pointer is indeterminate and accessing it (e.g. comparing it with new) > invokes undefined behavior. Please report that as a gcc bug (it doesn't mark it either). Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 8:19 ` Andreas Schwab @ 2014-05-20 15:45 ` Paul Eggert 2014-05-20 20:47 ` Rich Felker 0 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2014-05-20 15:45 UTC (permalink / raw) To: Andreas Schwab; +Cc: libc-alpha Andreas Schwab wrote: > Please report that as a gcc bug (it doesn't mark it either). As far as I can tell this is a bug in GCC's documentation, not in its implementation. As I mentioned cryptically in my previous message, if FOO has the malloc attribute and you execute 'int **p = foo (...);', the malloc attribute means that *P cannot point to previously-existing storage (because *P is uninitialized junk). That is why the attribute applies to calloc (because *P must be null, which also cannot point to previously-existing storage) but not to realloc (because *P might be initialized and might point to previously-existing storage). For more about this confusing topic, please see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56955 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 15:45 ` Paul Eggert @ 2014-05-20 20:47 ` Rich Felker 2014-05-20 20:56 ` Paul Eggert 0 siblings, 1 reply; 19+ messages in thread From: Rich Felker @ 2014-05-20 20:47 UTC (permalink / raw) To: Paul Eggert; +Cc: Andreas Schwab, libc-alpha On Tue, May 20, 2014 at 08:44:09AM -0700, Paul Eggert wrote: > Andreas Schwab wrote: > >Please report that as a gcc bug (it doesn't mark it either). > > As far as I can tell this is a bug in GCC's documentation, not in > its implementation. As I mentioned cryptically in my previous > message, if FOO has the malloc attribute and you execute 'int **p = > foo (...);', the malloc attribute means that *P cannot point to > previously-existing storage (because *P is uninitialized junk). > That is why the attribute applies to calloc (because *P must be > null, which also cannot point to previously-existing storage) but > not to realloc (because *P might be initialized and might point to > previously-existing storage). > > For more about this confusing topic, please see: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56955 What is the issue about "previously-existing storage"? Is the concern that *new->something might alias new (because new==old happens to be true and the object contained a pointer to itself or part of itself) or that *new->something merely can alias other existing memory (whereas GCC assumes it never can)? Rich ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 20:47 ` Rich Felker @ 2014-05-20 20:56 ` Paul Eggert 0 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2014-05-20 20:56 UTC (permalink / raw) To: Rich Felker; +Cc: libc-alpha On 05/20/2014 10:01 AM, Rich Felker wrote: > Is the concern > that *new->something might alias new (because new==old happens to be > true and the object contained a pointer to itself or part of itself) > or that *new->something merely can alias other existing memory > (whereas GCC assumes it never can)? The latter, if I understand your question correctly. I added a proposed patch to the GCC documentation here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56955#c6 Please give it a read, and follow up there if you have further questions. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-19 15:30 ` Joseph S. Myers 2014-05-20 4:35 ` Rich Felker @ 2014-05-20 12:50 ` Rüdiger Sonderfeld 2014-05-20 14:18 ` Paul Eggert 1 sibling, 1 reply; 19+ messages in thread From: Rüdiger Sonderfeld @ 2014-05-20 12:50 UTC (permalink / raw) To: Joseph S. Myers; +Cc: Paul Eggert, libc-alpha On Monday 19 May 2014 15:02:52 Joseph S. Myers wrote: > How has this patch been tested? This function declaration needs > conditioning on __USE_GNU, as no standard includes this function in > <stdlib.h>; I'd have expected you to get failures of the conform/ tests > for stdlib.h when you ran the testsuite. Yes, I got failures in the conform tests. (I also hit some nptl errors but I guess they are unrelated). I've added the __USE_GNU check to stdlib.h. I assume this is not necessary for malloc.h since malloc.h is not a standard header? I'll send and updated patch to the mailing list. But I wonder if there is any general interest or objections to adding the `reallocarray' function to glibc? Regards, Rüdiger ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 12:50 ` Rüdiger Sonderfeld @ 2014-05-20 14:18 ` Paul Eggert 2014-05-21 12:39 ` Rüdiger Sonderfeld 0 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2014-05-20 14:18 UTC (permalink / raw) To: Rüdiger Sonderfeld; +Cc: libc-alpha Rüdiger Sonderfeld wrote: > I wonder if there is any general interest or objections to adding the > `reallocarray' function to glibc? As an application developer I'm mildly interested if you can speed it up. GNU apps like 'ls', 'grep', etc. that need such a function typically use Gnulib's xnrealloc, which has the same signature and nearly the same behavior as reallocarray. xnrealloc and reallocarray's behaviors differ in two subtle ways, though. First, xnrealloc's 2nd arg must be nonzero; this avoids a runtime check for zero so it's a bit faster (xnrealloc is an inline function so much of the overflow test's work can typically be done at compile-time). Second, xnrealloc's result is guaranteed to be non-NULL if its first arg is nonzero; this lets the calling code be simpler. We could alter xnrealloc to be a simple wrapper for reallocarray if reallocarray is available. With the current reallocarray implementation, though, this might not be worth the trouble, because I suspect xnrealloc's test for overflow is faster than reallocarray's in the typical case where the last argument is a constant. If you could fix reallocarray to use the equivalent of __builtin_umul_overflow, though, this objection would be removed and it would make sense for xnrealloc to be a wrapper for reallocarray if available. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-20 14:18 ` Paul Eggert @ 2014-05-21 12:39 ` Rüdiger Sonderfeld 2014-05-22 5:45 ` Paul Eggert 0 siblings, 1 reply; 19+ messages in thread From: Rüdiger Sonderfeld @ 2014-05-21 12:39 UTC (permalink / raw) To: Paul Eggert; +Cc: libc-alpha On Tuesday 20 May 2014 07:06:13 Paul Eggert wrote: > First, xnrealloc's 2nd arg > must be nonzero; this avoids a runtime check for zero so it's a bit > faster (xnrealloc is an inline function so much of the overflow test's > work can typically be done at compile-time). An inline version certainly would make sense. But I don't think this could be done for glibc. What happens if the 2nd arg is zero in a call to `xnrealloc'? Since `reallocarray' should primarily be about providing a safer to use API I wouldn't want to introduce another case for undefined behavior. And I'd like to stay compatible as much as possible with the OpenBSD implementation (which will probably spread to other *BSDs as well). > Second, xnrealloc's result > is guaranteed to be non-NULL if its first arg is nonzero; this lets the > calling code be simpler. How can `xnrealloc' guarantee that the result will be non-NULL? > We could alter xnrealloc to be a simple wrapper for reallocarray if > reallocarray is available. With the current reallocarray > implementation, though, this might not be worth the trouble, because I > suspect xnrealloc's test for overflow is faster than reallocarray's in > the typical case where the last argument is a constant. If you could > fix reallocarray to use the equivalent of __builtin_umul_overflow, > though, this objection would be removed and it would make sense for > xnrealloc to be a wrapper for reallocarray if available. I've changed `reallocarray' (and `calloc') to call `check_mul_overflow' for the overflow check (v2 of the patch). This function could be replaced by a call to `__builtin_umul_overflow' once GCC provides such an extension. (However there would the problem of selecting the right overflow check due to the existence of `size_t` and `INTERNAL_SIZE_T'. But that can be discussed once the GCC extension exists.) Is there active work done on those overflow checks in GCC? Maybe it would make sense to provide an API like that, which if available uses the builtin. I'm not sure if glibc would be the right place for it though. But I guess this might be something gnulib could provide (if it hasn't something like that already). Regards, Rüdiger ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] Add reallocarray function. 2014-05-21 12:39 ` Rüdiger Sonderfeld @ 2014-05-22 5:45 ` Paul Eggert 0 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2014-05-22 5:45 UTC (permalink / raw) To: Rüdiger Sonderfeld; +Cc: libc-alpha Rüdiger Sonderfeld wrote: > What happens if the 2nd arg is zero in a call to `xnrealloc'? Undefined behavior. Agreed, this isn't suitable for reallocarray. > How can `xnrealloc' guarantee that the result will be non-NULL? It tests realloc's result and calls a _Noreturn function if it's null. Again, this is fine for Gnulib but not suitable for reallocarray -- we'd keep this part in the Gnulib wrapper. > Is there active work done on those overflow checks in GCC? The last message I know on the topic was last month. Sort of active, I guess. <https://gcc.gnu.org/ml/gcc/2014-04/msg00194.html> Perhaps someone should ping the GCC list. While we're on the subject, there was a proposal last October to add saturated arithmetic to glibc internals; see, e.g., <https://www.sourceware.org/ml/libc-alpha/2013-10/msg00905.html>. Presumably this arithmetic could use these builtins too. > this might be something gnulib could provide (if it hasn't something like that already). gnulib has several things like that already, which could be tuned to use __builtin_umul_overflow if available: * intprops.h's INT_MULTIPLY_OVERFLOW (A, B) returns true if multiplying the integers A and B would overflow. It works for any combination of integer types, so in this sense it's handier than __builtin_umul_overflow. * xsize.h's xtimes uses saturated arithmetic to multiply size_t values, and needs to check for overflow. * xalloc-oversized.h's xalloc_oversized checks for overflow in size_t multiplication, with the special case that SIZE_MAX represents a previously-overflowed computation (e.g., from xtimes). Perhaps we could add an MUL_OVERFLOW (A, B, PRESULT) macro to intprops.h (that uses __builtin_umul_overflow/__builtin_ulmul_overflow/etc.) if available) so that applications could use the builtins more directly if they prefer. It's not clear how useful that would be, though. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH] Add reallocarray function. 2014-05-18 21:05 [RFC][PATCH] Add reallocarray function Rüdiger Sonderfeld 2014-05-18 21:43 ` Paul Eggert @ 2014-09-01 15:48 ` Florian Weimer 2014-09-01 17:24 ` Rich Felker 1 sibling, 1 reply; 19+ messages in thread From: Florian Weimer @ 2014-09-01 15:48 UTC (permalink / raw) To: Rüdiger Sonderfeld, libc-alpha On 05/18/2014 10:32 PM, Rüdiger Sonderfeld wrote: > +/* 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__; I'm not sure if this is still on the table, but experience shows that the realloc interface is error-prone for another reason: The straight way to write an a reallocation, ptr = realloc(ptr, new_size); leads to a memory leak on error. It would be less error-prone to have reallocarray to update the pointer directly on success, e.g.: if (reallocarray(&ptr, new_count, sizeof(T)) < 0) { // handle error } However, this cannot be implemented as a C function, only as a macro. -- Florian Weimer / Red Hat Product Security ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH] Add reallocarray function. 2014-09-01 15:48 ` [RFC][PATCH] " Florian Weimer @ 2014-09-01 17:24 ` Rich Felker 2014-09-02 9:29 ` Florian Weimer 0 siblings, 1 reply; 19+ messages in thread From: Rich Felker @ 2014-09-01 17:24 UTC (permalink / raw) To: Florian Weimer; +Cc: Rüdiger Sonderfeld, libc-alpha On Mon, Sep 01, 2014 at 05:48:38PM +0200, Florian Weimer wrote: > On 05/18/2014 10:32 PM, Rüdiger Sonderfeld wrote: > >+/* 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__; > > I'm not sure if this is still on the table, but experience shows > that the realloc interface is error-prone for another reason: The > straight way to write an a reallocation, > > ptr = realloc(ptr, new_size); > > leads to a memory leak on error. It would be less error-prone to > have reallocarray to update the pointer directly on success, e.g.: > > if (reallocarray(&ptr, new_count, sizeof(T)) < 0) { > // handle error > } No, allocation functions which take void** are an extremely bad idiom because they encourage UB. It's rare that ptr actually has type void* (usually it has type T* for whatever type T the array members have). So to use an allocation function that takes void**, a correct program must use a temporary pointer, as in: void *tmp = ptr; if (reallocarray(&tmp, new_count, sizeof(T)) < 0) { ... } ptr = tmp; Note that this is even more awkward than the current situation with realloc. However, almost everyone will actually write: if (reallocarray((void **)&ptr, new_countl sizeof(T)) < 0) { ... } thereby invoking UB via an aliasing violation. Also, reallocarray is already defined by OpenBSD and perhaps others with a particular signature. Defining an incompatible function with the same name is just asking for trouble. In particular, if any program using the proposed GNU variant with your semantics, but links to a library which provides its own replacement function with the BSD semantics, the symbols will conflict and the wrong function will get called. > However, this cannot be implemented as a C function, only as a macro. Perhaps you meant to solve the aliasing violation a macro? It's possible with GNU C statement-expressions, but ugly, and not really clean. Perhaps there's a way to do it without any nonstandard extensions too, but I don't think this kind of ugly hack that LOOKS LIKE an aliasing violation until you decipher the macro belongs in glibc. Rich ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH] Add reallocarray function. 2014-09-01 17:24 ` Rich Felker @ 2014-09-02 9:29 ` Florian Weimer 2014-09-02 13:03 ` Rich Felker 0 siblings, 1 reply; 19+ messages in thread From: Florian Weimer @ 2014-09-02 9:29 UTC (permalink / raw) To: Rich Felker; +Cc: Rüdiger Sonderfeld, libc-alpha On 09/01/2014 07:24 PM, Rich Felker wrote: >> I'm not sure if this is still on the table, but experience shows >> that the realloc interface is error-prone for another reason: The >> straight way to write an a reallocation, >> >> ptr = realloc(ptr, new_size); >> >> leads to a memory leak on error. It would be less error-prone to >> have reallocarray to update the pointer directly on success, e.g.: >> >> if (reallocarray(&ptr, new_count, sizeof(T)) < 0) { >> // handle error >> } > > No, allocation functions which take void** are an extremely bad idiom > because they encourage UB. That's why I wrote that reallocarray has to be a macro. On the other hand, I'm not particularly worried about the aliasing violation because according to one reading of the standard, realloc returns a pointer to untyped (but partially initialized) memory, which needs out-of-language support anyway. > Also, reallocarray is already defined by OpenBSD and perhaps others > with a particular signature. We'd have to give the fixed version a separate name, to avoid another strerror_r-like fiasco. -- Florian Weimer / Red Hat Product Security ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH] Add reallocarray function. 2014-09-02 9:29 ` Florian Weimer @ 2014-09-02 13:03 ` Rich Felker 0 siblings, 0 replies; 19+ messages in thread From: Rich Felker @ 2014-09-02 13:03 UTC (permalink / raw) To: Florian Weimer; +Cc: Rüdiger Sonderfeld, libc-alpha On Tue, Sep 02, 2014 at 11:16:05AM +0200, Florian Weimer wrote: > On 09/01/2014 07:24 PM, Rich Felker wrote: > > >>I'm not sure if this is still on the table, but experience shows > >>that the realloc interface is error-prone for another reason: The > >>straight way to write an a reallocation, > >> > >> ptr = realloc(ptr, new_size); > >> > >>leads to a memory leak on error. It would be less error-prone to > >>have reallocarray to update the pointer directly on success, e.g.: > >> > >> if (reallocarray(&ptr, new_count, sizeof(T)) < 0) { > >> // handle error > >> } > > > >No, allocation functions which take void** are an extremely bad idiom > >because they encourage UB. > > That's why I wrote that reallocarray has to be a macro. > > On the other hand, I'm not particularly worried about the aliasing > violation because according to one reading of the standard, realloc > returns a pointer to untyped (but partially initialized) memory, > which needs out-of-language support anyway. This is a non-sequitur. Even if "realloc needs out-of-language support" (a topic I don't want to mix with this discussion), that would not be a reason to preclude unrelated compiler optimization. The fact that T* and void* cannot alias is unrelated to anything about the contents of the member realloc returns. > >Also, reallocarray is already defined by OpenBSD and perhaps others > >with a particular signature. > > We'd have to give the fixed version a separate name, to avoid > another strerror_r-like fiasco. I think it's much better just to avoid it. Like I said the "fix" actually makes a worse interface that's clunkier to use. And if the caller is just going to abort when reallocarray fails (this is very bad practice, but it's common GNU practice) then there's no point in using a temporary anyway. Direct assignment works fine. Rich ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-09-02 13:03 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-18 21:05 [RFC][PATCH] Add reallocarray function Rüdiger Sonderfeld 2014-05-18 21:43 ` Paul Eggert 2014-05-18 21:50 ` [RFC][PATCH v2] " Rüdiger Sonderfeld 2014-05-19 4:27 ` Rüdiger Sonderfeld 2014-05-19 15:30 ` Joseph S. Myers 2014-05-20 4:35 ` Rich Felker 2014-05-20 8:17 ` Paul Eggert 2014-05-20 8:19 ` Andreas Schwab 2014-05-20 15:45 ` Paul Eggert 2014-05-20 20:47 ` Rich Felker 2014-05-20 20:56 ` Paul Eggert 2014-05-20 12:50 ` Rüdiger Sonderfeld 2014-05-20 14:18 ` Paul Eggert 2014-05-21 12:39 ` Rüdiger Sonderfeld 2014-05-22 5:45 ` Paul Eggert 2014-09-01 15:48 ` [RFC][PATCH] " Florian Weimer 2014-09-01 17:24 ` Rich Felker 2014-09-02 9:29 ` Florian Weimer 2014-09-02 13:03 ` Rich Felker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).