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: Paul Eggert <eggert at cs dot ucla dot edu>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, libc-alpha at sourceware dot org
- Date: Fri, 26 Feb 2016 11:38:52 -0800
- 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>
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.)
/* 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?
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.
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.
/* 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);
+ /* Record the we got a 'Permission denied' error. If we end
the -> that
+# define EXECVP(__file, __argv) execvp (__file, __argv)
No need for the underscores here.