bugfix: clear FD_CLOEXEC in child instead of parent before fork #311

Sjors commented at 12:16 PM on July 20, 2026: member

Commit 652934fb793743ab1bf962b90c7a7d6f6681c199 in #274 sets FD_CLOEXEC in order "to ensure sockets are not leaked if processes are spawned". However it's cleared too early, before forking. This PR clears it after forking, but still before exec.

The first commit adds a helper for signal-safely emitting an error message, since the second commit also needs this.

The second commit and a code comment explain why it's unsafe to clear FD_CLOEXEC before fork. ryanofsky also described it:

As I understand it, this race has always existed, and was not introduced in 652934f. What 652934f did was start using FD_CLOEXEC which narrowed the race window, without completely closing it. This followup PR fixes the race more completely, but there is still a small race between creating the socketpair and applying the cause the CLOEXEC flags.

The race happens when a SpawnProcess call happens at the same time as a separate fork call in unrelated thread not using SpawnProcess. Because SpawnProcess creates a socket pair, if a separate fork happens in another thread, it could inherit the socket pair file descriptors and keep them open too long if it is not looping over them and closing them like SpawnProcess is. As I understand it this could result in socketpair connections staying open even after the SpawnProcess parent or child have closed them, so onDisconnect events might not be triggered, and resources might not be freed.

A test in https://github.com/Sjors/libmultiprocess/commit/1e1ff0397bf26b69c0c4b0dfffac4106490f482c demonstrates the issue, but is not included in the PR to keep things simple.

This should not be an actual problem in the way Bitcoin Core uses libmultiprocess today, but it may be with Windows and/or multiple connection support.

DrahtBot commented at 12:16 PM on July 20, 2026: none

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ryanofsky, ViniciusCestarii, xyzconstant

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#312 (util: report back child errors to parent and throw by ViniciusCestarii)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

util, refactor: Add ChildFail helper for post-fork child errors

Factor out the write-message-then-_exit(126) pattern used when a
syscall fails in the post-fork child of SpawnProcess, and document its
constraints (async-signal-safe calls only, message must not allocate)
in one place.

The execvp failure path is intentionally left alone: replacing perror()
there would lose the errno string, and improving it is already covered
by the existing TODO about reporting errors to the parent via a pipe.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

8550ee6a31

util: Clear FD_CLOEXEC in child instead of parent before fork

SpawnProcess cleared FD_CLOEXEC on the child's socket in the parent
before forking. Between that fcntl() and fork(), a concurrent fork+exec
on another thread would snapshot the descriptor with the close-on-exec
flag already cleared, leaking it into an unrelated child process where
it survives exec.

A leaked duplicate of the socket also prevents the parent from seeing
EOF when the spawned process exits.

Instead, keep both descriptors close-on-exec in the parent for their
entire lifetime and have the intended child clear the flag on its own
copy of the descriptor between fork() and exec().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

1e0c7ff9a5

Sjors force-pushed on Jul 20, 2026

ryanofsky commented at 7:40 PM on July 20, 2026: collaborator

Code review ACK 1e0c7ff9a51aedf9ddbda13dc3d8f03428da1c87. Thanks for the fix! I think it's be good to add bugfix: to the title and make PR description describe bug more practically.

As I understand it, this race has always existed, and was not introduced in 652934fb793743ab1bf962b90c7a7d6f6681c199. What 652934fb793743ab1bf962b90c7a7d6f6681c199 did was start using FD_CLOEXEC which narrowed the race window, without completely closing it. This followup PR fixes the race more completely, but there is still a small race between creating the socketpair and applying the cause the CLOEXEC flags.

The race happens when a SpawnProcess call happens at the same time as a separate fork call in unrelated thread not using SpawnProcess. Because SpawnProcess creates a socket pair, if a separate fork happens in another thread, it could inherit the socket pair file descriptors and keep them open too long if it is not looping over them and closing them like SpawnProcess is. As I understand it this could result in socketpair connections staying open even after the SpawnProcess parent or child have closed them, so onDisconnect events might not be triggered, and resources might not be freed.

ViniciusCestarii commented at 8:20 PM on July 20, 2026: contributor

ACK 1e0c7ff9a51aedf9ddbda13dc3d8f03428da1c87 verified that without this change the test https://github.com/Sjors/libmultiprocess/commit/1e1ff0397bf26b69c0c4b0dfffac4106490f482c fails and that this really tightens the race window to just between socketpair and fcntl syscalls.

xyzconstant approved

xyzconstant commented at 1:52 AM on July 21, 2026: contributor

tACK 1e0c7ff9a51aedf9ddbda13dc3d8f03428da1c87

Built and ran tests successfully. This PR adds utility changes without affecting proxy or mpgen.

LGTM, nice changes!

Sjors renamed this:
~~Clear FD_CLOEXEC in child instead of parent before fork~~
bugfix: clear FD_CLOEXEC in child instead of parent before fork
on Jul 21, 2026

Sjors commented at 8:25 AM on July 21, 2026: member

@ryanofsky I added your explanation as part of the PR description.

ryanofsky merged this on Jul 30, 2026

ryanofsky closed this on Jul 30, 2026

Sjors deleted the branch on Jul 31, 2026