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: Mon, 29 Oct 2018 16:32:34 -0300
- 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>
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;
> }
> }
>