Commit a6fc80d
committed
Merge bitcoin-core/libmultiprocess#311: bugfix: clear FD_CLOEXEC in child instead of parent before fork
1e0c7ff util: Clear FD_CLOEXEC in child instead of parent before fork (Sjors Provoost)
8550ee6 util, refactor: Add ChildFail helper for post-fork child errors (Sjors Provoost)
Pull request description:
Commit 652934f in bitcoin#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](bitcoin-core/libmultiprocess@652934f). What [652934f](bitcoin-core/libmultiprocess@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 Sjors/libmultiprocess@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.
ACKs for top commit:
ViniciusCestarii:
ACK 1e0c7ff verified that without this change the test Sjors/libmultiprocess@1e1ff03 fails and that this really tightens the race window to just between socketpair and fcntl syscalls.
ryanofsky:
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.
xyzconstant:
tACK 1e0c7ff
Tree-SHA512: eaf127de19ad579dc2adc1276f7e160e9b6478257cadccfa7b4dd3faf348cb11b58394d1e7987e81a6fc0001410c73f923c0d7ce63c235f43d0c599226bffc131 file changed
+23-9Lines changed: 23 additions & 9 deletions
Expand file treeCollapse file tree
Open diff view settings
Collapse file
+23-9Lines changed: 23 additions & 9 deletions
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
60 | 60 | |
61 | 61 | |
62 | 62 | |
| 63 | + |
| 64 | + |
| 65 | + |
| 66 | + |
| 67 | + |
| 68 | + |
| 69 | + |
| 70 | + |
| 71 | + |
| 72 | + |
| 73 | + |
| 74 | + |
63 | 75 | |
64 | 76 | |
65 | 77 | |
| ||
130 | 142 | |
131 | 143 | |
132 | 144 | |
133 | | - |
134 | | - |
135 | | - |
136 | | - |
137 | | - |
138 | 145 | |
139 | 146 | |
140 | 147 | |
| ||
147 | 154 | |
148 | 155 | |
149 | 156 | |
150 | | - |
151 | | - |
152 | | - |
153 | | - |
| 157 | + |
154 | 158 | |
155 | 159 | |
156 | 160 | |
| ||
163 | 167 | |
164 | 168 | |
165 | 169 | |
| 170 | + |
| 171 | + |
| 172 | + |
| 173 | + |
| 174 | + |
| 175 | + |
| 176 | + |
| 177 | + |
| 178 | + |
| 179 | + |
166 | 180 | |
167 | 181 | |
168 | 182 | |
|
0 commit comments