This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]