This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] RFC: Add posix_spawn_file_actions_closefrom
On 24/05/2019 11:37, Florian Weimer wrote:
> * 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.
Ok, I would add a test to check for it.
>
>>>> +/* 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.
Yeah, this is exactly the random close calls I would like to avoid. But
I also don't see a better option.
>
>>>> + {
>>>> + 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.
>
I will check with different permutations by changing the opendir buffer
and a file descriptor set with different set of gaps.