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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 26-02-2016 17:49, Adhemerval Zanella wrote:
> 
> 
> On 26-02-2016 16:38, Paul Eggert wrote:
>> This one has similar problems with int vs ptrdiff_t. Also:
>>
>> On 02/26/2016 05:56 AM, Adhemerval Zanella wrote:
>>>   for (const char *p = path; ; p = subp)
>>>     {
>>>        if (errno == ENOEXEC)
>>>         maybe_script_execute (buffer, argv, envp);
>>
>> This has O(P*C) behavior, where P is the length of the path and C is the argument count. How about changing it to have O(P + C) behavior instead, by allocating the substitute argv in __execvpe, and reusing it each time through the loop? (Admittedly the current code also has this performance bug.)
>>
> 
> I do not oppose, although I would like to focus on fixing the usability and
> conformance bugs first and set the performance goal as possible future
> improvement. I will add a comment about this possible optimization.
> 
>>>   /* Construct an argument list for the shell.  */
>>>   char *new_argv[argc];
>>
>> This should be "argc +1", not "argc". Shouldn't we have a test case to catch bugs like this?
> 
> Indeed I will change that.  I add a testcase.
> 
>>
>>>   new_argv[0] = (char *) _PATH_BSHELL;
>>>   new_argv[1] = (char *) file;
>>>   while (argc > 1)
>>>     {
>>>       new_argv[argc] = argv[argc - 1];
>>>       --argc;
>>>     }
>>
>> This mishandles the case where argc == 1, which is possible with an empty argument vector. The resulting argument vector is not null-terminated. (Admittedly the current code also has this bug.) I suppose we should have a test case for this.
>>
> 
> I will fix that.

In fact I think original code is correct. Although execvpe is an GNU extension I
see it should follow the specification of already defined exec* POSIX functions.
And according to POSIX [1], "argv[0] should point to a filename string that 
is associated with the process being started by one of the exec functions" (this
differ with man pages with uses the wording 'by convention' to define first
argument should point to process being stated).

So I see that:

  char *args[] = { NULL }; 
  execvpe (scriptname, args, NULL)

Which triggers the issue is invalid.