Precompute argv before fork in SpawnProcess

Evaluate fd_to_args and build argv in the parent before fork().

In the child between fork() and execvp(), avoid throwing/allocating
and use _exit(1) on error.

This likely fixes the occasional deadlock in Bitcoin Core functional
test, noticed in bitcoin/bitcoin#34187.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

Author: Sjors

include/mp/util.h

@@ -221,10 +221,11 @@ 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.

src/mp/util.cpp

@@ -122,24 +122,50 @@ 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) {
-        if (pid) (void)close(fds[1]);
-        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];
 }
@@ -159,7 +185,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;