This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/3] posix: execvpe cleanup
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Paul Eggert <eggert at cs dot ucla dot edu>, libc-alpha at sourceware dot org
- Date: Sat, 27 Feb 2016 14:04:55 -0300
- Subject: Re: [PATCH 2/3] posix: execvpe cleanup
- Authentication-results: sourceware.org; auth=none
- References: <1456495001-5298-1-git-send-email-adhemerval dot zanella at linaro dot org> <1456495001-5298-3-git-send-email-adhemerval dot zanella at linaro dot org> <56D0A9CC dot 4080608 at cs dot ucla dot edu> <56D0BA5D dot 5080103 at linaro dot 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.