Skip to content

Navigation Menu

Sign in
Appearance settings
Sign in
Sign up
Appearance settings

Commit e5e367e

Browse filesBrowse the repository at this point in the historyBrowse files
ryanofskyryanofsky
committed
Merge bitcoin-core/libmultiprocess#312: util: report back child errors to parent and throw
3f05b11 util: kill and reap child on SpawnProcess error (ViniciusCestarii) 4a56c18 util: report back child error to parent and throw (ViniciusCestarii) Pull request description: Report execvp() and close() failures to the parent via a socket and throw from SpawnProcess. Previously the parent didn't have a way of knowing whether the child process successfully execute or not without polling for it or waiting for it. Now SpawnProcess waits for the exec() to happen before returning and before throwing it also reaps the child, preventing zombie process. Also make post-fork child not rely on on perror(), which is not async-signal-safe. This addresses the TODO on commit 69652f0 ACKs for top commit: xyzconstant: LGTM. ACK 3f05b11 ryanofsky: Code review ACK 3f05b11. Looks great and makes the libmultiprocess library more competent at spawning processes, actually providing specific error information if this fails Tree-SHA512: 670aaa121e33fe89aaf69c8111d9b5763a449ddfed0b1886c848effd4dbdb857277a7fe6e2e7f47bda7fed580295160d444f75d50886e22481374c796eafbb0e
2 parents 2220df6 + 3f05b11 commit e5e367e
Copy full SHA for e5e367e

2 files changed

+158-11Lines changed: 158 additions & 11 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
+140-11Lines changed: 140 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111
#include <kj/common.h>
1212
#include <kj/debug.h>
1313
#include <kj/string-tree.h>
14+
#include <optional>
1415
#include <pthread.h>
16+
#include <csignal>
1517
#include <sstream>
1618
#include <string>
1719
#include <sys/types.h>
@@ -72,6 +74,102 @@ template <std::size_t N>
7274
_exit(126);
7375
}
7476

