From d4177062b3f4e7113aab9d40d994eabe711cb371 Mon Sep 17 00:00:00 2001 From: ViniciusCestarii Date: Mon, 20 Jul 2026 15:13:42 -0300 Subject: [PATCH 1/2] util: report back child error to parent and throw --- src/mp/util.cpp | 76 ++++++++++++++++++++++++++++++------ test/mp/test/spawn_tests.cpp | 18 +++++++++ 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 42cda9ce..4c05444d 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -117,9 +117,30 @@ std::string LogEscape(const kj::StringTree& string, size_t max_size) return result; } +enum class ChildErrorOp +{ + CLOSE, + EXECVP, +}; + +struct ChildError { + ChildErrorOp which; + int err; +}; + +const char* ChildErrorName(ChildErrorOp which) +{ + switch (which) { + case ChildErrorOp::CLOSE: return "close"; + case ChildErrorOp::EXECVP: return "execvp"; + } + return "unknown"; +} + std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_info_to_args) { auto fds{SocketPair()}; + auto error_fds{SocketPair()}; // Evaluate the callback and build the argv array before forking. // @@ -137,40 +158,73 @@ 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]); + throw std::system_error(err, std::system_category(), "close"); } - static constexpr char msg[] = "SpawnProcess(child): close(fds[1]) failed\n"; - const ssize_t writeResult = ::write(STDERR_FILENO, msg, sizeof(msg) - 1); + ChildError error{.which = ChildErrorOp::CLOSE, .err = err}; + const ssize_t writeResult = ::write(error_fds[0], &error, sizeof(error)); (void)writeResult; _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); } } 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"); + + ChildError error{.which = ChildErrorOp::EXECVP, .err = errno}; + const ssize_t writeResult = ::write(error_fds[0], &error, sizeof(error)); + (void)writeResult; _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]); + throw std::system_error(err, std::system_category(), "close"); + } + + ChildError error{}; + ssize_t readResult; + do { + readResult = ::read(error_fds[1], &error, sizeof(error)); + } while (readResult < 0 && errno == EINTR); + (void)close(error_fds[1]); + if (readResult < 0) { + (void)close(fds[1]); + throw std::system_error(errno, std::system_category(), "read"); + } + if (readResult > 0 && error.err) { + (void)close(fds[1]); + // Wait it here to avoid leaking a zombie process and then throw. + (void)::waitpid(pid, nullptr, 0); + throw std::system_error(error.err, std::system_category(), ChildErrorName(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 From a761acaa0897d8e5326996d58315d3d50e0f387c Mon Sep 17 00:00:00 2001 From: ViniciusCestarii Date: Tue, 21 Jul 2026 10:30:35 -0300 Subject: [PATCH 2/2] util: extract ReadSpawnResult and report read() failures as SpawnError --- src/mp/util.cpp | 47 ++++++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 4c05444d..f8344768 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -117,26 +117,41 @@ std::string LogEscape(const kj::StringTree& string, size_t max_size) return result; } -enum class ChildErrorOp +enum class SpawnErrorOp { CLOSE, EXECVP, + READ, }; -struct ChildError { - ChildErrorOp which; +struct SpawnError { + SpawnErrorOp which; int err; }; -const char* ChildErrorName(ChildErrorOp which) +const char* SpawnErrorName(SpawnErrorOp which) { switch (which) { - case ChildErrorOp::CLOSE: return "close"; - case ChildErrorOp::EXECVP: return "execvp"; + case SpawnErrorOp::CLOSE: return "close"; + case SpawnErrorOp::EXECVP: return "execvp"; + case SpawnErrorOp::READ: return "read"; } return "unknown"; } +SpawnError ReadSpawnResult(int fd) +{ + SpawnError error{}; + ssize_t readResult; + do { + readResult = ::read(fd, &error, sizeof(error)); + } while (readResult < 0 && errno == EINTR); + if (readResult < 0) { + return SpawnError{.which = SpawnErrorOp::READ, .err = errno}; + } + return error; +} + std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_info_to_args) { auto fds{SocketPair()}; @@ -176,7 +191,7 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ (void)close(error_fds[1]); throw std::system_error(err, std::system_category(), "close"); } - ChildError error{.which = ChildErrorOp::CLOSE, .err = err}; + SpawnError error{.which = SpawnErrorOp::CLOSE, .err = err}; const ssize_t writeResult = ::write(error_fds[0], &error, sizeof(error)); (void)writeResult; _exit(126); @@ -195,7 +210,7 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ execvp(argv[0], argv.data()); - ChildError error{.which = ChildErrorOp::EXECVP, .err = errno}; + SpawnError error{.which = SpawnErrorOp::EXECVP, .err = errno}; const ssize_t writeResult = ::write(error_fds[0], &error, sizeof(error)); (void)writeResult; _exit(127); @@ -209,21 +224,11 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ throw std::system_error(err, std::system_category(), "close"); } - ChildError error{}; - ssize_t readResult; - do { - readResult = ::read(error_fds[1], &error, sizeof(error)); - } while (readResult < 0 && errno == EINTR); + const SpawnError error{ReadSpawnResult(error_fds[1])}; (void)close(error_fds[1]); - if (readResult < 0) { - (void)close(fds[1]); - throw std::system_error(errno, std::system_category(), "read"); - } - if (readResult > 0 && error.err) { + if (error.err) { (void)close(fds[1]); - // Wait it here to avoid leaking a zombie process and then throw. - (void)::waitpid(pid, nullptr, 0); - throw std::system_error(error.err, std::system_category(), ChildErrorName(error.which)); + throw std::system_error(error.err, std::system_category(), SpawnErrorName(error.which)); } return {pid, fds[1]}; }