Skip to content

Navigation Menu

Sign in
Appearance settings
Sign up
Appearance settings

Commit a6fc80d

Browse filesBrowse the repository at this point in the historyBrowse files
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: eaf127de19ad579dc2adc1276f7e160e9b6478257cadccfa7b4dd3faf348cb11b58394d1e7987e81a6fc0001410c73f923c0d7ce63c235f43d0c599226bffc13
2 parents f5c15ce + 1e0c7ff commit a6fc80d
Copy full SHA for a6fc80d

1 file changed

+23-9Lines changed: 23 additions & 9 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/mp/util.cpp‎

Copy file name to clipboardExpand all lines: src/mp/util.cpp
+23-9Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,18 @@ size_t MaxFd()
6060
}
6161
}
6262

63+
//! Report an error and exit from the post-fork child of a multi-threaded
64+
//! process, where only async-signal-safe calls (like write and _exit) are
65+
//! allowed. Accepting only a reference to a char array (in practice a string
66+
//! literal) ensures no allocation is needed at the call site.
67+
template <std::size_t N>
68+
[[noreturn]] void ChildFail(const char (&msg)[N]) noexcept
69+
{
70+
const ssize_t written = ::write(STDERR_FILENO, msg, N - 1);
71+
(void)written;
72+
_exit(126);
73+
}
74+
6375
} // namespace
6476

6577
std::string ThreadName(const char* exe_name)
@@ -130,11 +142,6 @@ std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_
130142
const std::vector<std::string> args{connect_info_to_args(std::to_string(fds[0]))};
131143
const std::vector<char*> argv{MakeArgv(args)};
132144

133-
// Clear FD_CLOEXEC on fds[0] before forking so it survives exec in the child.
134-
int fds0_flags;
135-
KJ_SYSCALL(fds0_flags = fcntl(fds[0], F_GETFD));
136-
KJ_SYSCALL(fcntl(fds[0], F_SETFD, fds0_flags & ~FD_CLOEXEC));
137-
138145
ProcessId pid = fork();
139146
if (pid == -1) {
140147
throw std::system_error(errno, std::system_category(), "fork");
@@ -147,10 +154,7 @@ std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_
147154
(void)close(fds[1]);
148155
throw std::system_error(errno, std::system_category(), "close");
149156
}
150-
static constexpr char msg[] = "SpawnProcess(child): close(fds[1]) failed\n";
151-
const ssize_t writeResult = ::write(STDERR_FILENO, msg, sizeof(msg) - 1);
152-
(void)writeResult;
153-
_exit(126);
157+
ChildFail("SpawnProcess(child): close(fds[1]) failed\n");
154158
}
155159

156160
if (!pid) {
@@ -163,6 +167,16 @@ std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_
163167
}
164168
}
165169

170+
// Clear FD_CLOEXEC on socket 0 so it survives exec. Doing this here
171+
// rather than in the parent before forking avoids a window where a
172+
// concurrent fork in another thread would leak the descriptor into an
173+
// unrelated child process (which would not clear it).
174+
// fcntl is async-signal-safe.
175+
const int fds0_flags = fcntl(fds[0], F_GETFD);
176+
if (fds0_flags == -1 || fcntl(fds[0], F_SETFD, fds0_flags & ~FD_CLOEXEC) == -1) {
177+
ChildFail("SpawnProcess(child): clearing FD_CLOEXEC failed\n");
178+
}
179+
166180
execvp(argv[0], argv.data());
167181
// NOTE: perror() is not async-signal-safe; calling it here in a
168182
// post-fork child may deadlock in multithreaded parents.

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.