diff --git a/src/mp/util.cpp b/src/mp/util.cpp index f524ab23..96bd344e 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -11,7 +11,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -72,6 +74,102 @@ template _exit(126); } +enum class SpawnErrorOp +{ + CLOSE, + EXECVP, + READ, + FCNTL, +}; + +struct SpawnError { + SpawnErrorOp which; + int err; +}; + +const char* SpawnErrorName(SpawnErrorOp which) +{ + switch (which) { + case SpawnErrorOp::CLOSE: return "close"; + case SpawnErrorOp::EXECVP: return "execvp"; + case SpawnErrorOp::READ: return "read"; + case SpawnErrorOp::FCNTL: return "fcntl"; + } + return "unknown"; +} + +// Read the child's error report. Returns nullopt on success, or a SpawnError to +// throw on failure. Success is a clean EOF: read() returns 0 with nothing +// buffered because the child's write end was closed by a successful exec (via +// FD_CLOEXEC). A fully-read struct is the failure the child reported. A read() +// error, or an EOF partway through the struct (the child died mid-report), is +// surfaced as a SpawnErrorOp::READ failure rather than being mistaken for +// success. A single read() is not guaranteed to return all sizeof(SpawnError) +// bytes (the socket is SOCK_STREAM, which has no message boundaries) and may be +// interrupted by a signal, so loop until the whole struct is read. +// +// Note that a clean EOF is also what happens if the child is killed before it +// reaches exec, so this function reports success in that case. There is no good +// way to distinguish the two here, but callers will still see the failure when +// they call WaitProcess and get the child's exit status. +std::optional ReadSpawnResult(int fd) +{ + SpawnError error{}; + char* buf = reinterpret_cast(&error); + size_t remaining = sizeof(error); + while (remaining > 0) { + const ssize_t n = ::read(fd, buf, remaining); + if (n < 0) { + if (errno == EINTR) continue; + return SpawnError{.which = SpawnErrorOp::READ, .err = errno}; + } + if (n == 0) { + if (remaining == sizeof(error)) return std::nullopt; // clean EOF: success + return SpawnError{.which = SpawnErrorOp::READ, .err = EPROTO}; // torn report + } + buf += n; + remaining -= static_cast(n); + } + return error; +} + +// Write the whole SpawnError to fd, retrying short writes and EINTR so the +// parent never sees a torn struct. Runs in the post-fork child, so it must stay +// async-signal-safe: it only calls write() and does not allocate or throw. This +// is best-effort -- if the write cannot complete there is nothing useful the +// child can do, so it stops and lets the caller _exit(). +void WriteSpawnError(int fd, const SpawnError& error) +{ + const char* buf = reinterpret_cast(&error); + size_t remaining = sizeof(error); + while (remaining > 0) { + const ssize_t n = ::write(fd, buf, remaining); + if (n < 0) { + if (errno == EINTR) continue; + break; + } + buf += n; + remaining -= static_cast(n); + } + if (remaining > 0) { + // The parent's read end is gone (e.g. the parent exited before the + // child could report), so the structured error can't be delivered. + // Leave a breadcrumb on stderr and exit. The exit code is irrelevant + // here since no live parent remains to wait on it. + ChildFail("SpawnProcess(child): failed and could not report error to parent\n"); + } +} + +// Get rid of a child process the parent is abandoning because SpawnProcess is +// about to throw, so it is not left behind as a zombie. The child may still be +// alive, so kill it first: waiting without that could block for as long as the +// spawned program runs. +void KillAndReapChild(ProcessId pid) +{ + (void)::kill(pid, SIGKILL); + while (::waitpid(pid, /*status=*/nullptr, /*options=*/0) == -1 && errno == EINTR) {} +} + } // namespace std::string ThreadName(const char* exe_name) @@ -132,6 +230,9 @@ std::string LogEscape(const kj::StringTree& string, size_t max_size) std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_info_to_args) { auto fds{SocketPair()}; + // Only used for the child to report errors back to the parent, so a one-way + // pipe would be sufficient, but reuse the existing SocketPair() helper. + auto error_fds{SocketPair()}; // Evaluate the callback and build the argv array before forking. // @@ -144,25 +245,36 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ ProcessId pid = fork(); if (pid == -1) { - throw std::system_error(errno, std::system_category(), "fork"); + const int err = errno; + (void)close(fds[0]); + (void)close(fds[1]); + (void)close(error_fds[0]); + (void)close(error_fds[1]); + throw std::system_error(err, std::system_category(), "fork"); } // Parent process closes the descriptor for socket 0, child closes the // descriptor for socket 1. On failure, the parent throws, but the child // must _exit(126) (post-fork child must not throw). if (close(fds[pid ? 0 : 1]) != 0) { + const int err = errno; if (pid) { (void)close(fds[1]); - throw std::system_error(errno, std::system_category(), "close"); + (void)close(error_fds[0]); + (void)close(error_fds[1]); + KillAndReapChild(pid); + throw std::system_error(err, std::system_category(), "close"); } - ChildFail("SpawnProcess(child): close(fds[1]) failed\n"); + WriteSpawnError(error_fds[0], {.which = SpawnErrorOp::CLOSE, .err = err}); + _exit(126); } if (!pid) { // Child process must close all potentially open descriptors, except - // socket 0. Do not throw, allocate, or do non-fork-safe work here. + // socket 0 and the error-reporting socket 0. Do not throw, allocate, or + // do non-fork-safe work here. const int maxFd = MaxFd(); for (int fd = 3; fd < maxFd; ++fd) { - if (fd != fds[0]) { + if (fd != fds[0] && fd != error_fds[0]) { close(fd); } } @@ -174,17 +286,34 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ // fcntl is async-signal-safe. const int fds0_flags = fcntl(fds[0], F_GETFD); if (fds0_flags == -1 || fcntl(fds[0], F_SETFD, fds0_flags & ~FD_CLOEXEC) == -1) { - ChildFail("SpawnProcess(child): clearing FD_CLOEXEC failed\n"); + const int err = errno; + WriteSpawnError(error_fds[0], {.which = SpawnErrorOp::FCNTL, .err = err}); + _exit(126); } execvp(argv[0], argv.data()); - // NOTE: perror() is not async-signal-safe; calling it here in a - // post-fork child may deadlock in multithreaded parents. - // TODO: Report errors to the parent via a pipe (e.g. write errno) - // so callers can get diagnostics without relying on perror(). - perror("execvp failed"); + + const int err = errno; + WriteSpawnError(error_fds[0], {.which = SpawnErrorOp::EXECVP, .err = err}); _exit(127); } + + // Close the parent's copy of the child's write end. + if (close(error_fds[0]) != 0) { + const int err = errno; + (void)close(error_fds[1]); + (void)close(fds[1]); + KillAndReapChild(pid); + throw std::system_error(err, std::system_category(), "close"); + } + + const std::optional error{ReadSpawnResult(error_fds[1])}; + (void)close(error_fds[1]); + if (error) { + (void)close(fds[1]); + KillAndReapChild(pid); + throw std::system_error(error->err, std::system_category(), SpawnErrorName(error->which)); + } return {pid, fds[1]}; } diff --git a/test/mp/test/spawn_tests.cpp b/test/mp/test/spawn_tests.cpp index bdc9ef54..5f991275 100644 --- a/test/mp/test/spawn_tests.cpp +++ b/test/mp/test/spawn_tests.cpp @@ -4,8 +4,11 @@ #include +#include +#include #include +#include #include #include #include @@ -13,6 +16,8 @@ #include #include #include +#include +#include #include #include #include @@ -113,5 +118,18 @@ KJ_TEST("SpawnProcess does not run callback in child") KJ_EXPECT(exited, "Timeout waiting for child process to exit"); KJ_EXPECT(WIFEXITED(status) && WEXITSTATUS(status) == 0); } + +KJ_TEST("SpawnProcess throws on execvp failure") +{ + try { + SpawnProcess([&](SpawnConnectInfo) -> std::vector { + return {"/nonexistent/binary"}; + }); + KJ_EXPECT(false, "expected SpawnProcess to throw"); + } catch (const std::system_error& e) { + KJ_EXPECT(e.code().value() == ENOENT); + KJ_EXPECT(std::string_view{e.what()}.find("execvp") != std::string_view::npos); + } +} } // namespace test } // namespace mp