diff --git a/include/mp/util.h b/include/mp/util.h index e5b4dd1..6cd11fd 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -221,13 +221,16 @@ using FdToArgsFn = std::function(int fd)>; //! Spawn a new process that communicates with the current process over a socket //! pair. Returns pid through an output argument, and file descriptor for the -//! local side of the socket. Invokes fd_to_args callback with the remote file -//! descriptor number which returns the command line arguments that should be -//! used to execute the process, and which should have the remote file -//! descriptor embedded in whatever format the child process expects. +//! local side of the socket. +//! The fd_to_args callback is invoked in the parent process before fork(). +//! It must not rely on child pid/state, and must return the command line +//! arguments that should be used to execute the process. Embed the remote file +//! descriptor number in whatever format the child process expects. int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args); //! Call execvp with vector args. +//! Not safe to call in a post-fork child of a multi-threaded process. +//! Currently only used by mpgen at build time. void ExecProcess(const std::vector& args); //! Wait for a process to exit and return its exit code. diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 509913b..7b48608 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,17 @@ namespace fs = std::filesystem; namespace mp { namespace { +std::vector MakeArgv(const std::vector& args) +{ + std::vector argv; + argv.reserve(args.size() + 1); + for (const auto& arg : args) { + argv.push_back(const_cast(arg.c_str())); + } + argv.push_back(nullptr); + return argv; +} + //! Return highest possible file descriptor. size_t MaxFd() { @@ -111,35 +123,57 @@ int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args) throw std::system_error(errno, std::system_category(), "socketpair"); } + // Evaluate the callback and build the argv array before forking. + // + // The parent process may be multi-threaded and holding internal library + // locks at fork time. In that case, running code that allocates memory or + // takes locks in the child between fork() and exec() can deadlock + // indefinitely. Precomputing arguments in the parent avoids this. + const std::vector args{fd_to_args(fds[0])}; + const std::vector argv{MakeArgv(args)}; + pid = fork(); if (pid == -1) { throw std::system_error(errno, std::system_category(), "fork"); } - // Parent process closes the descriptor for socket 0, child closes the descriptor for socket 1. + // 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) { - throw std::system_error(errno, std::system_category(), "close"); + if (pid) { + (void)close(fds[1]); + throw std::system_error(errno, 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); + (void)writeResult; + _exit(126); } + if (!pid) { - // Child process must close all potentially open descriptors, except socket 0. + // Child process must close all potentially open descriptors, except + // 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]) { close(fd); } } - ExecProcess(fd_to_args(fds[0])); + + 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"); + _exit(127); } return fds[1]; } void ExecProcess(const std::vector& args) { - std::vector argv; - argv.reserve(args.size()); - for (const auto& arg : args) { - argv.push_back(const_cast(arg.c_str())); - } - argv.push_back(nullptr); + const std::vector argv{MakeArgv(args)}; if (execvp(argv[0], argv.data()) != 0) { perror("execvp failed"); if (errno == ENOENT && !args.empty()) { @@ -152,7 +186,7 @@ void ExecProcess(const std::vector& args) int WaitProcess(int pid) { int status; - if (::waitpid(pid, &status, 0 /* options */) != pid) { + if (::waitpid(pid, &status, /*options=*/0) != pid) { throw std::system_error(errno, std::system_category(), "waitpid"); } return status; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 2a1a7e9..1f21ba4 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -26,6 +26,7 @@ if(BUILD_TESTING AND TARGET CapnProto::kj-test) ${MP_PROXY_HDRS} mp/test/foo-types.h mp/test/foo.h + mp/test/spawn_tests.cpp mp/test/test.cpp ) include(${PROJECT_SOURCE_DIR}/cmake/TargetCapnpSources.cmake) diff --git a/test/mp/test/spawn_tests.cpp b/test/mp/test/spawn_tests.cpp new file mode 100644 index 0000000..4c7edba --- /dev/null +++ b/test/mp/test/spawn_tests.cpp @@ -0,0 +1,110 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +// Poll for child process exit using waitpid(..., WNOHANG) until the child exits +// or timeout expires. Returns true if the child exited and status_out was set. +// Returns false on timeout or error. +static bool WaitPidWithTimeout(int pid, std::chrono::milliseconds timeout, int& status_out) +{ + const auto deadline = std::chrono::steady_clock::now() + timeout; + while (std::chrono::steady_clock::now() < deadline) { + const int r = ::waitpid(pid, &status_out, WNOHANG); + if (r == pid) return true; + if (r == 0) { + std::this_thread::sleep_for(std::chrono::milliseconds{1}); + continue; + } + // waitpid error + return false; + } + return false; +} + +} // namespace + +KJ_TEST("SpawnProcess does not run callback in child") +{ + // This test is designed to fail deterministically if fd_to_args is invoked + // in the post-fork child: a mutex held by another parent thread at fork + // time appears locked forever in the child. + std::mutex target_mutex; + std::mutex control_mutex; + std::condition_variable control_cv; + bool locked{false}; + bool release{false}; + + // Holds target_mutex until the releaser thread updates release + std::thread locker([&] { + std::unique_lock target_lock(target_mutex); + { + std::lock_guard g(control_mutex); + locked = true; + } + control_cv.notify_one(); + + std::unique_lock control_lock(control_mutex); + control_cv.wait(control_lock, [&] { return release; }); + }); + + // Wait for target_mutex to be held by the locker thread. + { + std::unique_lock l(control_mutex); + control_cv.wait(l, [&] { return locked; }); + } + + // Release the lock shortly after SpawnProcess starts. + std::thread releaser([&] { + // In the unlikely event a CI machine overshoots this delay, a + // regression could be missed. This is preferable to spurious + // test failures. + std::this_thread::sleep_for(std::chrono::milliseconds{50}); + { + std::lock_guard g(control_mutex); + release = true; + } + control_cv.notify_one(); + }); + + int pid{-1}; + const int fd{mp::SpawnProcess(pid, [&](int child_fd) -> std::vector { + // If this callback runs in the post-fork child, target_mutex appears + // locked forever (the owning thread does not exist), so this deadlocks. + std::lock_guard g(target_mutex); + return {"true", std::to_string(child_fd)}; + })}; + ::close(fd); + + int status{0}; + // Give the child up to 1 second to exit. If it does not, terminate it and + // reap it to avoid leaving a zombie behind. + const bool exited{WaitPidWithTimeout(pid, std::chrono::milliseconds{1000}, status)}; + if (!exited) { + ::kill(pid, SIGKILL); + ::waitpid(pid, &status, /*options=*/0); + } + + releaser.join(); + locker.join(); + + KJ_EXPECT(exited, "Timeout waiting for child process to exit"); + KJ_EXPECT(WIFEXITED(status) && WEXITSTATUS(status) == 0); +}