Re: [PATCH] RFC: Add posix_spawn_file_actions_closefrom

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

>> The test doesn't exercise the gaps case.
>
> Do you mean gaps in file descriptor initial set before posix_spawn?

Yes, where the directory descriptor is in the middle of the closefrom
range.

>>> +/* Close all file descriptor up to FROM by interacting /proc/self/fd.
>>> +   Any failure should */
>>> +static bool
>>> +spawn_closefrom (int from)
>>> +{
>>> +  /* Increasing the buffer size incurs in less getdents syscalls from
>>> +     readdir, however it would require more stack size to be allocated
>>> +     on __spawnix.  */
>>> +  char buffer[sizeof (struct __dirstream) + 2 * sizeof (struct dirent)];
>> 
>> We could allocate this on the heap, in the parent.  Maybe we could
>> opendir in the parent, and play with the underlying descriptor in the
>> child?  Then you wouldn't need to add __opendir_inplace at all.  Given
>> that we know what our implementation looks like, this should be fairly
>> safe.
>
> I don't have a strong opinion here, it would add some complexity on parent
> helper which would need to transverse all file actions, call opendir, and
> deallocate after helper process returns.  My idea is to keep the required 
> logic more in place, so its more obvious where things are initiated.

You could turn one of the padding elements in posix_spawn_file_actions_t
into a flag and have posix_spawn_file_actions_addclosefrom_np set the
flag.  Then the second iteration isn't necessary.

>>> +  DIR *dp;
>>> +  if ((dp = __opendir_inplace ("/proc/self/fd", buffer, sizeof buffer))
>>> +      == NULL)
>>> +    return false;
>> 
>> This could check for ENFILE/EMFILE/ENOMEM and try closing descriptors
>> directly in case of that error, to make room for the new descriptor.
>> But perhaps that's not worth the complexity.
>
> Hum, this could be an enhancement indeed.  However the main issue is 
> to find which is the lower opened file descriptor greater than FROM
> without polling /proc/self/fd or by using close with random file
> descriptors.

You can do close (from), close (from + 1), etc., up to a certain limit,
and retry if one of the close calls doesn't return EBADF.  The magic
limit is needed in case the closefrom does not overlap with any file
descriptors.

>>> +    {
>>> +      if (dirp->d_name[0] == '.')
>>> +        continue;
>>> +
>>> +      char *endptr;
>>> +      long int fd = strtol (dirp->d_name, &endptr, 10);
>>> +      if (*endptr != '\0' || fd < 0 || fd > INT_MAX)
>>> +	{
>>> +	  ret = false;
>>> +	  break;
>>> +	}
>>> +
>>> +      if (fd == dirfd (dp) || fd < from)
>>> +        continue;
>>> +
>>> +      __close (fd);
>>> +    }
>>> +  __closedir (dp);
>>> +
>>> +  return ret;
>>> +}
>> 
>> I'm not sure if this is entirely correct.  If we close some descriptors,
>> and then readdir calls getdents64, what will the kernel return?  Will
>> there be a gap in the descriptor list?  (Curiously, it's the same issue
>> we have the the fork handler list. 8-)
>
> It does not seems to be case with my experiments.  I hack opendir to 
> allocate the minimum workable buffer (__dirstream plus a 
> struct dirent, about 40 bytes on x86_64) to force each readdir to
> call getdents.  A simple testcase shows:

It's still looks very implementation-defined to me.  proc_readfd_common
does this:

	for (fd = ctx->pos - 2;
	     fd < files_fdtable(files)->max_fds;
	     fd++, ctx->pos++) {

And I think ctx->pos somehow corresponds to d_off.  But I don't see a
1:1 correspondence between descriptors and offsets.  I wonder whether
the single-entry case is indeed the worst-possible test case for this.

Thanks,
Florian