Re: [PATCH] Improve execl* functions
Federico Terraneo <[email protected]>
| Newsgroups | gmane.comp.lib.newlib |
|---|---|
| Message-ID | <PAXP251MB0317779CA2949A67244DB482F9E82@PAXP251MB0317.EURP251.PROD.OUTLOOK.COM> |
On 31/01/25 14:28, Richard Earnshaw (lists) wrote: > It can be tricky. Unfortunately, some people don't properly rebuild things after changing the master files. I'd start from a clean repo and try to get to the point where running autoconf is a no-op. Then you'll know that your changes aren't the source of the problems. If you can't then we can dig through the git history to try to identify where things went wrong. > > R. Found it. It's commit commit 48f1655c955c154b3165692e02aa66699212453c (HEAD) Author: Alexey Lapshin <[email protected]> Date: Fri Aug 30 09:35:24 2024 +0000 newlib: xtensa: remove sys/xtensa. use machine/xtensa and the next commit. They remove the libc/sys/xtensa directory but Makefile.in was not committed. If I autoreconf from the previous commit, autoreconf is a no-op. If I autoreconf after that, I get changes in Makefile.in There apparently is nothing wrong in my autotools installation. I thus propose those two patches. The first just updates newlib/Makefile.in so it is consistent with the current newlib directory. The second is the actual patch I was working on.
0001-newlib-update-Makefile.in.patch
(text/x-patch, 132.9 KB) - not displayed
0002-Lift-256-arg-limit-in-execl-execle-and-execlp-add-en.patch
(text/x-patch, 9.7 KB)
From 2f43f7cea30f3b66f17b7a01bd77a3c26b3522a0 Mon Sep 17 00:00:00 2001 From: Terraneo Federico <[email protected]> Date: Tue, 28 Jan 2025 17:19:18 +0100 Subject: [PATCH 2/2] Lift 256 arg limit in execl, execle and execlp, add --enable-newlib-use-malloc-in-execl option The previous version of these functions allocated a 256 entry array and copied arguments in that array with no bound checking. That implementation always occupied 1024 bytes of stack for the array even in the common case in which the number of passed arguments is far less than 256, risking stack overflows in environments with small stacks, and caused a stack buffer overflow if called with more than 256 arguments. The improved implementation counts the actual number of passed arguments and allocates a suitable buffer. The default implementation uses alloca to allocate the buffer to satisfy the POSIX.1-2008 requirement that execl and execle should be callable from signal handlers, but it is possible to override this behavior and use malloc for targets where the risk of stack overflow due to unbounded stack allocations is a more pressing requirement than the corner case of allowing execl calls from signal handlers. --- newlib/README | 8 ++++++++ newlib/configure | 19 +++++++++++++++++++ newlib/configure.ac | 13 +++++++++++++ newlib/libc/posix/execl.c | 33 +++++++++++++++++++++++++++++++-- newlib/libc/posix/execle.c | 33 +++++++++++++++++++++++++++++++-- newlib/libc/posix/execlp.c | 28 ++++++++++++++++++++++++++-- newlib/newlib.hin | 3 +++ 7 files changed, 131 insertions(+), 6 deletions(-) diff --git a/newlib/README b/newlib/README index 69496f695..e652c4a92 100644 --- a/newlib/README +++ b/newlib/README @@ -364,6 +364,14 @@ One feature can be enabled by specifying `--enable-FEATURE=yes' or less conversion accuracy. Enabled by default. +`--enable-newlib-use-malloc-in-execl' + Use malloc instead of alloca in execl, execle and execlp. Using malloc + avoids unbounded stack allocations in those functions, preventing stack + overflows especially for platforms without virtual memory and small stacks. + However, POSIX.1-2008 requires execl and execle to be callable from signal + handlers while malloc isn't. + Disabled by default. + `--enable-multilib' Build many library versions. Enabled by default. diff --git a/newlib/configure b/newlib/configure index bf8d08100..bd5f641b4 100755 --- a/newlib/configure +++ b/newlib/configure @@ -999,6 +999,7 @@ enable_newlib_nano_formatted_io enable_newlib_retargetable_locking enable_newlib_long_time_t enable_newlib_use_gdtoa +enable_newlib_use_malloc_in_execl enable_multilib enable_target_optspace enable_malloc_debugging @@ -1668,6 +1669,7 @@ Optional Features: --enable-newlib-retargetable-locking Allow locking routines to be retargeted at link time --enable-newlib-long-time_t define time_t to long --enable-newlib-use-gdtoa Use gdtoa rather than legacy ldtoa + --enable-newlib-use-malloc-in-execl use malloc instead of alloca in execl, execle and execlp --enable-multilib build many library versions (default) --enable-target-optspace optimize for space --enable-malloc-debugging indicate malloc debugging requested @@ -2594,6 +2596,17 @@ else newlib_use_gdtoa=yes fi +# Check whether --enable-newlib-use-malloc-in-execl was given. +if test "${enable_newlib_use_malloc_in_execl+set}" = set; then : + enableval=$enable_newlib_use_malloc_in_execl; case "${enableval}" in + yes) newlib_use_malloc_in_execl=yes;; + no) newlib_use_malloc_in_execl=no ;; + *) as_fn_error $? "bad value ${enableval} for newlib-use-malloc-in-execl option" "$LINENO" 5 ;; + esac +else + newlib_use_malloc_in_execl=no +fi + # Default to --enable-multilib # Check whether --enable-multilib was given. if test "${enable_multilib+set}" = set; then : @@ -6674,6 +6687,12 @@ $as_echo "#define _WANT_USE_GDTOA 1" >>confdefs.h fi +if test "${newlib_use_malloc_in_execl}" = "yes"; then + +$as_echo "#define _EXECL_USE_MALLOC 1" >>confdefs.h + +fi + if test "x${iconv_encodings}" != "x" \ || test "x${iconv_to_encodings}" != "x" \ diff --git a/newlib/configure.ac b/newlib/configure.ac index 55e5a9446..c6833cfb1 100644 --- a/newlib/configure.ac +++ b/newlib/configure.ac @@ -313,6 +313,15 @@ AC_ARG_ENABLE(newlib-use-gdtoa, esac fi], [newlib_use_gdtoa=yes])dnl +dnl Support --enable-newlib-use-malloc-in-execl +AC_ARG_ENABLE(newlib-use-malloc-in-execl, +[ --enable-newlib-use-malloc-in-execl use malloc instead of alloca in execl, execle and execlp], +[case "${enableval}" in + yes) newlib_use_malloc_in_execl=yes;; + no) newlib_use_malloc_in_execl=no ;; + *) AC_MSG_ERROR(bad value ${enableval} for newlib-use-malloc-in-execl option) ;; + esac], [newlib_use_malloc_in_execl=no])dnl + AM_ENABLE_MULTILIB(, ..) NEWLIB_CONFIGURE(.) @@ -527,6 +536,10 @@ if test "${newlib_use_gdtoa}" = "yes"; then AC_DEFINE(_WANT_USE_GDTOA, 1, [Define if using gdtoa rather than legacy ldtoa.]) fi +if test "${newlib_use_malloc_in_execl}" = "yes"; then + AC_DEFINE(_EXECL_USE_MALLOC, 1, [Define if using malloc for execl, execle and execlp.]) +fi + dnl dnl Parse --enable-newlib-iconv-encodings option argument dnl diff --git a/newlib/libc/posix/execl.c b/newlib/libc/posix/execl.c index c3b4e55bd..7e327db71 100644 --- a/newlib/libc/posix/execl.c +++ b/newlib/libc/posix/execl.c @@ -7,6 +7,9 @@ #include <_ansi.h> #include <unistd.h> +#include <errno.h> +#include <stdlib.h> +#include <alloca.h> /* Only deal with a pointer to environ, to work around subtle bugs with shared libraries and/or small data systems where the user declares his own @@ -24,7 +27,29 @@ execl (const char *path, { int i; va_list args; - const char *argv[256]; + const char **argv; + + i = 1; + va_start (args, arg0); + do + i++; + while (va_arg (args, const char *) != NULL); + va_end (args); + /* POSIX.1-2008 requires execl() to be callable from signal handlers while + malloc() isn't, so we default to using alloca(). However, unbounded stack + allocations is at a high risk of stack overflow and undefined behavior in + environments without virtual memory and small stacks, so we let targets + override the default an use malloc() */ +#ifndef _EXECL_USE_MALLOC + argv = alloca (i * sizeof(const char *)); +#else + argv = malloc (i * sizeof(const char *)); + if (argv == NULL) + { + errno = ENOMEM; + return -1; + } +#endif va_start (args, arg0); argv[0] = arg0; @@ -34,6 +59,10 @@ execl (const char *path, while (argv[i++] != NULL); va_end (args); - return _execve (path, (char * const *) argv, *p_environ); + i = _execve (path, (char * const *) argv, *p_environ); +#ifdef _EXECL_USE_MALLOC + free (argv); +#endif + return i; } #endif /* !_NO_EXECVE */ diff --git a/newlib/libc/posix/execle.c b/newlib/libc/posix/execle.c index 34f0ea373..fd4f42af6 100644 --- a/newlib/libc/posix/execle.c +++ b/newlib/libc/posix/execle.c @@ -7,6 +7,9 @@ #include <_ansi.h> #include <unistd.h> +#include <errno.h> +#include <stdlib.h> +#include <alloca.h> #include <stdarg.h> @@ -20,7 +23,29 @@ execle (const char *path, int i; va_list args; const char * const *envp; - const char *argv[256]; + const char **argv; + + i = 1; + va_start (args, arg0); + do + i++; + while (va_arg (args, const char *) != NULL); + va_end (args); + /* POSIX.1-2008 requires execle() to be callable from signal handlers while + malloc() isn't, so we default to using alloca(). However, unbounded stack + allocations is at a high risk of stack overflow and undefined behavior in + environments without virtual memory and small stacks, so we let targets + override the default an use malloc() */ +#ifndef _EXECL_USE_MALLOC + argv = alloca (i * sizeof(const char *)); +#else + argv = malloc (i * sizeof(const char *)); + if (argv == NULL) + { + errno = ENOMEM; + return -1; + } +#endif va_start (args, arg0); argv[0] = arg0; @@ -31,7 +56,11 @@ execle (const char *path, envp = va_arg (args, const char * const *); va_end (args); - return _execve (path, (char * const *) argv, (char * const *) envp); + i = _execve (path, (char * const *) argv, (char * const *) envp); +#ifdef _EXECL_USE_MALLOC + free (argv); +#endif + return i; } #endif /* !_NO_EXECVE */ diff --git a/newlib/libc/posix/execlp.c b/newlib/libc/posix/execlp.c index b845c88c5..214dc77f8 100644 --- a/newlib/libc/posix/execlp.c +++ b/newlib/libc/posix/execlp.c @@ -7,6 +7,9 @@ #include <_ansi.h> #include <unistd.h> +#include <errno.h> +#include <stdlib.h> +#include <alloca.h> #include <stdarg.h> @@ -19,7 +22,24 @@ execlp (const char *path, { int i; va_list args; - const char *argv[256]; + const char **argv; + + i = 1; + va_start (args, arg0); + do + i++; + while (va_arg (args, const char *) != NULL); + va_end (args); +#ifndef _EXECL_USE_MALLOC + argv = alloca (i * sizeof(const char *)); +#else + argv = malloc (i * sizeof(const char *)); + if (argv == NULL) + { + errno = ENOMEM; + return -1; + } +#endif va_start (args, arg0); argv[0] = arg0; @@ -29,7 +49,11 @@ execlp (const char *path, while (argv[i++] != NULL); va_end (args); - return execvp (path, (char * const *) argv); + i = execvp (path, (char * const *) argv); +#ifdef _EXECL_USE_MALLOC + free (argv); +#endif + return i; } #endif /* !_NO_EXECVE */ diff --git a/newlib/newlib.hin b/newlib/newlib.hin index 831846940..22e323e48 100644 --- a/newlib/newlib.hin +++ b/newlib/newlib.hin @@ -35,6 +35,9 @@ /* EL/IX level */ #undef _ELIX_LEVEL +/* Define if using malloc for execl, execle and execlp. */ +#undef _EXECL_USE_MALLOC + /* Define if fseek functions support seek optimization. */ #undef _FSEEK_OPTIMIZATION -- 2.39.5