[PATCH 2/3] posix: execvpe cleanup

[Paul Eggert]

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.