bugfix: clear FD_CLOEXEC in child instead of parent before fork#311
bugfix: clear FD_CLOEXEC in child instead of parent before fork#311Sjors wants to merge 2 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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>
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>
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 1e0c7ff. 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 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.
|
ACK 1e0c7ff verified that without this change the test Sjors@1e1ff03 fails and that this really tightens the race window to just between socketpair and fcntl syscalls. |
xyzconstant
left a comment
xyzconstant
left a comment
There was a problem hiding this comment.
tACK 1e0c7ff
Built and ran tests successfully. This PR adds utility changes without affecting proxy or mpgen.
LGTM, nice changes!
Sjors
changed the title
|
@ryanofsky I added your explanation as part of the PR description. |
5 participants
Commit 652934f in #274 sets
FD_CLOEXECin 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_CLOEXECbefore fork. ryanofsky also described it:A test in Sjors@1e1ff03 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.