Message ID | f73cfb4a-aa7f-44fc-3a53-66053263fbc6@acm.org |
---|---|
State | New |
Headers | show |
Series | [libiberty] pex-unix error behaviour | expand |
On Tue, Aug 21, 2018 at 8:03 AM, Nathan Sidwell <nathan@acm.org> wrote: > > 1) If we know we're using vfork, we can tell the parent directly via the > current stack frame and suitable use of volatiles. I'm pretty reluctant to rely on this special behavior of vfork. A system could plausibly #define vfork fork and almost all programs would continue to work, but not this one. I think we would need a really compelling advantage to start doing handling vfork specially. But, since errors in this code are essentially non-existent, I don't see a compelling advantage here. Is there some larger scheme this is in aid of? Ian
On 08/21/2018 12:37 PM, Ian Lance Taylor wrote: > On Tue, Aug 21, 2018 at 8:03 AM, Nathan Sidwell <nathan@acm.org> wrote: >> >> 1) If we know we're using vfork, we can tell the parent directly via the >> current stack frame and suitable use of volatiles. > > I'm pretty reluctant to rely on this special behavior of vfork. A > system could plausibly #define vfork fork and almost all programs if '#define vfork fork' is in effect, we already consider it not vfork: #ifdef vfork /* Autoconf may define this to fork for us. */ # define VFORK_STRING "fork" #else # define VFORK_STRING "vfork" #endif The patch continues to use that distinction. As I mentioned, my manual testing (by adding such a #define), behaved as expected -- the existing behaviour. > would continue to work, but not this one. I think we would need a > really compelling advantage to start doing handling vfork specially. > But, since errors in this code are essentially non-existent, I don't > see a compelling advantage here. Is there some larger scheme this is > in aid of? On the modules branch the user can provide a mapper program, which we then communicate with. So we're now execing something user-controlled, not part of our configuration. The error behaviour if that program fails to be exec'd is confusing -- there's an error about the exec failing but it's not attached to location information and the like, then there's a much more obvious error about communication failing. I found it confusing the first time I tripped over it (and I was writing that bit of the compiler :) nathan
On Tue, Aug 21, 2018 at 11:15 AM, Nathan Sidwell <nathan@acm.org> wrote: > On 08/21/2018 12:37 PM, Ian Lance Taylor wrote: >> >> On Tue, Aug 21, 2018 at 8:03 AM, Nathan Sidwell <nathan@acm.org> wrote: >>> >>> >>> 1) If we know we're using vfork, we can tell the parent directly via the >>> current stack frame and suitable use of volatiles. >> >> >> I'm pretty reluctant to rely on this special behavior of vfork. A >> system could plausibly #define vfork fork and almost all programs > > > if '#define vfork fork' is in effect, we already consider it not vfork: > > #ifdef vfork /* Autoconf may define this to fork for us. */ > # define VFORK_STRING "fork" > #else > # define VFORK_STRING "vfork" > #endif > > The patch continues to use that distinction. As I mentioned, my manual > testing (by adding such a #define), behaved as expected -- the existing > behaviour. OK, but what about a system that does int vfork() { return fork(); } ? I'm certainly willing to believe that your patch works reliably on GNU/Linux, but this code has to be extremely portable. >> would continue to work, but not this one. I think we would need a >> really compelling advantage to start doing handling vfork specially. >> But, since errors in this code are essentially non-existent, I don't >> see a compelling advantage here. Is there some larger scheme this is >> in aid of? > > > On the modules branch the user can provide a mapper program, which we then > communicate with. So we're now execing something user-controlled, not part > of our configuration. > > The error behaviour if that program fails to be exec'd is confusing -- > there's an error about the exec failing but it's not attached to location > information and the like, then there's a much more obvious error about > communication failing. I found it confusing the first time I tripped over > it (and I was writing that bit of the compiler :) Is there any reason we couldn't fix that without relying on the odd behavior of vfork? Ian
On 08/21/2018 03:04 PM, Ian Lance Taylor wrote: > On Tue, Aug 21, 2018 at 11:15 AM, Nathan Sidwell <nathan@acm.org> wrote: > OK, but what about a system that does > > int vfork() { return fork(); } > > ? Isn't such a system just buggy? Hm, apparently not. Because of the requirement the user just calls 'exec(), _exit ()' a conformant program cannot tell whether it's fork-like or not. However we're already violating that constraint by dinking environ and calling close & write [in ways that are ok regardless of a fork-like attribute]. I see the man page says 'The requirements put on vfork() by the standards are weaker than those put on fork(2), so an implementation where the two are synonymous is compliant. In particular, the programmer cannot rely on the parent remaining blocked until the child either terminates or calls execve(2), and cannot rely on any specific behavior with respect to shared memory.' pants. I suppose we could add an autoconf test to probe whether the child and parent are serialized or not. that may or may not be simpler than ... >> The error behaviour if that program fails to be exec'd is confusing -- >> there's an error about the exec failing but it's not attached to location >> information and the like, then there's a much more obvious error about >> communication failing. I found it confusing the first time I tripped over >> it (and I was writing that bit of the compiler :) > > Is there any reason we couldn't fix that without relying on the odd > behavior of vfork? Maybe, ideas? (but this seemed the simplest way for me to get it to do what I wanted :) nathan
On Tue, Aug 21, 2018 at 12:46 PM, Nathan Sidwell <nathan@acm.org> wrote: > On 08/21/2018 03:04 PM, Ian Lance Taylor wrote: >> >> On Tue, Aug 21, 2018 at 11:15 AM, Nathan Sidwell <nathan@acm.org> wrote: > > >> OK, but what about a system that does >> >> int vfork() { return fork(); } >> >> ? > > > Isn't such a system just buggy? Hm, apparently not. Because of the > requirement the user just calls 'exec(), _exit ()' a conformant program > cannot tell whether it's fork-like or not. However we're already violating > that constraint by dinking environ and calling close & write [in ways that > are ok regardless of a fork-like attribute]. I see the man page says 'The > requirements put on vfork() by the standards are weaker than those put on > fork(2), so an implementation where the two are synonymous is compliant. In > particular, the programmer cannot rely on the parent remaining blocked until > the child either terminates or calls execve(2), and cannot rely on any > specific behavior with respect to shared memory.' > > pants. I suppose we could add an autoconf test to probe whether the child > and parent are serialized or not. that may or may not be simpler than ... > >>> The error behaviour if that program fails to be exec'd is confusing -- >>> there's an error about the exec failing but it's not attached to location >>> information and the like, then there's a much more obvious error about >>> communication failing. I found it confusing the first time I tripped >>> over >>> it (and I was writing that bit of the compiler :) >> >> >> Is there any reason we couldn't fix that without relying on the odd >> behavior of vfork? > > > Maybe, ideas? (but this seemed the simplest way for me to get it to do what > I wanted :) The usual technique for better error handling is to open a pipe before the fork, mark the pipe as close-on-exec, and have the child write error information to the pipe. The parent can then read from the pipe; if it reads nothing, then the pipe was closed by the exec and all is well. Ian
2018-08-21 Nathan Sidwell <nathan@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; } }