From 805334b26c7e6e83557234f2008497c72176a6cd Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella Date: Wed, 19 Sep 2018 12:14:34 -0700 Subject: [PATCH] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) 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 #23640] * 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 | 9 +++++++ posix/tst-spawn.c | 44 ++++++++++++++++++++++++++------ sysdeps/posix/spawni.c | 18 ++++++++++--- sysdeps/unix/sysv/linux/spawni.c | 18 ++++++++++--- 4 files changed, 75 insertions(+), 14 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8121c71714..f51ba49d17 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2019-01-03 Adhemerval Zanella + + [BZ #23640] + * 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. + 2019-01-03 Zack Weinberg * include/features.h (__GLIBC_USE_DEPRECATED_SCANF): New __GLIBC_USE diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c index a9fd6008fa..b45e0d885e 100644 --- a/posix/tst-spawn.c +++ b/posix/tst-spawn.c @@ -44,16 +44,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. */ @@ -67,25 +70,32 @@ 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_COMPARE (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); @@ -94,6 +104,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. */ @@ -122,6 +133,10 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s, TEST_COMPARE (read (fd1, buf, sizeof buf), strlen (fd1string)); TEST_COMPARE_BLOB (fd1string, strlen (fd1string), buf, strlen (fd1string)); + TEST_COMPARE (xlseek (fd5, 0, SEEK_SET), 0); + TEST_COMPARE (read (fd5, buf, sizeof buf), strlen (fd5string)); + TEST_COMPARE_BLOB (fd5string, strlen (fd5string), buf, strlen (fd5string)); + return 0; } @@ -137,6 +152,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; @@ -147,26 +163,30 @@ 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 + /* 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. */ xwrite (temp_fd1, fd1string, strlen (fd1string)); xwrite (temp_fd2, fd2string, strlen (fd2string)); xwrite (temp_fd3, fd3string, strlen (fd3string)); + xwrite (temp_fd5, fd5string, strlen (fd5string)); /* Close the third file. It'll be opened by `spawn'. */ xclose (temp_fd3); @@ -185,17 +205,24 @@ 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_COMPARE (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_COMPARE (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"; @@ -204,6 +231,7 @@ do_test (int argc, char *argv[]) spargv[i++] = fd3name; spargv[i++] = fd4name; spargv[i++] = name1; + spargv[i++] = fd5name; spargv[i] = NULL; TEST_COMPARE (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ), diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c index 483dc2681d..631a1798f6 100644 --- a/sysdeps/posix/spawni.c +++ b/sysdeps/posix/spawni.c @@ -204,9 +204,21 @@ __spawni_child (void *arguments) break; case spawn_do_dup2: - if (__dup2 (action->action.dup2_action.fd, - action->action.dup2_action.newfd) - != 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) + { + 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 c497869a74..353bcf5b33 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -253,9 +253,21 @@ __spawni_child (void *arguments) break; case spawn_do_dup2: - if (__dup2 (action->action.dup2_action.fd, - action->action.dup2_action.newfd) - != 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) + { + 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;