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
--
Nathan Sidwell
2018-08-21 Nathan Sidwell <nat...@acm.org>
* 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 <vfork.h>
@@ -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;
}
}
