Re: [PATCH] Improve execl* functions

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