IPC stability bugs

Bug 1

Process spawning leaks a socket if closing the child's end fails

Status
Fixed
Symptom
File descriptor leak; worst effect seen: nothing observed
Where seen
Never observed
Trigger
Starting a child process (fork and exec)
First reported
2026-01-09 (first report)
Fixed
2026-01-13
Fix
mp#237
Shipped to Bitcoin Core
bitcoin#34363
In released binaries
Yes (v30.0-v30.3)
Defect ever on master
Yes

Summary

If SpawnProcess() failed to close the child's end of its socketpair in the parent process, it threw without closing the parent's end, leaking a file descriptor. close() on a freshly created socket essentially never fails, so this was never observed in practice. Found in code review and fixed in mp#237 ("Made SpawnProcess() behavior safe post fork()"). In Bitcoin Core releases through v30.x the only caller of SpawnProcess is the hidden echoipc test RPC.

What happened

Never observed at runtime. ryanofsky spotted it (2026-01-09) while reviewing mp#237, @Sjors's fix for a deadlock in spawned child processes: "pre-existing bug here where if close(fds[0]) fails in the parent fd[1] will not be closed either and could be leaked". Sjors fixed it in a separate commit of the same PR.

Root cause

SpawnProcess() in src/mp/util.cpp creates a socketpair, forks, and then closes one end in each process: the parent closes fds[0] (the child's end) and keeps fds[1], the child closes fds[1]. Both used one statement:

if (close(fds[pid ? 0 : 1]) != 0) {
    throw std::system_error(errno, std::system_category(), "close");
}

In the parent, a failure threw before fds[1] was returned to the caller, so nothing ever closed it. The code has had this error path since SpawnProcess was added.

The code

SpawnProcess in src/mp/util.cpp, quoted from 30a8681de626^:

int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args)
{
    int fds[2];
    if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) {
        throw std::system_error(errno, std::system_category(), "socketpair");
    }

    pid = fork();
    // ...
    // Parent process closes the descriptor for socket 0, child closes the descriptor for socket 1.
    if (close(fds[pid ? 0 : 1]) != 0) {
        throw std::system_error(errno, std::system_category(), "close");
    }
    // ...
    return fds[1];
}

In the parent, fds[1] is the caller's end of the socket and is only handed over by the final return. If closing the child's end fds[0] fails, the function throws first, and nothing else holds fds[1], so the descriptor stays open for the life of the process.

The fix (30a8681de626) closes it on that path:

@@ -128,6 +128,7 @@ int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args)
     // Parent process closes the descriptor for socket 0, child closes the descriptor for socket 1.
     if (close(fds[pid ? 0 : 1]) != 0) {
+        if (pid) (void)close(fds[1]);
         throw std::system_error(errno, std::system_category(), "close");
     }

The parent now releases both socket descriptors before throwing. The child side of this path is left alone here; a companion commit makes the child report the error and _exit instead of throwing.

Fix

The fix commit 30a8681de626 (Sjors, co-authored by ryanofsky) adds if (pid) (void)close(fds[1]); before the throw (+1). The companion commit in the same PR changes the child side of this path to write a message and _exit instead of throwing.

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern Error paths that skip cleanup or completion: A throw skips cleanup

  • Prevented by a different API shape
  • Earliest detector code review (existed then)
  • Diagnosis easy
  • Fix design easy
  • Rust partly

Would Rust have prevented it? Partly. An OwnedFd would close the socket when the error return unwinds, so the leak disappears with idiomatic types, but that is RAII the C++ could have used too; leaking a resource is not a compile error in Rust and raw fds from libc stay raw. More on Rust

Could it have been prevented?

Yes, by holding the two socketpair descriptors in an RAII owner from the moment socketpair() returns, so that any throw between creation and the final return fds[1] closes whatever has not been handed over. SpawnProcess() kept them as a raw int fds[2] and threw std::system_error from the middle of the function, which is how a failed close(fds[0]) in the parent leaked fds[1] (f324c666150b, 2019). The one-line fix (30a8681de626) closes the other end on that path by hand, and the same shape leaked again on the fork() == -1 path until mp#312 closed both ends there seven months later; a wrapper would have covered both at once.

Could it have been found sooner?

Only by reading the code, which is how it was found: ryanofsky noticed it while reviewing the post-fork rewrite of the same lines, six and a half years after they were written. close() on a fresh socket does not fail in practice, so no test or sanitizer would ever take the path, and LeakSanitizer does not track descriptors. A static checker that follows resource ownership across throw edges could flag it, but the project runs none, and the value of catching it is small.

What made it hard to debug?

Nothing; it was never observed. The only judgement was whether to fix it at all, which ryanofsky's own comment hedged ("if one close fails, not sure what chances of other one succeeding could be"), and Sjors fixed it in a separate commit in the same PR. It is in the ledger because the survey asked for leaks and borderline cases.

Lesson

When a function creates two resources and throws in between, the error path needs the same care as the success path; put the resources in an owner whose destructor releases them, and every future throw in that function is covered without anyone remembering to add a close.

Notes

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field Value
Symptom fd-leak
Worst effect observed none
Seen in never
Scope production
Trigger Starting a child process (fork-exec)
Phase any
Side spawn
Component spawn
Root cause kind resource-leak
Fix kind one-line-tweak
Detection review
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence low