| Message ID | 20180919224624.3920-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] Use libsupport for tst-spawn.c | expand |
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; > } > } >
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; >> } >> } >>
My mail client ate the original submission, so responding to your ping. On 13/12/18 2:54 AM, Adhemerval Zanella wrote: > 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. >>> BZ # on ChangeLog. >>> * 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; >>> + } It might be simpler to write this as follows to flatten out the nesting a bit. It also makes the location of the comment a lot more relevant because the clearing of the flag happens right under it: if (action->action.dup2_action.fd == action->action.dup2_action.newfd) { 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; } else if (__dup2 (action->action.dup2_action.fd, action->action.dup2_action.newfd) != action->action.dup2_action.newfd) 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; >>> } >>> } >>> Likewise. Siddhesh
* adhemerval zanella: > 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; What happened to name4 and temp_fd4? fe4string is not used, either. Thanks, Florian
On 02/01/2019 09:07, Florian Weimer wrote: > * adhemerval zanella: > >> 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; > > What happened to name4 and temp_fd4? fe4string is not used, either. > > Thanks, > Florian I used fd5 for consistency with handle_restart local fdX and do_test fdXname.
On 31/12/2018 14:34, Siddhesh Poyarekar wrote: > My mail client ate the original submission, so responding to your ping. > > On 13/12/18 2:54 AM, Adhemerval Zanella wrote: >> 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. >>>> > > BZ # on ChangeLog. Fixed. >>>> 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; >>>> + } > > It might be simpler to write this as follows to flatten out the nesting a bit. It also makes the location of the comment a lot more relevant because the clearing of the flag happens right under it: I followed your suggestion, thanks. > > if (action->action.dup2_action.fd == > action->action.dup2_action.newfd) > { > 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; > } > else if (__dup2 (action->action.dup2_action.fd, > action->action.dup2_action.newfd) > != action->action.dup2_action.newfd) > 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; >>>> } >>>> } >>>> > > Likewise. > > Siddhesh Pushed as 805334b26c.
On Thu, Jan 03 2019, Adhemerval Zanella wrote:
> Pushed as 805334b26c.
After this commit, tst-spawn and tst-spawn-static are failing for me,
with the following error messages:
tst-spawn.c:128: numeric comparison failure
left: 29 (0x1d); from: errno
right: 9 (0x9); from: EBADF
error: 1 test failures
tst-spawn.c:128: numeric comparison failure
left: 29 (0x1d); from: errno
right: 9 (0x9); from: EBADF
error: 1 test failures
tst-spawn.c:252: numeric comparison failure
left: 1 (0x1); from: WEXITSTATUS (status)
right: 0 (0x0); from: 0
tst-spawn.c:257: numeric comparison failure
left: 1 (0x1); from: WEXITSTATUS (status)
right: 0 (0x0); from: 0
error: 2 test failures
And it's weird, because tst-spawn.c:128 is not a test for the newly
added fd5, it's an old test for fd1.
Have you seen similar results somewhere else?
On Sep 19 2018, adhemerval.zanella@linaro.org wrote: > @@ -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 s/dile/file/ > @@ -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; This overruns spargv. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
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; } }
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(-) -- 2.17.1