Merge #237: Made SpawnProcess() behavior safe post fork()

5205a87 test: check SpawnProcess post-fork safety (Sjors Provoost)
69652f0 Precompute argv before fork in SpawnProcess (Sjors Provoost)
30a8681 SpawnProcess: avoid fd leak on close failure (Sjors Provoost)
14e926a refactor: extract MakeArgv helper (Sjors Provoost)

Pull request description:

  Bitcoin Core functional tests for the `echoipc` RPC method were occasionally timing out, see bitcoin/bitcoin#34187. A helpful LLM quickly figured out it was due to doing unsafe things after `fork()`.

  https://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html

  Fix it by having `SpawnProcess` evaluate `fd_to_args` and build `argv` in the parent _before_ `fork()`.

  `SpawnProcess` now avoids `ExecProcess`. I intentionally did not fix the latter, not just to reduce churn, but also because it's now only used by `mpgen` and it doesn't seem worth  losing the debug errors it (unsafely) throws.

  The last commit adds a test which reproduces the issue.

ACKs for top commit:
  ryanofsky:
    Code review ACK 5205a87, just tweaking error codes and output since last review and fixing potential (pre-existing) descriptor leak on failure bug.

Tree-SHA512: 812a053c7ca1ed06e29e8f845bf62d888b44fd4b8b486ab0e735b8ebe9368aec0989ad5b094c8745c301264247351955633d147b9caf17643a54d88c2930d0cd

Author: ryanofsky

include/mp/util.h

@@ -221,13 +221,16 @@ using FdToArgsFn = std::function<std::vector<std::string>(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<std::string>& args);
 
 //! Wait for a process to exit and return its exit code.

src/mp/util.cpp

@@ -14,6 +14,7 @@
 #include <pthread.h>
 #include <sstream>
 #include <string>
+#include <sys/types.h>
 #include <sys/resource.h>
 #include <sys/socket.h>
 #include <sys/wait.h>
@@ -36,6 +37,17 @@ namespace fs = std::filesystem;
 namespace mp {
 namespace {
 
+std::vector<char*> MakeArgv(const std::vector<std::string>& args)
+{
+    std::vector<char*> argv;
+    argv.reserve(args.size() + 1);
+    for (const auto& arg : args) {
+        argv.push_back(const_cast<char*>(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<std::string> args{fd_to_args(fds[0])};
+    const std::vector<char*> 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<std::string>& args)
 {
-    std::vector<char*> argv;
-    argv.reserve(args.size());
-    for (const auto& arg : args) {
-        argv.push_back(const_cast<char*>(arg.c_str()));
-    }
-    argv.push_back(nullptr);
+    const std::vector<char*> 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<std::string>& 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;

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)

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 <mp/util.h>
+
+#include <kj/test.h>
+
+#include <chrono>
+#include <compare>
+#include <condition_variable>
+#include <csignal>
+#include <cstdlib>
+#include <mutex>
+#include <string>
+#include <sys/wait.h>
+#include <thread>
+#include <unistd.h>
+#include <vector>
+
+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<std::mutex> target_lock(target_mutex);
+        {
+            std::lock_guard<std::mutex> g(control_mutex);
+            locked = true;
+        }
+        control_cv.notify_one();
+
+        std::unique_lock<std::mutex> 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<std::mutex> 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<std::mutex> g(control_mutex);
+            release = true;
+        }
+        control_cv.notify_one();
+    });
+
+    int pid{-1};
+    const int fd{mp::SpawnProcess(pid, [&](int child_fd) -> std::vector<std::string> {
+        // 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<std::mutex> 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);
+}