Skip to content

Commit b52bc21

Browse files
committed
ExternalProcess: don't kill the process group when a spawned command fails
Two bugs made a failed external command tear down the entire process group: - The fork child threw cRuntimeError when execvp fails. In the child (a copy of the sim process) that unwinds into a full simulation teardown, running ~ExternalProcess() with pid==0 -> stopProcess() -> "/bin/kill -SIGTERM 0", which SIGTERMs the whole process group. A1: report the errno on the redirected stderr and _exit(127) instead of throwing, so the parent just sees the process exit and applies its onExit policy. Use _exit (not exit) so the fork child never runs the parent's atexit/static destructors. - stopProcess() guarded only "pid != -1", so pid==0 (or any non-positive value) would issue "kill 0" / "kill -1" against the process group. A2: guard "pid > 0". Surfaced by CI: running INET's module suite concurrently under a single podman-exec process group, a missing external tool (ping/killall absent from the container) tripped this and SIGTERM'd the whole run (opp_env included -> exit 143) at ~140/321. With this fix a missing/failing command fails only its own test.
1 parent 9097c7b commit b52bc21

1 file changed

Lines changed: 19 additions & 2 deletions

File tree

src/inet/common/ExternalProcess.cc

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@
88

99
#include "inet/common/NetworkNamespaceContext.h"
1010

11+
#include <cerrno>
12+
#include <cstdio>
13+
#include <cstring>
1114
#include <fcntl.h>
15+
#include <unistd.h>
1216

1317
namespace inet {
1418

@@ -115,7 +119,17 @@ void ExternalProcess::startProcess()
115119
}
116120
args.push_back(nullptr);
117121
execvp(args[0], &args[0]);
118-
throw cRuntimeError("Failed to execute command");
122+
// execvp only returns on failure, and this is the fork child -- a copy of the
123+
// simulation process. It must NOT throw: a cRuntimeError here unwinds into a
124+
// full simulation teardown *inside the child*, which runs ~ExternalProcess()
125+
// with pid==0, whose stopProcess() then executes `/bin/kill -SIGTERM 0` and
126+
// SIGTERMs the entire process group (the whole concurrent test run, not just
127+
// this command). Report the failure on the (redirected) stderr and _exit()
128+
// immediately, so the parent simply observes the process exit and applies its
129+
// `onExit` policy. Use _exit (not exit) to skip the parent's atexit/static
130+
// destructors, which must never run in the fork child.
131+
std::fprintf(stderr, "ExternalProcess: cannot execute '%s': %s\n", command, std::strerror(errno));
132+
_exit(127);
119133
}
120134
else { // parent process
121135
close(stdout_pipe[1]);
@@ -135,7 +149,10 @@ void ExternalProcess::startProcess()
135149
void ExternalProcess::stopProcess()
136150
{
137151
EV_DEBUG << "Stopping process: " << command << std::endl;
138-
if (pid != -1) {
152+
// Only ever kill a real child PID. pid==-1 means "never started"; pid==0 must never
153+
// reach here (that is the fork child -- startProcess now _exit()s instead of throwing),
154+
// and `kill 0` / `kill -1` would signal the whole process group, not just this command.
155+
if (pid > 0) {
139156
rtScheduler->removeCallback(processStdout, this);
140157
rtScheduler->removeCallback(processStderr, this);
141158
std::string prefix = !strncmp("sudo", command, 4) ? "sudo " : "";

0 commit comments

Comments
 (0)