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: Fri, 26 Feb 2016 17:49:33 -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>
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.
>> bool got_eacces = false;
>> for (const char *p = path; ; p = subp)
>> {
>> char buffer[path_len + file_len + 1];
>
> Declare 'buffer' just before the loop starts, not in the loop body. That will make the code run a bit faster, as the buffer will be allocated and deallocated only once. This is OK since (path_len + file_len + 1) does not change.
>
I will change that.
>> /* Set current path considered plus a '/'. */
>> memcpy (buffer, p, subp - p);
>> buffer[subp - p] = '/';
>> /* And the file to execute. */
>> memcpy (buffer + (subp - p) + !!(subp > p), file, file_len + 1);
>
> Shouldn't this be using mempcpy? That should simplify the code. I find the "!!" ugly and unnecessary; there's nothing wrong with treating a boolean as an int, and besides, !! on a boolean still gives you a boolean so what's the point? Something like this, perhaps:
>
> /* Use the current path entry, plus a '/' if nonempty, plus the file to execute. */
> char *pend = mempcpy (buffer, p, subp - p);
> *pend = '/';
> memcpy (pend + (p < subp), file, file_len + 1);
>
>
I do not have a preference regarding to "!!" and your suggestion seems better.
I will change that.
>> + /* Record the we got a 'Permission denied' error. If we end
>
> the -> that
Ack.
>
>> +# define EXECVP(__file, __argv) execvp (__file, __argv)
>
> No need for the underscores here.
OK, I will change that.