77+
enum class SpawnErrorOp
78+
{
79+
CLOSE,
80+
EXECVP,
81+
READ,
82+
FCNTL,
83+
};
84+
85+
struct SpawnError {
86+
SpawnErrorOp which;
87+
int err;
88+
};
89+
90+
const char* SpawnErrorName(SpawnErrorOp which)
91+
{
92+
switch (which) {
93+
case SpawnErrorOp::CLOSE: return "close";
94+
case SpawnErrorOp::EXECVP: return "execvp";
95+
case SpawnErrorOp::READ: return "read";
96+
case SpawnErrorOp::FCNTL: return "fcntl";
97+
}
98+
return "unknown";
99+
}
100+
101+
// Read the child's error report. Returns nullopt on success, or a SpawnError to
102+
// throw on failure. Success is a clean EOF: read() returns 0 with nothing
103+
// buffered because the child's write end was closed by a successful exec (via
104+
// FD_CLOEXEC). A fully-read struct is the failure the child reported. A read()
105+
// error, or an EOF partway through the struct (the child died mid-report), is
106+
// surfaced as a SpawnErrorOp::READ failure rather than being mistaken for
107+
// success. A single read() is not guaranteed to return all sizeof(SpawnError)
108+
// bytes (the socket is SOCK_STREAM, which has no message boundaries) and may be
109+
// interrupted by a signal, so loop until the whole struct is read.
110+
//
111+
// Note that a clean EOF is also what happens if the child is killed before it
112+
// reaches exec, so this function reports success in that case. There is no good
113+
// way to distinguish the two here, but callers will still see the failure when
114+
// they call WaitProcess and get the child's exit status.
115+
std::optional<SpawnError> ReadSpawnResult(int fd)
116+
{
117+
SpawnError error{};
118+
char* buf = reinterpret_cast<char*>(&error);
119+
size_t remaining = sizeof(error);
120+
while (remaining > 0) {
121+
const ssize_t n = ::read(fd, buf, remaining);
122+
if (n < 0) {
123+
if (errno == EINTR) continue;
124+
return SpawnError{.which = SpawnErrorOp::READ, .err = errno};
125+
}
126+
if (n == 0) {
127+
if (remaining == sizeof(error)) return std::nullopt; // clean EOF: success
128+
return SpawnError{.which = SpawnErrorOp::READ, .err = EPROTO}; // torn report
129+
}
130+
buf += n;
131+
remaining -= static_cast<size_t>(n);
132+
}
133+
return error;
134+
}
135+
136+
// Write the whole SpawnError to fd, retrying short writes and EINTR so the
137+
// parent never sees a torn struct. Runs in the post-fork child, so it must stay
138+
// async-signal-safe: it only calls write() and does not allocate or throw. This
139+
// is best-effort -- if the write cannot complete there is nothing useful the
140+
// child can do, so it stops and lets the caller _exit().
141+
void WriteSpawnError(int fd, const SpawnError& error)
142+
{
143+
const char* buf = reinterpret_cast<const char*>(&error);
144+
size_t remaining = sizeof(error);
145+
while (remaining > 0) {
146+
const ssize_t n = ::write(fd, buf, remaining);
147+
if (n < 0) {
148+
if (errno == EINTR) continue;
149+
break;
150+
}
151+
buf += n;
152+
remaining -= static_cast<size_t>(n);
153+
}
154+
if (remaining > 0) {
155+
// The parent's read end is gone (e.g. the parent exited before the
156+
// child could report), so the structured error can't be delivered.
157+
// Leave a breadcrumb on stderr and exit. The exit code is irrelevant
158+
// here since no live parent remains to wait on it.
159+
ChildFail("SpawnProcess(child): failed and could not report error to parent\n");
160+
}
161+
}
162+
163+
// Get rid of a child process the parent is abandoning because SpawnProcess is
164+
// about to throw, so it is not left behind as a zombie. The child may still be
165+
// alive, so kill it first: waiting without that could block for as long as the
166+
// spawned program runs.
167+
void KillAndReapChild(ProcessId pid)
168+
{
169+
(void)::kill(pid, SIGKILL);
170+
while (::waitpid(pid, /*status=*/nullptr, /*options=*/0) == -1 && errno == EINTR) {}
171+
}
172+
75173
} // namespace
76174

