This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: GNU C Library <libc-alpha at sourceware dot org>
- Date: Wed, 12 Dec 2018 19:24:36 -0200
- Subject: Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
- References: <20180919224624.3920-1-adhemerval.zanella@linaro.org> <20180919224624.3920-2-adhemerval.zanella@linaro.org> <9981d6d8-48e9-ce63-8205-d45d40cd69f9@linaro.org>
Ping (x2).
On 29/10/2018 16:32, Adhemerval Zanella wrote:
> Ping.
>
> On 19/09/2018 19:46, adhemerval.zanella@linaro.org wrote:
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>> Austin Group issue #411 [1] proposes that posix_spawn file action
>> posix_spawn_file_actions_adddup2 resets the close-on-exec when
>> source and destination refer to same file descriptor.
>>
>> It solves the issue on multi-thread applications which uses
>> close-on-exec as default, and want to hand-chose specifically
>> file descriptor to purposefully inherited into a child process.
>> Current approach to achieve this scenario is to use two adddup2 file
>> actions and a temporary file description which do not conflict with
>> any other, coupled with a close file action to avoid leaking the
>> temporary file descriptor. This approach, besides being complex,
>> may fail with EMFILE/ENFILE file descriptor exaustion.
>>
>> This can be more easily accomplished with an in-place removal of
>> FD_CLOEXEC. Although the resulting adddup2 semantic is slight
>> different than dup2 (equal file descriptors should be handled as
>> no-op), the proposed possible solution are either more complex
>> (fcntl action which a limited set of operations) or results in
>> unrequired operations (dup3 which also returns EINVAL for same
>> file descriptor).
>>
>> Checked on aarch64-linux-gnu.
>>
>> * posix/tst-spawn.c (do_prepare, handle_restart, do_test): Add
>> posix_spawn_file_actions_adddup2 test to check O_CLOCEXEC reset.
>> * sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Add
>> close-on-exec reset for adddup2 file action.
>> * sysdeps/posix/spawni.c (__spawni_child): Likewise.
>>
>> [1] http://austingroupbugs.net/view.php?id=411
>> ---
>> ChangeLog | 6 ++++
>> posix/tst-spawn.c | 47 +++++++++++++++++++++++++-------
>> sysdeps/posix/spawni.c | 21 ++++++++++++--
>> sysdeps/unix/sysv/linux/spawni.c | 21 ++++++++++++--
>> 4 files changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
>> index 638ba1ba55..2354417020 100644
>> --- a/posix/tst-spawn.c
>> +++ b/posix/tst-spawn.c
>> @@ -40,17 +40,19 @@ static int restart;
>> static char *name1;
>> static char *name2;
>> static char *name3;
>> +static char *name5;
>>
>> /* Descriptors for the temporary files. */
>> static int temp_fd1 = -1;
>> static int temp_fd2 = -1;
>> static int temp_fd3 = -1;
>> +static int temp_fd5 = -1;
>>
>> /* The contents of our files. */
>> static const char fd1string[] = "This file should get closed";
>> static const char fd2string[] = "This file should stay opened";
>> static const char fd3string[] = "This file will be opened";
>> -
>> +static const char fd5string[] = "This file should stay opened (O_CLOEXEC)";
>>
>> /* We have a preparation function. */
>> static void
>> @@ -63,24 +65,31 @@ do_prepare (int argc, char *argv[])
>> TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
>> TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
>> TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
>> + TEST_VERIFY_EXIT ((temp_fd5 = create_temp_file ("spawn", &name5)) != -1);
>> +
>> + int flags;
>> + TEST_VERIFY_EXIT ((flags = fcntl (temp_fd5, F_GETFD, &flags)) != -1);
>> + TEST_VERIFY_EXIT (fcntl (temp_fd5, F_SETFD, flags | FD_CLOEXEC) == 0);
>> }
>> #define PREPARE do_prepare
>>
>> static int
>> handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>> - const char *fd4s, const char *name)
>> + const char *fd4s, const char *name, const char *fd5s)
>> {
>> char buf[100];
>> int fd1;
>> int fd2;
>> int fd3;
>> int fd4;
>> + int fd5;
>>
>> /* First get the descriptors. */
>> fd1 = atol (fd1s);
>> fd2 = atol (fd2s);
>> fd3 = atol (fd3s);
>> fd4 = atol (fd4s);
>> + fd5 = atol (fd5s);
>>
>> /* Sanity check. */
>> TEST_VERIFY_EXIT (fd1 != fd2);
>> @@ -89,6 +98,7 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>> TEST_VERIFY_EXIT (fd2 != fd3);
>> TEST_VERIFY_EXIT (fd2 != fd4);
>> TEST_VERIFY_EXIT (fd3 != fd4);
>> + TEST_VERIFY_EXIT (fd4 != fd5);
>>
>> /* First the easy part: read from the file descriptor which is
>> supposed to be open. */
>> @@ -116,6 +126,10 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>> TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
>> TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
>>
>> + TEST_VERIFY_EXIT (lseek (fd5, 0, SEEK_SET) == 0);
>> + TEST_VERIFY_EXIT (read (fd5, buf, sizeof buf) == strlen (fd5string));
>> + TEST_VERIFY_EXIT (memcmp (fd5string, buf, strlen (fd5string)) == 0);
>> +
>> return 0;
>> }
>>
>> @@ -131,6 +145,7 @@ do_test (int argc, char *argv[])
>> char fd2name[18];
>> char fd3name[18];
>> char fd4name[18];
>> + char fd5name[18];
>> char *name3_copy;
>> char *spargv[12];
>> int i;
>> @@ -141,21 +156,24 @@ do_test (int argc, char *argv[])
>> + "--library-path" optional
>> + the library path optional
>> + the application name
>> - - five parameters left if called through re-execution
>> + - six parameters left if called through re-execution
>> + file descriptor number which is supposed to be closed
>> + the open file descriptor
>> + the newly opened file descriptor
>> - + thhe duped second descriptor
>> + + the duped second descriptor
>> + the name of the closed descriptor
>> + + the duped fourth dile descriptor which O_CLOEXEC should be
>> + remove by adddup2.
>> */
>> - if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
>> + if (argc != (restart ? 7 : 2) && argc != (restart ? 7 : 5))
>> FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
>>
>> if (restart)
>> - return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
>> + return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5],
>> + argv[6]);
>>
>> - /* Prepare the test. We are creating two files: one which file descriptor
>> - will be marked with FD_CLOEXEC, another which is not. */
>> + /* Prepare the test. We are creating four files: two which file descriptor
>> + will be marked with FD_CLOEXEC, another which is not */
>>
>> /* Write something in the files. */
>> TEST_VERIFY_EXIT (write (temp_fd1, fd1string, strlen (fd1string))
>> @@ -164,6 +182,8 @@ do_test (int argc, char *argv[])
>> == strlen (fd2string));
>> TEST_VERIFY_EXIT (write (temp_fd3, fd3string, strlen (fd3string))
>> == strlen (fd3string));
>> + TEST_VERIFY_EXIT (write (temp_fd5, fd5string, strlen (fd5string))
>> + == strlen (fd5string));
>>
>> /* Close the third file. It'll be opened by `spawn'. */
>> close (temp_fd3);
>> @@ -183,17 +203,23 @@ do_test (int argc, char *argv[])
>> memset (name3_copy, 'X', strlen (name3_copy));
>>
>> /* We dup the second descriptor. */
>> - fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
>> + fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, MAX (temp_fd3, temp_fd5)))) + 1;
>> TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd2,
>> fd4) == 0);
>>
>> + /* We clear the O_CLOEXEC on fourth descriptor, so it should be
>> + stay open on child. */
>> + TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd5,
>> + temp_fd5) == 0);
>> +
>> /* Now spawn the process. */
>> snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
>> snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
>> snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
>> snprintf (fd4name, sizeof fd4name, "%d", fd4);
>> + snprintf (fd5name, sizeof fd5name, "%d", temp_fd5);
>>
>> - for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
>> + for (i = 0; i < (argc == (restart ? 7 : 5) ? 4 : 1); i++)
>> spargv[i] = argv[i + 1];
>> spargv[i++] = (char *) "--direct";
>> spargv[i++] = (char *) "--restart";
>> @@ -202,6 +228,7 @@ do_test (int argc, char *argv[])
>> spargv[i++] = fd3name;
>> spargv[i++] = fd4name;
>> spargv[i++] = name1;
>> + spargv[i++] = fd5name;
>> spargv[i] = NULL;
>>
>> TEST_VERIFY_EXIT (posix_spawn (&pid, argv[1], &actions, NULL, spargv,
>> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
>> index b138ab4393..d322db5c19 100644
>> --- a/sysdeps/posix/spawni.c
>> +++ b/sysdeps/posix/spawni.c
>> @@ -204,10 +204,25 @@ __spawni_child (void *arguments)
>> break;
>>
>> case spawn_do_dup2:
>> - if (__dup2 (action->action.dup2_action.fd,
>> - action->action.dup2_action.newfd)
>> + /* Austin Group issue #411 requires adddup2 action with source
>> + and destination being equal to remove close-on-exec flag. */
>> + if (action->action.dup2_action.fd
>> != action->action.dup2_action.newfd)
>> - goto fail;
>> + {
>> + if (__dup2 (action->action.dup2_action.fd,
>> + action->action.dup2_action.newfd)
>> + != action->action.dup2_action.newfd)
>> + goto fail;
>> + }
>> + else
>> + {
>> + int fd = action->action.dup2_action.newfd;
>> + int flags = __fcntl (fd, F_GETFD, 0);
>> + if (flags == -1)
>> + goto fail;
>> + if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>> + goto fail;
>> + }
>> break;
>> }
>> }
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index 85239cedbf..b2304ffe60 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>> @@ -253,10 +253,25 @@ __spawni_child (void *arguments)
>> break;
>>
>> case spawn_do_dup2:
>> - if (__dup2 (action->action.dup2_action.fd,
>> - action->action.dup2_action.newfd)
>> + /* Austin Group issue #411 requires adddup2 action with source
>> + and destination being equal to remove close-on-exec flag. */
>> + if (action->action.dup2_action.fd
>> != action->action.dup2_action.newfd)
>> - goto fail;
>> + {
>> + if (__dup2 (action->action.dup2_action.fd,
>> + action->action.dup2_action.newfd)
>> + != action->action.dup2_action.newfd)
>> + goto fail;
>> + }
>> + else
>> + {
>> + int fd = action->action.dup2_action.newfd;
>> + int flags = __fcntl (fd, F_GETFD, 0);
>> + if (flags == -1)
>> + goto fail;
>> + if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>> + goto fail;
>> + }
>> break;
>> }
>> }
>>