From patchwork Tue Aug 21 15:03:07 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathan Sidwell X-Patchwork-Id: 960495 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-484098-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=acm.org Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="TFQBeJMV"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41vv8j5bYQz9s4V for ; Wed, 22 Aug 2018 01:04:21 +1000 (AEST) Received: (qmail 121506 invoked by alias); 21 Aug 2018 15:03:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 121326 invoked by uid 89); 21 Aug 2018 15:03:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-9.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=HX-Received:a81, configurations X-HELO: mail-yw1-f48.google.com Received: from mail-yw1-f48.google.com (HELO mail-yw1-f48.google.com) (209.85.161.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 21 Aug 2018 15:03:13 +0000 Received: by mail-yw1-f48.google.com with SMTP id 14-v6so805143ywe.2 for ; Tue, 21 Aug 2018 08:03:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:to:from:subject:message-id:date:user-agent:mime-version :content-language; bh=/1Ytj9mxjQ9Ai72nIFYYmL/fLDIoGtdFWFuQV8dh2ts=; b=TFQBeJMV5TL+Kfdj9R8yuCKL1rDD7PjDd8Vf+GVSI1ASzM4j7Rkqtr4FR1E7InclXH a1Mj0EEBVHmXk0EWmy8h3rRq1AVVV1Eef+7kChYGMTXrQHqmbmYM3duW/Pf9EN1maR6v IddJxmy1lIHoYzaf/fH4acwfCZj3ws44FeU6WuOgm0OYJhCTwKoMQBTK5dPdKbJKi0KO nYbTg7zn+sK0OYjNsnEYobrjR9MRfYIsxLUaLXG0lOHigQ6q93zwI1pPftcGw7vLNQ20 BITyBHYs1qfbEet79pR64gJ61moWvUWav/Seh4oPDXcZXWiilW/2zPjbNPJ3u+5TCFht lEwQ== Received: from ?IPv6:2620:10d:c0a3:20fb:7500:e7fb:4a6f:2254? ([2620:10d:c091:200::5c29]) by smtp.googlemail.com with ESMTPSA id 126-v6sm7767627ywl.48.2018.08.21.08.03.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Aug 2018 08:03:09 -0700 (PDT) Sender: Nathan Sidwell To: GCC Patches , Ian Lance Taylor From: Nathan Sidwell Subject: [libiberty patch] pex-unix error behaviour Message-ID: Date: Tue, 21 Aug 2018 11:03:07 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 This is the second patch for pex-unix's child spawning. Right now, if we manage to (v)fork, but the execvp fails, we use write to emit an error and then _exit. The parent discovers the child failed when trying to communicate or wait for it. This can be improved in 2 ways. 1) If we know we're using vfork, we can tell the parent directly via the current stack frame and suitable use of volatiles. The parent blocks until the child exits (via _exit or successful execvp). We can then look where the child will have written error information and use that. The error looks like: class-1_a.C:3:15: error: failed execvp of mapper '|nope': No such file or directory 3 | export module One; | ^~~ (this is coming from the module code) 2) Otherwise, if we're using fork we keep the old behaviour, but we can use stdio rather than write directly. The error looks like: cc1plus: error trying to exec 'nope': execvp: No such file or directory class-1_a.C:3:15: error: unexpected close from mapper '|nope' 3 | export module One; | ^~~ class-1_a.C:3:15: error: mapper '|nope' exit status 255 (the cc1plus error is that coming from pex-unix, the later diagnostic is when the parent discovers something's wrong trying to communicate). I had to change the indentation more than expected, to avoid a maybe clobbered warning about the (non-volatile) bad_fn. If you'd like that indentation altered separately, let me know. booted and tested on x86_64-linux. I manually tested both the fork and vfork configurations with a non-existent child executable as shown above. ok? nathan 2018-08-21 Nathan Sidwell * pex-unix.c (VFORK_STRING): Replace this string with ... (VFORK_IS_FORK): ... this boolean. (pex_unix_exec_child): Communicate from child to parent when using vfork. Use stdio for error when using fork. Index: pex-unix.c =================================================================== --- pex-unix.c (revision 263701) +++ pex-unix.c (working copy) @@ -60,9 +60,9 @@ extern int errno; #endif #ifdef vfork /* Autoconf may define this to fork for us. */ -# define VFORK_STRING "fork" +# define VFORK_IS_FORK 1 #else -# define VFORK_STRING "vfork" +# define VFORK_IS_FORK 0 #endif #ifdef HAVE_VFORK_H #include @@ -579,10 +579,17 @@ pex_unix_exec_child (struct pex_obj *obj This clobbers the parent's environ so we need to restore it. It would be nice to use one of the exec* functions that takes an environment as a parameter, but that may have portability - issues. */ - char **save_environ = environ; - - const char *bad_fn = NULL; + issues. It is marked volatile so the child doesn't consider it a + dead variable and therefore clobber where ever it is stored. */ + char **volatile save_environ = environ; + + /* If we are using a true vfork, these allow the child to convey an + error to the parent immediately -- rather than discover it later + when trying to communicate. Notice we're already in undefined + territory by not immediately calling execv[p] or _exit in the + child. */ + volatile int child_errno = 0; + const char *volatile child_bad_fn = NULL; for (retries = 0; retries < 4; ++retries) { @@ -595,113 +602,127 @@ pex_unix_exec_child (struct pex_obj *obj switch (pid) { - case -1: - *err = errno; - *errmsg = VFORK_STRING; - return (pid_t) -1; - case 0: /* Child process. */ - if (!bad_fn && in != STDIN_FILE_NO) - { - if (dup2 (in, STDIN_FILE_NO) < 0) - bad_fn = "dup2"; - else if (close (in) < 0) - bad_fn = "close"; - } - if (!bad_fn && out != STDOUT_FILE_NO) - { - if (dup2 (out, STDOUT_FILE_NO) < 0) - bad_fn = "dup2"; - else if (close (out) < 0) - bad_fn = "close"; - } - if (!bad_fn && errdes != STDERR_FILE_NO) - { - if (dup2 (errdes, STDERR_FILE_NO) < 0) - bad_fn = "dup2"; - else if (close (errdes) < 0) - bad_fn = "close"; - } - if (!bad_fn && toclose >= 0) - { - if (close (toclose) < 0) - bad_fn = "close"; - } - if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0) - { - if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0) - bad_fn = "dup2"; - } - if (!bad_fn) - { - if (env) - /* NOTE: In a standard vfork implementation this clobbers - the parent's copy of environ "too" (in reality there's - only one copy). This is ok as we restore it below. */ - environ = (char**) env; - if ((flags & PEX_SEARCH) != 0) - { - execvp (executable, to_ptr32 (argv)); - bad_fn = "execvp"; - } - else - { - execv (executable, to_ptr32 (argv)); - bad_fn = "execv"; - } - } - - /* Something failed, report an error. We don't use stdio - routines, because we might be here due to a vfork call. */ { - ssize_t retval = 0; - int err = errno; + const char *bad_fn = NULL; + if (!bad_fn && in != STDIN_FILE_NO) + { + if (dup2 (in, STDIN_FILE_NO) < 0) + bad_fn = "dup2"; + else if (close (in) < 0) + bad_fn = "close"; + } + if (!bad_fn && out != STDOUT_FILE_NO) + { + if (dup2 (out, STDOUT_FILE_NO) < 0) + bad_fn = "dup2"; + else if (close (out) < 0) + bad_fn = "close"; + } + if (!bad_fn && errdes != STDERR_FILE_NO) + { + if (dup2 (errdes, STDERR_FILE_NO) < 0) + bad_fn = "dup2"; + else if (close (errdes) < 0) + bad_fn = "close"; + } + if (!bad_fn && toclose >= 0) + { + if (close (toclose) < 0) + bad_fn = "close"; + } + if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0) + { + if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0) + bad_fn = "dup2"; + } + if (!bad_fn) + { + if (env) + /* NOTE: In a standard vfork implementation this clobbers + the parent's copy of environ "too" (in reality there's + only one copy). This is ok as we restore it below. */ + environ = (char**) env; + if ((flags & PEX_SEARCH) != 0) + { + execvp (executable, to_ptr32 (argv)); + bad_fn = "execvp"; + } + else + { + execv (executable, to_ptr32 (argv)); + bad_fn = "execv"; + } + } + + /* Something failed. */ + int retval = -1; + if (VFORK_IS_FORK) + { + /* We really forked. We cannot tell the parent the error, + but we can use stdio. */ + if (fprintf (stderr, "%s: error trying to exec '%s': %s: %s\n", + obj->pname, executable, bad_fn, xstrerror (errno)) < 0 + || fflush (stderr) != 0) + retval = -2; + } + else + { + /* We used vfork, therefore running in the same address + space as the parent. Tell it we failed. */ + child_bad_fn = bad_fn; + child_errno = errno; + } -#define writeerr(s) (retval |= write (STDERR_FILE_NO, s, strlen (s))) - writeerr (obj->pname); - writeerr (": error trying to exec '"); - writeerr (executable); - writeerr ("': "); - writeerr (bad_fn); - writeerr (": "); - writeerr (xstrerror (err)); - writeerr ("\n"); -#undef writeerr - - /* Exit with -2 if the error output failed, too. */ - _exit (retval < 0 ? -2 : -1); + _exit (retval); } /* NOTREACHED */ return (pid_t) -1; + case -1: + /* Failed to (v)fork. */ + child_bad_fn = VFORK_IS_FORK ? "fork" : "vfork"; + /* Don't need to save errno. */ + /* FALLTHROUGH */ + default: /* Parent process. */ + { + /* Restore environ. + Note that the parent either doesn't run until the child execs/exits + (standard vfork behaviour), or if it does run then vfork is behaving + more like fork. In either case we needn't worry about clobbering + the child's copy of environ. */ + environ = save_environ; + + const char *bad_fn = child_bad_fn; + if (!VFORK_IS_FORK) + { + int e = child_errno; + if (e) + /* The child managed to give us an error, before failing to + start. Use that. */ + errno = e; + } - /* Restore environ. - Note that the parent either doesn't run until the child execs/exits - (standard vfork behaviour), or if it does run then vfork is behaving - more like fork. In either case we needn't worry about clobbering - the child's copy of environ. */ - environ = save_environ; - - if (!bad_fn && in != STDIN_FILE_NO) - if (close (in) < 0) - bad_fn = "close"; - if (!bad_fn && out != STDOUT_FILE_NO) - if (close (out) < 0) - bad_fn = "close"; - if (!bad_fn && errdes != STDERR_FILE_NO) - if (close (errdes) < 0) - bad_fn = "close"; - - if (bad_fn) - { - *err = errno; - *errmsg = bad_fn; - return (pid_t) -1; - } + if (!bad_fn && in != STDIN_FILE_NO) + if (close (in) < 0) + bad_fn = "close"; + if (!bad_fn && out != STDOUT_FILE_NO) + if (close (out) < 0) + bad_fn = "close"; + if (!bad_fn && errdes != STDERR_FILE_NO) + if (close (errdes) < 0) + bad_fn = "close"; + if (bad_fn) + { + *err = errno; + *errmsg = bad_fn; + return (pid_t) -1; + } + } return pid; } }