Re: [PATCH] Improve execl* functions

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 <[email protected]>
> 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);