From patchwork Thu Sep 1 09:28:20 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 15150 Received: (qmail 4127 invoked by alias); 1 Sep 2016 09:28:33 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 3487 invoked by uid 89); 1 Sep 2016 09:28:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS, URIBL_RED autolearn=no version=3.3.2 spammy=H*F:D*dk, fds, disposition, cnt X-HELO: mail-ua0-f177.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Qm5BPF1KnDglef/18fIvpaZkVuSAd46etFXfbbT6DZk=; b=k8JTTxva1WFFmPLCx3PLFvYbP8vmFCwFDnuPiPqcjaMLerr+uYDwnJ1Sz7sjWjVdwP WNgKNy8Fn+kbuLkAoLlVR8FCxzJULMhszZjtns+VLuumZ7faO0/Q9Mko0409Xh/ok7HT 5hierff6X2h183NfMTSIjVMEzIL1kO4L7utLFh8CBLjqllEq3D7HMv2Bmm5ZaKeaNi/4 ekuNXRIyryXwI3TzsN0OiPgta7CyrTaipqu4FfIcuc3qBhXLy9yZ7O+tFSLJO8jhLBCy jDsj9axA1vU8rzN1NbTUXtxlVSCP8Ir6OZw2VbE5aonSlJ6EQT84ImLBc5YHK9jJxohn 9N1A== X-Gm-Message-State: AE9vXwPeGeB98y1PqVhV0ao9UkaWeEi7A1SoEojbXqMKXPCUlw5626ppz24aEh3qFs2iPJBQeaH1B4yt5/JSVw== X-Received: by 10.176.1.167 with SMTP id 36mr8475292ual.140.1472722100639; Thu, 01 Sep 2016 02:28:20 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1454343665-1706-1-git-send-email-adhemerval.zanella@linaro.org> <1454343665-1706-4-git-send-email-adhemerval.zanella@linaro.org> <87mvjsprqb.fsf@rasmusvillemoes.dk> From: Rasmus Villemoes Date: Thu, 1 Sep 2016 11:28:20 +0200 Message-ID: Subject: Re: [PATCH v2 3/3] posix: New Linux posix_spawn{p} implementation To: Joseph Myers Cc: libc-alpha@sourceware.org, Adhemerval Zanella On 1 September 2016 at 00:08, Joseph Myers wrote: > On Wed, 31 Aug 2016, Rasmus Villemoes wrote: > >> Rather late to the party, but I think there's a few bugs here. Most >> importantly, dup() doesn't preserve the CLOEXEC flag, so if we do move >> the write end around like this, the fd will not automatically be closed >> during the exec, and hence the parent won't receive EOF and will block >> in read() call until the child finally exits. That's easily fixable with >> fcntl(p, F_DUPFD_CLOEXEC, 0). Pretty annoying to add a test for, though. > > In Linux-specific code we can assume the presence of dup3 (which needs to > be called by the name __dup3 in implementations of POSIX functions). What I meant was that it is a little hard to write a regression test for this bug, since we don't know beforehand what fds the pipe2() call will give us, making it hard to create actions that is guaranteed to exercise this code. But, thinking a bit more about this, why do we even need a pipe to ensure the child is gone, when we already set CLONE_VFORK? Can't we just exploit the fact that we run in the same VM as the parent and make the child write a non-zero error code to the spawn_args structure? That would eliminate this problem entirely. Something like below (sorry if gmail has whitespace-damaged it). Rasmus From: Rasmus Villemoes Date: Thu, 1 Sep 2016 11:03:43 +0200 Subject: [PATCH] linux: spawni.c: simplify error reporting to parent Using VFORK already ensures that the parent does not run until the child has either exec'ed succesfully or called _exit. Hence we don't need to read from a CLOEXEC pipe to ensure proper synchronization - we just make explicit use of the fact the the child and parent run in the same VM, so the child can write an error code to a field of the posix_spawn_args struct instead of sending it through a pipe. This eliminates some annoying bookkeeping that is necessary to avoid the file actions from clobbering the write end of the pipe, and getting rid of the pipe creation in the first place means fewer system calls and fewer chanches for the spawn to fail (e.g. if we're close to EMFILE). Signed-off-by: Rasmus Villemoes --- sysdeps/unix/sysv/linux/spawni.c | 63 ++++++++++++---------------------------- 1 file changed, 18 insertions(+), 45 deletions(-) diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index bb3eecf..a3c4175 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -44,11 +44,12 @@ 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 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 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 Unix standard contains a long explanation of the way to signal @@ -79,7 +80,6 @@ struct posix_spawn_args { - int pipe[2]; sigset_t oldmask; const char *file; int (*exec) (const char *, char *const *, char *const *); @@ -89,6 +89,7 @@ struct posix_spawn_args ptrdiff_t argc; char *const *envp; int xflags; + int err; }; /* Older version requires that shell script without shebang definition @@ -125,11 +126,8 @@ __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 SIG_IGN. It does by iterating over all signals and although it could @@ -203,17 +201,6 @@ __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: @@ -280,14 +267,12 @@ __spawni_child (void *arguments) (2.15). */ maybe_script_execute (args); - ret = -errno; - fail: - /* Since sizeof errno < PIPE_BUF, the write is atomic. */ - ret = -ret; - if (ret) - while (write_not_cancel (p, &ret, sizeof ret) < 0) - continue; + /* errno should have an appropriate non-zero value, but make sure + that's the case so that our parent knows we failed to + exec. There's no EUNKNOWN or EINTERNALBUG, so we use a value + which is clearly bogus. */ + args->err = errno ? : EHOSTDOWN; _exit (SPAWN_ERROR); } @@ -304,9 +289,6 @@ __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. */ @@ -333,15 +315,12 @@ __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)) - { - close_not_cancel (args.pipe[0]); - close_not_cancel (args.pipe[1]); - return errno; - } + return errno; /* Disable asynchronous cancellation. */ int cs = LIBC_CANCEL_ASYNC (); + args.err = 0; args.file = file; args.exec = exec; args.fa = file_actions; @@ -355,9 +334,8 @@ __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. These condition as - signal below either by pipe write (_exit with SPAWN_ERROR) or - a successful execve. + suspend until the child calls execve or _exit. + 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 @@ -365,13 +343,10 @@ __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) { - if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) - ec = 0; - else + ec = args.err; + if (ec != 0) __waitpid (new_pid, NULL, 0); } else @@ -379,8 +354,6 @@ __spawnix (pid_t * pid, const char *file, __munmap (stack, stack_size); - close_not_cancel (args.pipe[0]); - if ((ec == 0) && (pid != NULL)) *pid = new_pid;