77175
std::string ThreadName(const char* exe_name)
@@ -132,6 +230,9 @@ std::string LogEscape(const kj::StringTree& string, size_t max_size)
132230
std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_info_to_args)
133231
{
134232
auto fds{SocketPair()};
233+
// Only used for the child to report errors back to the parent, so a one-way
234+
// pipe would be sufficient, but reuse the existing SocketPair() helper.
235+
auto error_fds{SocketPair()};
135236

136237
// Evaluate the callback and build the argv array before forking.
137238
//
@@ -144,25 +245,36 @@ std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_
144245

145246
ProcessId pid = fork();
146247
if (pid == -1) {
147-
throw std::system_error(errno, std::system_category(), "fork");
248+
const int err = errno;
249+
(void)close(fds[0]);
250+
(void)close(fds[1]);
251+
(void)close(error_fds[0]);
252+
(void)close(error_fds[1]);
253+
throw std::system_error(err, std::system_category(), "fork");
148254
}
149255
// Parent process closes the descriptor for socket 0, child closes the
150256
// descriptor for socket 1. On failure, the parent throws, but the child
151257
// must _exit(126) (post-fork child must not throw).
152258
if (close(fds[pid ? 0 : 1]) != 0) {
259+
const int err = errno;
153260
if (pid) {
154261
(void)close(fds[1]);
155-
throw std::system_error(errno, std::system_category(), "close");
262+
(void)close(error_fds[0]);
263+
(void)close(error_fds[1]);
264+
KillAndReapChild(pid);
265+
throw std::system_error(err, std::system_category(), "close");
156266
}
157-
ChildFail("SpawnProcess(child): close(fds[1]) failed\n");
267+
WriteSpawnError(error_fds[0], {.which = SpawnErrorOp::CLOSE, .err = err});
268+
_exit(126);
158269
}
159270

160271
if (!pid) {
161272
// Child process must close all potentially open descriptors, except
162-
// socket 0. Do not throw, allocate, or do non-fork-safe work here.
273+
// socket 0 and the error-reporting socket 0. Do not throw, allocate, or
274+
// do non-fork-safe work here.
163275
const int maxFd = MaxFd();
164276
for (int fd = 3; fd < maxFd; ++fd) {
165-
if (fd != fds[0]) {
277+
if (fd != fds[0] && fd != error_fds[0]) {
166278
close(fd);
167279
}
168280
}
@@ -174,17 +286,34 @@ std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_
174286
// fcntl is async-signal-safe.
175287
const int fds0_flags = fcntl(fds[0], F_GETFD);
176288
if (fds0_flags == -1 || fcntl(fds[0], F_SETFD, fds0_flags & ~FD_CLOEXEC) == -1) {
177-
ChildFail("SpawnProcess(child): clearing FD_CLOEXEC failed\n");
289+
const int err = errno;
290+
WriteSpawnError(error_fds[0], {.which = SpawnErrorOp::FCNTL, .err = err});
291+
_exit(126);
178292
}
179293

180294
execvp(argv[0], argv.data());
181-
// NOTE: perror() is not async-signal-safe; calling it here in a
182-
// post-fork child may deadlock in multithreaded parents.
183-
// TODO: Report errors to the parent via a pipe (e.g. write errno)
184-
// so callers can get diagnostics without relying on perror().
185-
perror("execvp failed");
295+
296+
const int err = errno;
297+
WriteSpawnError(error_fds[0], {.which = SpawnErrorOp::EXECVP, .err = err});
186298
_exit(127);
187299
}
300+
301+
// Close the parent's copy of the child's write end.
302+
if (close(error_fds[0]) != 0) {
303+
const int err = errno;
304+
(void)close(error_fds[1]);
305+
(void)close(fds[1]);
306+
KillAndReapChild(pid);
307+
throw std::system_error(err, std::system_category(), "close");
308+
}
309+
310+
const std::optional<SpawnError> error{ReadSpawnResult(error_fds[1])};
311+
(void)close(error_fds[1]);
312+
if (error) {
313+
(void)close(fds[1]);
314+
KillAndReapChild(pid);
315+
throw std::system_error(error->err, std::system_category(), SpawnErrorName(error->which));
316+
}
188317
return {pid, fds[1]};
189318
}
190319

Collapse file

‎test/mp/test/spawn_tests.cpp‎

Copy file name to clipboardExpand all lines: test/mp/test/spawn_tests.cpp
+18Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,20 @@
44

55
#include <mp/util.h>
66

7+
#include <kj/common.h>
8+
#include <kj/debug.h>
79
#include <kj/test.h>
810

11+
#include <cerrno>
912
#include <chrono>
1013
#include <compare>
1114
#include <condition_variable>
1215
#include <csignal>
1316
#include <cstdlib>
1417
#include <mutex>
1518
#include <string>
19+
#include <string_view>
20+
#include <system_error>
1621
#include <sys/wait.h>
1722
#include <thread>
1823
#include <tuple>
@@ -113,5 +118,18 @@ KJ_TEST("SpawnProcess does not run callback in child")
113118
KJ_EXPECT(exited, "Timeout waiting for child process to exit");
114119
KJ_EXPECT(WIFEXITED(status) && WEXITSTATUS(status) == 0);
115120
}
121+
122+
KJ_TEST("SpawnProcess throws on execvp failure")
123+
{
124+
try {
125+
SpawnProcess([&](SpawnConnectInfo) -> std::vector<std::string> {
126+
return {"/nonexistent/binary"};
127+
});
128+
KJ_EXPECT(false, "expected SpawnProcess to throw");
129+
} catch (const std::system_error& e) {
130+
KJ_EXPECT(e.code().value() == ENOENT);
131+
KJ_EXPECT(std::string_view{e.what()}.find("execvp") != std::string_view::npos);
132+
}
133+
}
116134
} // namespace test
117135
} // namespace mp

0 commit comments

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