Re: [PATCH v2 1/2] posix: execvpe cleanup

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 27-01-2016 14:17, Paul Eggert wrote:
> Adhemerval Zanella wrote:
>> +  int argc = 0;
>> +  while (argv[argc++])
>> +    if (argc > NCARGS)
> 
> This won't work on platforms where NCARGS is INT_MAX.
> 
>> +  char *new_argv[argc];
> 
> This can allocate up to NCARGS * sizeof (char *) bytes on the stack, which is too much. I suggest allocating on the stack only if __libc_use_alloca says it's OK.
> 

Right, I can change to:

--
  int argc = 0;
  do
    if ((argc+1) == NCARGS)
      { 
        errno = E2BIG;
        return;
      } 
  while (argv[argc++] != NULL);

  /* Linux accepts a very large argument number (INT_MAX to fit on a signed
     32-bit integer).  To limit stack allocation we set it to a lower
     bound.  */
  if (!__libc_alloca_cutoff (argc * sizeof (char*)))
    {
      errno = E2BIG;
      return;
    }
--

This will take care of argc overflow and stack allocation.  The only problem is
for such code path (execvpe for a shell script without shebang) total arguments
will be limited to __MAX_ALLOCA_CUTOFF / sizeof (char*), which for current
glibc settings would be 16384 for 32 bits and 8192.  I can live with it, specially
since it a functionality which IMHO we should not have provided in first place
(it is provided only on compat mode for posix_spawn{p}).


>> +  size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
> 
> What about platforms that don't define PATH_MAX because there's no limit? Or what if PATH_MAX is larger than what __libc_use_alloca would allow?

I noted glibc code usually set PATH_MAX to 1024 if it is not defined, so following
the code system/posix/getcwd.c should suffice:

#ifndef PATH_MAX
# ifdef MAXPATHLEN
#  define PATH_MAX MAXPATHLEN
# else
#  define PATH_MAX 1024
# endif
#endif

About '__libc_use_alloca' I can add a test like that:

  size_t file_len = __strnlen (file, NAME_MAX + 1);
  size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;

  if ((file_len > NAME_MAX)
      || !__libc_alloca_cutoff (path_len + file_len + 1))
    {
      errno = ENAMETOOLONG;
      return -1;
    }

We current __MAX_ALLOCA_CUTOFF of 65536 I think we have plenty of room for various
directory limits configurations.