Improve execl* functions

Message ID PAXP251MB03177947C164F6FEF2BB97F9F9ED2@PAXP251MB0317.EURP251.PROD.OUTLOOK.COM
State New
Headers
Series Improve execl* functions |

Commit Message

Federico Terraneo Jan. 26, 2025, 11:14 a.m. UTC
  Hi,
I propose the following patch to the execl* functions in newlib that 
does the following:
- it returns an error if more than the maximum number of arguments are 
passed instead of corrupting the stack
- it allows to tweak the maximum number of arguments at the time of 
compiling newlib by defining the ARG_NUM_MAX macro

This patch is originally meant for the Miosix OS to allow reducing the 
maximum number of arguments with the intent of reducing the stack usage 
of the execl* functions in low memory microcontrollers, but I think it 
is of general applicability and can of course be useful also to increase 
this value for desktop-class targets.

Best regards,
Federico Terraneo
  

Comments

Torbjorn SVENSSON Jan. 27, 2025, 10:35 a.m. UTC | #1
Hi Federico,

I'm in no possition to either accept or reject this patch, but here is 
what I've seen when revieing you patch.

On 2025-01-26 12:14, Federico Terraneo wrote:
> Hi,
> I propose the following patch to the execl* functions in newlib that 
> does the following:
> - it returns an error if more than the maximum number of arguments are 
> passed instead of corrupting the stack
> - it allows to tweak the maximum number of arguments at the time of 
> compiling newlib by defining the ARG_NUM_MAX macro
> 
> This patch is originally meant for the Miosix OS to allow reducing the 
> maximum number of arguments with the intent of reducing the stack usage 
> of the execl* functions in low memory microcontrollers, but I think it 
> is of general applicability and can of course be useful also to increase 
> this value for desktop-class targets.
> 
> Best regards,
> Federico Terraneo
> 
> 
> 
> 0001-Fix-stack-overflow-in-exec-add-ARG_NUM_MAX.patch
> 
>  From 9d06b54b1f2a2957bc40af10c6dc09867662325f Mon Sep 17 00:00:00 2001
> From: Terraneo Federico <fede.tft@miosix.org>
> Date: Sat, 25 Jan 2025 11:51:15 +0100
> Subject: [PATCH] Fix stack overflow in exec*; add ARG_NUM_MAX
> 
> ---
>   newlib/libc/posix/execl.c  | 19 +++++++++++++++----
>   newlib/libc/posix/execle.c | 17 ++++++++++++++---
>   newlib/libc/posix/execlp.c | 19 +++++++++++++++----
>   3 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/newlib/libc/posix/execl.c b/newlib/libc/posix/execl.c
> index c3b4e55bd..044205cc6 100644
> --- a/newlib/libc/posix/execl.c
> +++ b/newlib/libc/posix/execl.c
> @@ -7,6 +7,7 @@
>   
>   #include <_ansi.h>
>   #include <unistd.h>
> +#include <errno.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
> @@ -16,6 +17,10 @@ static char ***p_environ = &environ;
>   
>   #include <stdarg.h>
>   
> +#ifndef ARG_NUM_MAX
> +#define ARG_NUM_MAX 256
> +#endif
> +
>   int
>   execl (const char *path,
>         const char *arg0, ...)
> @@ -24,14 +29,20 @@ execl (const char *path,
>   {
>     int i;
>     va_list args;
> -  const char *argv[256];
> +  const char *argv[ARG_NUM_MAX];
>   
>     va_start (args, arg0);
>     argv[0] = arg0;
>     i = 1;
> -  do
> -      argv[i] = va_arg (args, const char *);
> -  while (argv[i++] != NULL);
> +  do {
> +    if(i>=ARG_NUM_MAX)

if (i >= ARG_NUM_MAX)

(Add the missing spaces).

> +    {
> +      va_end (args);
> +      errno=E2BIG;

errno = E2BIG;

(Add the missing spaces).

Same applies to the other files in the patch.

Kind regards,
Torbjörn

> +      return -1;
> +    }
> +    argv[i] = va_arg (args, const char *);
> +  } while (argv[i++] != NULL);
>     va_end (args);
>   
>     return _execve (path, (char * const  *) argv, *p_environ);
> diff --git a/newlib/libc/posix/execle.c b/newlib/libc/posix/execle.c
> index 34f0ea373..7a25ae8ef 100644
> --- a/newlib/libc/posix/execle.c
> +++ b/newlib/libc/posix/execle.c
> @@ -7,10 +7,15 @@
>   
>   #include <_ansi.h>
>   #include <unistd.h>
> +#include <errno.h>
>   
>   
>   #include <stdarg.h>
>   
> +#ifndef ARG_NUM_MAX
> +#define ARG_NUM_MAX 256
> +#endif
> +
>   int
>   execle (const char *path,
>         const char *arg0, ...)
> @@ -20,14 +25,20 @@ execle (const char *path,
>     int i;
>     va_list args;
>     const char * const *envp;
> -  const char *argv[256];
> +  const char *argv[ARG_NUM_MAX];
>   
>     va_start (args, arg0);
>     argv[0] = arg0;
>     i = 1;
> -  do
> +  do {
> +    if(i>=ARG_NUM_MAX)
> +    {
> +      va_end (args);
> +      errno=E2BIG;
> +      return -1;
> +    }
>       argv[i] = va_arg (args, const char *);
> -  while (argv[i++] != NULL);
> +  } while (argv[i++] != NULL);
>     envp = va_arg (args, const char * const *);
>     va_end (args);
>   
> diff --git a/newlib/libc/posix/execlp.c b/newlib/libc/posix/execlp.c
> index b845c88c5..a437a56f0 100644
> --- a/newlib/libc/posix/execlp.c
> +++ b/newlib/libc/posix/execlp.c
> @@ -7,10 +7,15 @@
>   
>   #include <_ansi.h>
>   #include <unistd.h>
> +#include <errno.h>
>   
>   
>   #include <stdarg.h>
>   
> +#ifndef ARG_NUM_MAX
> +#define ARG_NUM_MAX 256
> +#endif
> +
>   int
>   execlp (const char *path,
>         const char *arg0, ...)
> @@ -19,14 +24,20 @@ execlp (const char *path,
>   {
>     int i;
>     va_list args;
> -  const char *argv[256];
> +  const char *argv[ARG_NUM_MAX];
>   
>     va_start (args, arg0);
>     argv[0] = arg0;
>     i = 1;
> -  do
> -      argv[i] = va_arg (args, const char *);
> -  while (argv[i++] != NULL);
> +  do {
> +    if(i>=ARG_NUM_MAX)
> +    {
> +      va_end (args);
> +      errno=E2BIG;
> +      return -1;
> +    }
> +    argv[i] = va_arg (args, const char *);
> +  } while (argv[i++] != NULL);
>     va_end (args);
>   
>     return execvp (path, (char * const *) argv);
  
Corinna Vinschen Jan. 27, 2025, 1:21 p.m. UTC | #2
Hi Federico,

On Jan 26 12:14, Federico Terraneo wrote:
> Hi,
> I propose the following patch to the execl* functions in newlib that does
> the following:
> - it returns an error if more than the maximum number of arguments are
> passed instead of corrupting the stack
> - it allows to tweak the maximum number of arguments at the time of
> compiling newlib by defining the ARG_NUM_MAX macro
> 
> This patch is originally meant for the Miosix OS to allow reducing the
> maximum number of arguments with the intent of reducing the stack usage of
> the execl* functions in low memory microcontrollers, but I think it is of
> general applicability and can of course be useful also to increase this
> value for desktop-class targets.
> 
> Best regards,
> Federico Terraneo
> 
> 

I'm sorry, but I don't think this is the right thing to do.  Actually,
not even for Miosix OS.  Here's why:

Adding ARG_NUM_MAX this way requires all targets to define their own
ARG_NUM_MAX if they want to support more than 256 args.  Right now,
there's no such limitation.

There already is a standarized way for all targets to tell userspace how
much memory can be used by the exec(3) family of functions:

  ARG_MAX from limits.h or sysconf (_SC_ARG_MAX) from unistd.h

  must be >= _POSIX_ARG_MAX (4096)

ARG_MAX does not define a max number of args.  It defines a max number
of bytes used for arguments and environment combined.  This is what
should be tested for in exec(3), if you want to enforce memory usage
restrictions.


Thanks,
Corinna
  
Federico Terraneo Jan. 27, 2025, 2:36 p.m. UTC | #3
Hi Corinna,

On 27/01/25 14:21, Corinna Vinschen wrote:
> Adding ARG_NUM_MAX this way requires all targets to define their own
> ARG_NUM_MAX if they want to support more than 256 args.  Right now,
> there's no such limitation.

As far as I can see, right now there is this limitation in newlib.
Let's have a look at the current code of one of these functions:

https://sourceware.org/git?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/posix/execl.c

What I see is a

const char *argv[256];

declared on the stack, and a do..while loop copying args into that array 
with no bound checking. Thus, attempting to pass more than 256 arguments 
will silently corrupt the stack.

Attached please find a test case that when compiled with the address 
sanitizer fails due to the stack buffer overflow when more than 256 
arguments are passed to the execl code above.

The purpose of my patch is to:
- avoid the stack buffer overflow in all cases
- allow targets to override the default 256 arguments limit if they so 
desire, either lowering to reduce stack use or increasing it if 256 is 
not enough

Let me know if I'm missing something.

Best regards,
Federico Terraneo
  
Corinna Vinschen Jan. 27, 2025, 5:26 p.m. UTC | #4
On Jan 27 15:36, Federico Terraneo wrote:
> Hi Corinna,
> 
> On 27/01/25 14:21, Corinna Vinschen wrote:
> > Adding ARG_NUM_MAX this way requires all targets to define their own
> > ARG_NUM_MAX if they want to support more than 256 args.  Right now,
> > there's no such limitation.
> 
> As far as I can see, right now there is this limitation in newlib.
> Let's have a look at the current code of one of these functions:
> 
> https://sourceware.org/git?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/posix/execl.c
> 
> What I see is a
> 
> const char *argv[256];
> 
> declared on the stack, and a do..while loop copying args into that array
> with no bound checking. Thus, attempting to pass more than 256 arguments
> will silently corrupt the stack.
> 
> Attached please find a test case that when compiled with the address
> sanitizer fails due to the stack buffer overflow when more than 256
> arguments are passed to the execl code above.
> 
> The purpose of my patch is to:
> - avoid the stack buffer overflow in all cases
> - allow targets to override the default 256 arguments limit if they so
> desire, either lowering to reduce stack use or increasing it if 256 is not
> enough

Huh, you're right.  Apologies, I was not reading all of your patch and
was thinking of the Cygwin exec(3), which doesn't have that restriction.

I checked the history, the exec functions are using this static stack
allocation since the last century, before the CVS repo was created.

If we keep your patch, please redefine ARG_NUM_MAX to _ARG_NUM_MAX.

But it would still be nice to have a more dynamic solution which doesn't
need ARG_NUM_MAX.


Thanks,
Corinna
  
Federico Terraneo Jan. 29, 2025, 8:53 a.m. UTC | #5
On 27/01/25 18:26, Corinna Vinschen wrote:
> But it would still be nice to have a more dynamic solution which doesn't
> need ARG_NUM_MAX.

Hi Corinna,
I spent some time into it, and it's more complicated than I wanted, but 
I have a patch in this direction nonetheless, that replaces the previous 
patch proposal.

The complexity is caused by POSIX.1-2008 which as far as I could tell 
requires execl* functions to be callable from signal handlers, while 
malloc isn't. That leaves alloca as the only way I can think of to lift 
the argument number limitation.

The issue is, newlib targets also microcontroller targets where there's 
no virtual memory and stacks are small (our Miosix OS is one example), 
thus unbounded stack allocation (or just large stack allocations like 
the 1024bytes array in the current version of the execl* code) cause 
stack overflows and undefined behavior.

Thus I propose the attached patch that by default uses alloca but allows 
individual targets to define _EXECL_USE_MALLOC if in their use case the 
risk of stack overflows and consequent undefined behavior is a more 
pressing requirement than the corner case of allowing execl* calls from 
signal handlers.

Even with the default alloca implementation, the patched code uses 
*less* stack than the current code when execl* functions are called with 
less that 256 arguments, and on platforms where the stack can grow (i.e, 
when virtual memory is available) it entirely avoids the stack buffer 
overflow that was present in the current code, so I think it's an 
overall improvement.

Best regards,
Federico Terraneo
  
R. Diez Jan. 29, 2025, 9:07 a.m. UTC | #6
Hallo Federico Terraneo:

> The complexity is caused by POSIX.1-2008 which
> [...]
> allows individual targets to define _EXECL_USE_MALLOC

The reason why _EXECL_USE_MALLOC exists in your patch is well explained in your e-mail, but an e-mail is not the right place for documentation.

In order to help future developers looking at the code, I would place the reason as a comment over the first "#ifndef _EXECL_USE_MALLOC".

Best regards,
   rdiez
  
Corinna Vinschen Jan. 29, 2025, 11:25 a.m. UTC | #7
Hi Federico,

On Jan 26 12:14, Federico Terraneo wrote:
> Hi,
> I propose the following patch to the execl* functions in newlib that does
> the following:
> - it returns an error if more than the maximum number of arguments are
> passed instead of corrupting the stack
> - it allows to tweak the maximum number of arguments at the time of
> compiling newlib by defining the ARG_NUM_MAX macro
> 
> This patch is originally meant for the Miosix OS to allow reducing the
> maximum number of arguments with the intent of reducing the stack usage of
> the execl* functions in low memory microcontrollers, but I think it is of
> general applicability and can of course be useful also to increase this
> value for desktop-class targets.
> 
> Best regards,
> Federico Terraneo
> 
> 

I'm sorry, but I don't think this is the right thing to do.  Actually,
not even for Miosix OS.  Here's why:

Adding ARG_NUM_MAX this way requires all targets to define their own
ARG_NUM_MAX if they want to support more than 256 args.  Right now,
there's no such limitation.

There already is a standarized way for all targets to tell userspace how
much memory can be used by the exec(3) family of functions:

  ARG_MAX from limits.h or sysconf (_SC_ARG_MAX) from unistd.h

  must be >= _POSIX_ARG_MAX (4096)

ARG_MAX does not define a max number of args.  It defines a max number
of bytes used for arguments and environment combined.  This is what
should be tested for in exec(3), if you want to enforce memory usage
restrictions.


Thanks,
Corinna
  
Corinna Vinschen Jan. 29, 2025, 11:26 a.m. UTC | #8
On Jan 29 09:53, Federico Terraneo wrote:
> On 27/01/25 18:26, Corinna Vinschen wrote:
> > But it would still be nice to have a more dynamic solution which doesn't
> > need ARG_NUM_MAX.
> 
> Hi Corinna,
> I spent some time into it, and it's more complicated than I wanted, but I
> have a patch in this direction nonetheless, that replaces the previous patch
> proposal.
> 
> The complexity is caused by POSIX.1-2008 which as far as I could tell
> requires execl* functions to be callable from signal handlers, while malloc
> isn't. That leaves alloca as the only way I can think of to lift the
> argument number limitation.
> 
> The issue is, newlib targets also microcontroller targets where there's no
> virtual memory and stacks are small (our Miosix OS is one example), thus
> unbounded stack allocation (or just large stack allocations like the
> 1024bytes array in the current version of the execl* code) cause stack
> overflows and undefined behavior.
> 
> Thus I propose the attached patch that by default uses alloca but allows
> individual targets to define _EXECL_USE_MALLOC if in their use case the risk
> of stack overflows and consequent undefined behavior is a more pressing
> requirement than the corner case of allowing execl* calls from signal
> handlers.
> 
> Even with the default alloca implementation, the patched code uses *less*
> stack than the current code when execl* functions are called with less that
> 256 arguments, and on platforms where the stack can grow (i.e, when virtual
> memory is available) it entirely avoids the stack buffer overflow that was
> present in the current code, so I think it's an overall improvement.

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.


Thanks,
Corinna
  
Federico Terraneo Jan. 31, 2025, 9:53 a.m. UTC | #9
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
  
Richard Earnshaw (lists) Jan. 31, 2025, 10:55 a.m. UTC | #10
On 31/01/2025 09:53, Federico Terraneo wrote:
> 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

You need to use the vanilla autoconf obtainable from the FSF.  Debian's version contains changes that generate the extra verbage that you are seeing.

R.
  
Federico Terraneo Jan. 31, 2025, 12:59 p.m. UTC | #11
On 31/01/25 11:55, Richard Earnshaw (lists) wrote:
> You need to use the vanilla autoconf obtainable from the FSF.  Debian's version contains changes that generate the extra verbage that you are seeing.

Hi,
I tried installing autoconf and automake from sources, see the command 
list attached below, but I'm still getting the same long list of changes 
in newlib/Makefile.in.

However, the first of these changes in the patch is the removal of the line

@HAVE_LIBC_SYS_XTENSA_DIR_TRUE@am__append_64 = libc/sys/xtensa/creat.c 
libc/sys/xtensa/isatty.c libc/sys/xtensa/clibrary_init.c

and I don't have the newlib/libc/sys/xtensa directory. I'm making my 
patch starting from commit

d52d983e5b69457c706c34097a328a7678107b1a
Author: Corinna Vinschen <corinna@vinschen.de>  2025-01-28 16:50:12
Cygwin: implement posix_close

Is it possible that the directory libc/sys/xtensa got removed, somebody 
did not run autoreconf and thus Makefile.in is in a somewhat 
inconsistent state in the git repository?


---
How I installed autoconf & automake

wget https://ftpmirror.gnu.org/autoconf/autoconf-2.69.tar.gz
wget https://ftpmirror.gnu.org/automake/automake-1.15.1.tar.gz
mkdir autobins
PFX=`pwd`/autobins
tar xvf autoconf-2.69.tar.gz
cd autoconf-2.69
./configure --prefix=$PFX
make
make install
cd ..
tar xvf automake-1.15.1.tar.gz
cd automake-1.15.1
./configure --prefix=$PFX
make
make install
cd ..
export PATH=`pwd`/autobins/bin:$PATH
  
Richard Earnshaw (lists) Jan. 31, 2025, 1:28 p.m. UTC | #12
On 31/01/2025 12:59, Federico Terraneo wrote:
> On 31/01/25 11:55, Richard Earnshaw (lists) wrote:
>> You need to use the vanilla autoconf obtainable from the FSF.  Debian's version contains changes that generate the extra verbage that you are seeing.
> 
> Hi,
> I tried installing autoconf and automake from sources, see the command list attached below, but I'm still getting the same long list of changes in newlib/Makefile.in.
> 
> However, the first of these changes in the patch is the removal of the line
> 
> @HAVE_LIBC_SYS_XTENSA_DIR_TRUE@am__append_64 = libc/sys/xtensa/creat.c libc/sys/xtensa/isatty.c libc/sys/xtensa/clibrary_init.c
> 
> and I don't have the newlib/libc/sys/xtensa directory. I'm making my patch starting from commit
> 
> d52d983e5b69457c706c34097a328a7678107b1a
> Author: Corinna Vinschen <corinna@vinschen.de>  2025-01-28 16:50:12
> Cygwin: implement posix_close
> 
> Is it possible that the directory libc/sys/xtensa got removed, somebody did not run autoreconf and thus Makefile.in is in a somewhat inconsistent state in the git repository?
> 
> 
> ---
> How I installed autoconf & automake
> 
> wget https://ftpmirror.gnu.org/autoconf/autoconf-2.69.tar.gz
> wget https://ftpmirror.gnu.org/automake/automake-1.15.1.tar.gz
> mkdir autobins
> PFX=`pwd`/autobins
> tar xvf autoconf-2.69.tar.gz
> cd autoconf-2.69
> ./configure --prefix=$PFX
> make
> make install
> cd ..
> tar xvf automake-1.15.1.tar.gz
> cd automake-1.15.1
> ./configure --prefix=$PFX
> make
> make install
> cd ..
> export PATH=`pwd`/autobins/bin:$PATH
> 

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.
  
Federico Terraneo Jan. 31, 2025, 3:02 p.m. UTC | #13
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 <alexey.lapshin@espressif.com>
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.
  
C Howland Jan. 31, 2025, 4:32 p.m. UTC | #14
> ------------------------------
> *From:* Federico Terraneo <fede.tft@hotmail.it>
> *Sent:* Friday, January 31, 2025 10:02 AM
> *To:* Richard Earnshaw (lists) <Richard.Earnshaw@arm.com>;
> newlib@sourceware.org <newlib@sourceware.org>
> *Subject:* Re: [PATCH] Improve execl* functions
>
>
> ...
>
> 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.
>
Federico:
     Two minor things.
     First, typo in repeated comment "override the default an use malloc()
*/", change "an" to "and".
     Second, suggest gating the associated include files as the code is
gated (marked as a patch to the result of your patch):

+#ifdef _EXECL_USE_MALLOC
 #include <errno.h>
 #include <stdlib.h>
+#else
 #include <alloca.h>
+#endif

Craig
  
Federico Terraneo Jan. 31, 2025, 4:42 p.m. UTC | #15
Hi,
updated patch 0002.
  
Corinna Vinschen Feb. 3, 2025, 2:06 p.m. UTC | #16
Hi Federico,

On Jan 31 17:42, Federico Terraneo wrote:
> Hi,
> updated patch 0002.

Both patches pushed.


Thanks,
Corinna


> From a13bf0a67ab85138cd31a6673494f30284fb9b60 Mon Sep 17 00:00:00 2001
> From: Terraneo Federico <fede.tft@miosix.org>
> 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  | 36 ++++++++++++++++++++++++++++++++++--
>  newlib/libc/posix/execle.c | 36 ++++++++++++++++++++++++++++++++++--
>  newlib/libc/posix/execlp.c | 31 +++++++++++++++++++++++++++++--
>  newlib/newlib.hin          |  3 +++
>  7 files changed, 140 insertions(+), 6 deletions(-)
  

Patch

From 9d06b54b1f2a2957bc40af10c6dc09867662325f Mon Sep 17 00:00:00 2001
From: Terraneo Federico <fede.tft@miosix.org>
Date: Sat, 25 Jan 2025 11:51:15 +0100
Subject: [PATCH] Fix stack overflow in exec*; add ARG_NUM_MAX

---
 newlib/libc/posix/execl.c  | 19 +++++++++++++++----
 newlib/libc/posix/execle.c | 17 ++++++++++++++---
 newlib/libc/posix/execlp.c | 19 +++++++++++++++----
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/newlib/libc/posix/execl.c b/newlib/libc/posix/execl.c
index c3b4e55bd..044205cc6 100644
--- a/newlib/libc/posix/execl.c
+++ b/newlib/libc/posix/execl.c
@@ -7,6 +7,7 @@ 
 
 #include <_ansi.h>
 #include <unistd.h>
+#include <errno.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
@@ -16,6 +17,10 @@  static char ***p_environ = &environ;
 
 #include <stdarg.h>
 
+#ifndef ARG_NUM_MAX
+#define ARG_NUM_MAX 256
+#endif
+
 int
 execl (const char *path,
       const char *arg0, ...)
@@ -24,14 +29,20 @@  execl (const char *path,
 {
   int i;
   va_list args;
-  const char *argv[256];
+  const char *argv[ARG_NUM_MAX];
 
   va_start (args, arg0);
   argv[0] = arg0;
   i = 1;
-  do
-      argv[i] = va_arg (args, const char *);
-  while (argv[i++] != NULL);
+  do {
+    if(i>=ARG_NUM_MAX)
+    {
+      va_end (args);
+      errno=E2BIG;
+      return -1;
+    }
+    argv[i] = va_arg (args, const char *);
+  } while (argv[i++] != NULL);
   va_end (args);
 
   return _execve (path, (char * const  *) argv, *p_environ);
diff --git a/newlib/libc/posix/execle.c b/newlib/libc/posix/execle.c
index 34f0ea373..7a25ae8ef 100644
--- a/newlib/libc/posix/execle.c
+++ b/newlib/libc/posix/execle.c
@@ -7,10 +7,15 @@ 
 
 #include <_ansi.h>
 #include <unistd.h>
+#include <errno.h>
 
 
 #include <stdarg.h>
 
+#ifndef ARG_NUM_MAX
+#define ARG_NUM_MAX 256
+#endif
+
 int
 execle (const char *path,
       const char *arg0, ...)
@@ -20,14 +25,20 @@  execle (const char *path,
   int i;
   va_list args;
   const char * const *envp;
-  const char *argv[256];
+  const char *argv[ARG_NUM_MAX];
 
   va_start (args, arg0);
   argv[0] = arg0;
   i = 1;
-  do
+  do {
+    if(i>=ARG_NUM_MAX)
+    {
+      va_end (args);
+      errno=E2BIG;
+      return -1;
+    }
     argv[i] = va_arg (args, const char *);
-  while (argv[i++] != NULL);
+  } while (argv[i++] != NULL);
   envp = va_arg (args, const char * const *);
   va_end (args);
 
diff --git a/newlib/libc/posix/execlp.c b/newlib/libc/posix/execlp.c
index b845c88c5..a437a56f0 100644
--- a/newlib/libc/posix/execlp.c
+++ b/newlib/libc/posix/execlp.c
@@ -7,10 +7,15 @@ 
 
 #include <_ansi.h>
 #include <unistd.h>
+#include <errno.h>
 
 
 #include <stdarg.h>
 
+#ifndef ARG_NUM_MAX
+#define ARG_NUM_MAX 256
+#endif
+
 int
 execlp (const char *path,
       const char *arg0, ...)
@@ -19,14 +24,20 @@  execlp (const char *path,
 {
   int i;
   va_list args;
-  const char *argv[256];
+  const char *argv[ARG_NUM_MAX];
 
   va_start (args, arg0);
   argv[0] = arg0;
   i = 1;
-  do
-      argv[i] = va_arg (args, const char *);
-  while (argv[i++] != NULL);
+  do {
+    if(i>=ARG_NUM_MAX)
+    {
+      va_end (args);
+      errno=E2BIG;
+      return -1;
+    }
+    argv[i] = va_arg (args, const char *);
+  } while (argv[i++] != NULL);
   va_end (args);
 
   return execvp (path, (char * const *) argv);
-- 
2.39.5