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 08:34, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> diff --git a/posix/spawn.h b/posix/spawn.h
>> index 471dbea022..095ee67a26 100644
>> --- a/posix/spawn.h
>> +++ b/posix/spawn.h
>> @@ -213,6 +213,12 @@ extern int posix_spawn_file_actions_addchdir_np (posix_spawn_file_actions_t *
>> extern int posix_spawn_file_actions_addfchdir_np (posix_spawn_file_actions_t *,
>> int __fd)
>> __THROW __nonnull ((1));
>> +
>> +/* Add an action to close all file descriptor greater than FROM durint
>> + spawn. This affects the subsequent file actions. */
>> +extern int posix_spawn_file_actions_addclosefrom_np (posix_spawn_file_actions_t *,
>> + int __from);
>> +
>
> Missing __THROW __nonnull ((11)), I think.
Ack.
>
>> diff --git a/posix/spawn_faction_addclosefrom.c b/posix/spawn_faction_addclosefrom.c
>> new file mode 100644
>> index 0000000000..24e6c62dbc
>> --- /dev/null
>> +++ b/posix/spawn_faction_addclosefrom.c
>
> Can we implement this on Hurd fairly quickly? Otherwise, this should
> probable be a stub that returns ENOSYS.
Not sure, I will need to check how to accomplish it on Hurd. I think it would
be better to add a ENOSYS wrapper for now.
>
>> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
>> new file mode 100644
>> index 0000000000..7ed03810e4
>
>> +static int
>> +do_test (int argc, char *argv[])
>> +{
>> + /* We must have one or four parameters left if called initially:
>> + + path for ld.so optional
>> + + "--library-path" optional
>> + + the library path optional
>> + + the application name
>> +
>> + Plus one parameter to indicate which test to execute through
>> + re-execution.
>> +
>> + So for default usage without --enable-hardcoded-path-in-tests, it
>> + will be called initially with 5 arguments and later with 2. For
>> + --enable-hardcoded-path-in-tests it will be called with 2 arguments
>> + regardless. */
>
> You could run this as a container test, so that this would become
> unnecessary. (Or link statically.)
Right, I think maybe container could be an option. Another possibility is
to add some API to make this process re-spawn less bloated.
>
> The test doesn't exercise the gaps case.
Do you mean gaps in file descriptor initial set before posix_spawn?
>
>> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
>> index 8e3ba480b7..b576b3233d 100644
>> --- a/sysdeps/posix/opendir.c
>> +++ b/sysdeps/posix/opendir.c
>
>> +DIR *
>> +__alloc_dir (int fd, bool close_fd, const struct stat64 *statp,
>> + void *buffer, size_t size)
>> {
>> /* We have to set the close-on-exit flag if the user provided the
>> file descriptor. */
>> @@ -114,31 +147,44 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
>> allocation = MIN (MAX ((size_t) statp->st_blksize, default_allocation),
>> MAX_DIR_BUFFER_SIZE);
>> #endif
>> -
>> - DIR *dirp = (DIR *) malloc (sizeof (DIR) + allocation);
>> - if (dirp == NULL)
>> + DIR *dirp;
>> + if (!buffer)
>
> Style issue, I think: buffer != NULL.
>
> Otherwise, this is a fairly nice hack to get an async-signal-safe
> readdir. But I'm not sure that it's what we need here. See below.
>
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index c1abf3f960..fe0ff95825 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>
>> +/* 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.
>
>> + 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.
>
>> + bool ret = true;
>> + struct dirent *dirp;
>> + while ((dirp = __readdir (dp)) != NULL)
>
> Should this be __readdir64?
It should indeed, we should definitely move away from non-LFS calls.
>
>> + {
>> + 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:
--
#include <unistd.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <assert.h>
#include <dirent.h>
#include <limits.h>
int main (int argc, char *argv[])
{
int fd1 = open ("/dev/null", O_WRONLY);
assert (fd1 != -1);
int fd2 = open ("/dev/null", O_WRONLY);
assert (fd2 != -1);
int fd3 = open ("/dev/null", O_WRONLY);
assert (fd3 != -1);
int fd4 = open ("/dev/null", O_WRONLY);
assert (fd4 != -1);
printf ("fd1=%d fd2=%d fd3=%d fd4=%d\n", fd1, fd2, fd3, fd4);
system ("ls /proc/self/fd");
int from = fd1 - 1;
DIR *dp = opendir ("/proc/self/fd");
assert (dp != NULL);
struct dirent *dirp;
while ((dirp = readdir (dp)) != NULL)
{
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)
break;
if (fd == dirfd (dp) || fd < from)
continue;
close (fd);
system ("ls /proc/self/fd");
}
closedir (dp);
return 0;
}
$ ./testrun.sh /tmp/test
fd1=3 fd2=4 fd3=5 fd4=6
0 1 2 3 4 5 6 7
0 1 2 3 4 5 6
0 1 2 4 5 6
0 1 2 5 6
0 1 2 6
0 1 2
$ ./testrun.sh --tool=strace /tmp/test 2>&1 | grep getdents64
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 1 entries */, 40) = 24
getdents64(7, /* 0 entries */, 40) = 0
--
I tried also to filter out an additional file descriptor by setting the
condition:
if (fd == dirfd (dp) || fd < from || fd == fd2)
continue;
And the result seems what is expected:
$ ./testrun.sh /tmp/test
fd1=3 fd2=4 fd3=5 fd4=6
0 1 2 3 4 5 6 7
0 1 2 3 4 5 6
0 1 2 4 5 6
0 1 2 4 6
0 1 2 4
I haven't actually read the kernel source to check what exactly getdents
does for /proc/self/fd updates neither if it handles it differently.
>
> If you share my concerns, maybe we should call getdents64 directly and
> parse the buffer? And restart after the buffer has been exhausted?
> (I have a patch with such parser functions and planned to submit it
> after the getdents64 syscall wrapper went in.)
I though about calling getdents64 directly, but I think we would just ended up
replicating readdir code on closefrom. The __opendir_inplace has the extra
advantage to work with readdir.
>
>> +
>> /* Function used in the clone call to setup the signals mask, posix_spawn
>> attributes, and file actions. It run on its own stack (provided by the
>> posix_spawn call). */
>> @@ -280,6 +321,11 @@ __spawni_child (void *arguments)
>> if (__fchdir (action->action.fchdir_action.fd) != 0)
>> goto fail;
>> break;
>> +
>> + case spawn_do_closefrom:
>> + if (!spawn_closefrom (action->action.closefrom_action.from))
>> + goto fail;
>> + break;
>> }
>> }
>
> The Hurd and generic implementations will need to be updated to handle
> spawn_do_closefrom as well, otherwise they will fail to build.
Ack.