Re: [PATCH] Improve execl* functions
Federico Terraneo <[email protected]>
| Newsgroups | gmane.comp.lib.newlib |
|---|---|
| Message-ID | <PAXP251MB031771250A67B7568978E569F9E82@PAXP251MB0317.EURP251.PROD.OUTLOOK.COM> |
On 29/01/25 12:26, Corinna Vinschen wrote: > Your patch looks good. At first I thought it's missing the ARG_MAX > check now, but yeah, in fact that should be tested in execve. > > Can you please add the rational for this change to the commit message? > Code comments like R. Diez suggested, would be helpful as well. > > Also, given this adds a compile time option, it would be helpful to > add a matching option to configure.ac and add it to the README file. > Hi, I tried adding the compile-time option and re-running autoreconf, but I'm hitting an issue. Despite using GNU Autoconf 2.69 and GNU Automake 1.15.1 as stated in the newlib/HOWTO file, running autoreconf in the newlib directory *before* making any changes to configure.ac generates tons of spurious changes to Makefile.in, see attached patch 0002. Any idea what I could be doing wrong, I'm on a Debian 12. In any case, patch 0001 and 0003 look ok, maybe it's possible to just skip patch 0002? Even though I'd be curious to know what's the issue with autoreconf, maybe some of you encountered it already? Best regards, Federico Terraneo
0001-Lift-256-arg-limit-in-execl-execle-and-execlp.patch
(text/x-patch, 5.5 KB)
From 9f418b2fbe8aefd0f43321317f74aa6b010d80b2 Mon Sep 17 00:00:00 2001 From: Terraneo Federico <[email protected]> Date: Tue, 28 Jan 2025 17:19:18 +0100 Subject: [PATCH 1/3] Lift 256 arg limit in execl, execle and execlp 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/libc/posix/execl.c | 33 +++++++++++++++++++++++++++++++-- newlib/libc/posix/execle.c | 33 +++++++++++++++++++++++++++++++-- newlib/libc/posix/execlp.c | 28 ++++++++++++++++++++++++++-- 3 files changed, 88 insertions(+), 6 deletions(-) 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 */ -- 2.39.5
0002-Despite-using-autoconf-2.69-and-automake-1.15.1-auto.patch
(text/x-patch, 132.8 KB) - not displayed
0003-Add-enable-newlib-use-malloc-in-execl-option-to-conf.patch
(text/x-patch, 4.4 KB)
From c18766c903eba2bbf293a858b66aeb91eb436a2e Mon Sep 17 00:00:00 2001 From: Terraneo Federico <[email protected]> Date: Fri, 31 Jan 2025 10:42:09 +0100 Subject: [PATCH 3/3] Add --enable-newlib-use-malloc-in-execl option to configure --- newlib/README | 8 ++++++++ newlib/configure | 19 +++++++++++++++++++ newlib/configure.ac | 13 +++++++++++++ newlib/newlib.hin | 3 +++ 4 files changed, 43 insertions(+) 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/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