util: report back child errors to parent and throw

src/mp/util.cpp

@@ -11,7 +11,9 @@
 #include <kj/common.h>
 #include <kj/debug.h>
 #include <kj/string-tree.h>
+#include <optional>
 #include <pthread.h>
+#include <csignal>
 #include <sstream>
 #include <string>
 #include <sys/types.h>
@@ -72,6 +74,102 @@ template <std::size_t N>
     _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<SpawnError> ReadSpawnResult(int fd)
+{
+    SpawnError error{};
+    char* buf = reinterpret_cast<char*>(&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<size_t>(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<const char*>(&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<size_t>(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<ProcessId, SocketId> 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<ProcessId, SocketId> 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<ProcessId, SocketId> 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<SpawnError> 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]};
 }
 

test/mp/test/spawn_tests.cpp

@@ -4,15 +4,20 @@
 
 #include <mp/util.h>
 
+#include <kj/common.h>
+#include <kj/debug.h>
 #include <kj/test.h>
 
+#include <cerrno>
 #include <chrono>
 #include <compare>
 #include <condition_variable>
 #include <csignal>
 #include <cstdlib>
 #include <mutex>
 #include <string>
+#include <string_view>
+#include <system_error>
 #include <sys/wait.h>
 #include <thread>
 #include <tuple>
@@ -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<std::string> {
+            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