Re: [PATCH 1/2] posix: execvpe cleanup

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 25-01-2016 14:52, Paul Eggert wrote:
> On 01/25/2016 06:24 AM, Adhemerval Zanella wrote:
>> -scripts_argv (const char *file, char *const argv[], int argc, char **new_argv)
>> +maybe_script_execute (const char *path, char *const argv[], char *const envp[])
> 
> Why change the arg name from "file" to "path"? The GNU tradition is to use names like "path" for PATH and the like, not for file names that may contain slashes.
> 

I will change  that.

>> +  int argc = 0;
>> +  while (argv[argc++]);
> 
> Please don't format 'while' loops that way. Use 'continue;'. Also, what happens if argc exceeds INT_MAX?

I will change the loop form.  One option to handle the INT_MAX is to limit argc
counting to ARG_MAX and return E2BIG if it is the case.

> 
>>     /* Construct an argument list for the shell.  */
>> +  char *new_argv[argc];
> 
> Why can't this overflow the stack when ARGC is large? The original code tried to check for this overflow and do the right thing; why remove the check, flawed as it was?
> 
> 

It will if the stack is not large enough and as the main idea is to allow posix_spawnp
use execvpe internally instead of reimplement it itself.  As my previous answer to 
Andreas I see some options on how to handle it:

1. Limit total stack allocation to a predefined value. Linux practically does not
   imposes limits to argument number (although ARG_MAX is defined to 131072, 
   fs/exec.c:_do_execve code itself checks against a value of INT_MAX, 2^32-1)
   so I think practically ARG_MAX is a feasible value which gives us a potentially
   stack allocation of 512 for 32-bit and 1MB for 64-bit.  It could be less for
   this maybe_script_run.

2. Just drop this execve path and document that by using along either vfork/clone
   can lead to memory leak.  I can work on posix_spawn{p} to use another version
   of execvpe with memory limit.

I really would like to no go on 2 for some reason: 1. from a QoI I see that the
dynamic allocation limits the exevpe utilization for only fork model (although
I do see that practically this would be most common use case), and 2. it will
required some code duplication on posix_spawnp.  So I am open to suggestions.