From ac8a2ae80345fbd494b3e459014da3099d0af678 Mon Sep 17 00:00:00 2001 From: Patrick Stewart Date: Tue, 12 Sep 2017 20:35:51 +0100 Subject: [PATCH] Revert "linux: spawni.c: simplify error reporting to parent" This reverts commit 4b4d4056bb154603f36c6f8845757c1012758158. --- sysdeps/unix/sysv/linux/spawni.c | 72 ++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 25 deletions(-) diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index c56f894a82..29083e0998 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -17,7 +17,6 @@ . */ #include -#include #include #include #include @@ -45,12 +44,11 @@ 3. Child must synchronize with parent to enforce 2. and to possible return execv issues. - The first issue is solved by blocking all signals in child, even - the NPTL-internal ones (SIGCANCEL and SIGSETXID). The second and - third issue is done by a stack allocation in parent, and by using a - field in struct spawn_args where the child can write an error - code. CLONE_VFORK ensures that the parent does not run until the - child has either exec'ed successfully or exited. */ + The first issue is solved by blocking all signals in child, even the + NPTL-internal ones (SIGCANCEL and SIGSETXID). The second and third issue + is done by a stack allocation in parent and a synchronization with using + a pipe or waitpid (in case or error). The pipe has the advantage of + allowing the child the communicate an exec error. */ /* The Unix standard contains a long explanation of the way to signal @@ -78,6 +76,7 @@ struct posix_spawn_args { + int pipe[2]; sigset_t oldmask; const char *file; int (*exec) (const char *, char *const *, char *const *); @@ -87,7 +86,6 @@ struct posix_spawn_args ptrdiff_t argc; char *const *envp; int xflags; - int err; }; /* Older version requires that shell script without shebang definition @@ -124,6 +122,10 @@ __spawni_child (void *arguments) struct posix_spawn_args *args = arguments; const posix_spawnattr_t *restrict attr = args->attr; const posix_spawn_file_actions_t *file_actions = args->fa; + int p = args->pipe[1]; + int ret; + + close_not_cancel (args->pipe[0]); /* The child must ensure that no signal handler are enabled because it shared memory with parent, so the signal disposition must be either SIG_DFL or @@ -202,6 +204,17 @@ __spawni_child (void *arguments) { struct __spawn_action *action = &file_actions->__actions[cnt]; + /* Dup the pipe fd onto an unoccupied one to avoid any file + operation to clobber it. */ + if ((action->action.close_action.fd == p) + || (action->action.open_action.fd == p) + || (action->action.dup2_action.fd == p)) + { + if ((ret = __dup (p)) < 0) + goto fail; + p = ret; + } + switch (action->tag) { case spawn_do_close: @@ -268,7 +281,6 @@ __spawni_child (void *arguments) __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK) ? &attr->__ss : &args->oldmask, 0); - args->err = 0; args->exec (args->file, args->argv, args->envp); /* This is compatibility function required to enable posix_spawn run @@ -276,13 +288,14 @@ __spawni_child (void *arguments) (2.15). */ maybe_script_execute (args); + ret = -errno; + fail: - /* errno should have an appropriate non-zero value; otherwise, - there's a bug in glibc or the kernel. For lack of an error code - (EINTERNALBUG) describing that, use ECHILD. Another option would - be to set args->err to some negative sentinel and have the parent - abort(), but that seems needlessly harsh. */ - args->err = errno ? : ECHILD; + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ + ret = -ret; + if (ret) + while (write_not_cancel (p, &ret, sizeof ret) < 0) + continue; _exit (SPAWN_ERROR); } @@ -299,6 +312,9 @@ __spawnix (pid_t * pid, const char *file, struct posix_spawn_args args; int ec; + if (__pipe2 (args.pipe, O_CLOEXEC)) + return errno; + /* To avoid imposing hard limits on posix_spawn{p} the total number of arguments is first calculated to allocate a mmap to hold all possible values. */ @@ -330,16 +346,17 @@ __spawnix (pid_t * pid, const char *file, void *stack = __mmap (NULL, stack_size, prot, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); if (__glibc_unlikely (stack == MAP_FAILED)) - return errno; + { + close_not_cancel (args.pipe[0]); + close_not_cancel (args.pipe[1]); + return errno; + } /* Disable asynchronous cancellation. */ int state; __libc_ptf_call (__pthread_setcancelstate, (PTHREAD_CANCEL_DISABLE, &state), 0); - /* Child must set args.err to something non-negative - we rely on - the parent and child sharing VM. */ - args.err = -1; args.file = file; args.exec = exec; args.fa = file_actions; @@ -353,8 +370,9 @@ __spawnix (pid_t * pid, const char *file, /* The clone flags used will create a new child that will run in the same memory space (CLONE_VM) and the execution of calling thread will be - suspend until the child calls execve or _exit. - + suspend until the child calls execve or _exit. These condition as + signal below either by pipe write (_exit with SPAWN_ERROR) or + a successful execve. Also since the calling thread execution will be suspend, there is not need for CLONE_SETTLS. Although parent and child share the same TLS namespace, there will be no concurrent access for TLS variables (errno @@ -362,18 +380,22 @@ __spawnix (pid_t * pid, const char *file, new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size, CLONE_VM | CLONE_VFORK | SIGCHLD, &args); + close_not_cancel (args.pipe[1]); + if (new_pid > 0) { - ec = args.err; - assert (ec >= 0); - if (ec != 0) - __waitpid (new_pid, NULL, 0); + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) + ec = 0; + else + __waitpid (new_pid, NULL, 0); } else ec = -new_pid; __munmap (stack, stack_size); + close_not_cancel (args.pipe[0]); + if ((ec == 0) && (pid != NULL)) *pid = new_pid; -- 2.14.1