Skip to content

Commit ca7b7ad

Browse files
andyclaude
andcommitted
JobStore: fix real null-pointer bug in socket comms (issue #65 root cause)
interactPort() deletes remoteconn and nulls it out the instant a socketComms handshake with eccejobmonitor succeeds ("all further comms is over socket") -- but interactGetOutput() (the function jmPORT returns into), cleanup(), and interactGetFiles() all called through remoteconn afterward with no null check, on every single successful socket-comms connection. Confirmed via a standalone repro that this is a genuine null-pointer dereference (immediate SIGSEGV in isolation; in the real, much larger eccejobstore process, the freed RCommand's memory is far more likely to already be reused by something else in between, which would explain the originally observed symptom better -- a busy CPU loop and eventual 3-minute heartbeat timeout, not a clean crash). Either way this is undefined behavior and the real reason socket comms never got past the initial handshake. Also hardened eccejobmonitor's own socket-comms launch: it's backgrounded with a raw "cmd &" (not execbg(), so it never got today's nohup fix), and interactPort() closes the launching connection right after the handshake -- under bash that risks SIGHUP killing the monitor before its first heartbeat, matching this issue's symptom and today's confirmed compute-job SIGHUP bug. Wrapped in "(trap '' HUP; ...)" rather than nohup, since nohup would redirect eccejobmonitor's stdout to nohup.out and break the port-handshake read that depends on it staying attached to the pty. csh already ignores SIGHUP for background jobs on session exit (confirmed today), so only bash needs this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 3c08697 commit ca7b7ad

1 file changed

Lines changed: 51 additions & 11 deletions

File tree

‎src/comm/commxt/JobStore.C‎

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -629,7 +629,12 @@ void cleanup(int exitStatus)
629629
if (socketComms)
630630
socketClose();
631631

632-
if (remoteconn->isOpen()) {
632+
// interactPort() deletes remoteconn and nulls it out once socket
633+
// comms takes over -- same real bug shape as the interactGetOutput()
634+
// fix above, just hit on successful completion instead of during
635+
// monitoring. Guarding rather than assuming remoteconn is always
636+
// still live here.
637+
if (remoteconn != (RCommand*)0 && remoteconn->isOpen()) {
633638
(void)remoteconn->exec("/bin/rm -f eccejobmonitor eccejobmonitor.conf "
634639
"eccejobmonitor.propbuf *.desc");
635640
delete remoteconn;
@@ -1172,7 +1177,7 @@ void interactGetFiles(void)
11721177
(refMachine->frontendMachine()!="" &&
11731178
(refMachine->frontendBypass()=="" ||
11741179
!RCommand::isSameDomain(refMachine->frontendBypass())))) {
1175-
if (remoteconn->isOpen()) {
1180+
if (remoteconn != (RCommand*)0 && remoteconn->isOpen()) {
11761181
logMessage("Benchmark", "Before shell based file transfer of output files");
11771182
status = remoteconn->shellget((const char**)fileStrs, calcdir);
11781183
logMessage("Benchmark", "After shell based file transfer of output files");
@@ -1693,6 +1698,25 @@ void initMon(void)
16931698
}
16941699

16951700
if (socketComms) {
1701+
// interactPort() below deletes remoteconn (closes the ssh/pty
1702+
// session) the moment the socket handshake succeeds, since "all
1703+
// further comms is over socket" -- but eccejobmonitor is still
1704+
// sitting in that session's process group as a backgrounded job.
1705+
// Under bash, closing the session sends it SIGHUP (confirmed
1706+
// today, same root cause as the execbg()/RCommand.C fix for
1707+
// long-running compute jobs dying the same way) -- killing the
1708+
// monitor right as socket comms takes over, so it can never send
1709+
// its next heartbeat. Matches this issue's symptom exactly
1710+
// (socket comms only; stdio comms never closes remoteconn mid-
1711+
// session so it isn't affected). csh already ignores SIGHUP for
1712+
// backgrounded jobs on session exit (confirmed empirically
1713+
// today), so only bash needs the explicit trap. Deliberately NOT
1714+
// using nohup here: nohup redirects stdout to nohup.out when it's
1715+
// still a tty, which would break the port-handshake read below
1716+
// (and the stdio-comms path's entire comms stream) that depends
1717+
// on eccejobmonitor's stdout staying attached to this pty.
1718+
if (cpLocalShell == "bash")
1719+
cmd = "(trap '' HUP; " + cmd + ")";
16961720
cmd += "&";
16971721
if (!remoteconn->expwrite(cmd))
16981722
restart("System", remoteconn->commError());
@@ -1937,15 +1961,31 @@ void interactGetOutput(void)
19371961

19381962
XtRemoveInput(jobMonitorId);
19391963

1940-
// check status of remote shell and prep it for further usage
1941-
// transferring output files, etc.
1942-
(void)remoteconn->expect1("\r\n+go+$");
1943-
if (remoteconn->isOpen())
1944-
logMessage("Benchmark", "eccejobmonitor done--"
1945-
"remote shell connection open");
1946-
else
1947-
logMessage("Benchmark", "eccejobmonitor done--"
1948-
"remote shell connection is not available");
1964+
// interactPort() deletes remoteconn and nulls it out the instant a
1965+
// socketComms handshake succeeds ("close remote shell since all
1966+
// further comms is over socket") -- and jmPORT sets doneFlag=true
1967+
// specifically to return here right after that happens, so this is
1968+
// the common case for socket comms, not a rare edge case. Calling
1969+
// through remoteconn unconditionally below was a genuine
1970+
// use-after-free/null-dereference on every successful socket-comms
1971+
// connection: found by tracing why socket comms (issue #65) never
1972+
// got past the initial handshake even when eccejobmonitor's own
1973+
// side connected correctly -- accessing the freed RCommand's stale
1974+
// internal fd here plausibly explains the observed busy CPU loop and
1975+
// eventual 3-minute heartbeat timeout (a corrupted/recycled fd value
1976+
// being read from freed memory, retried internally by the expect
1977+
// library, rather than a clean crash).
1978+
if (remoteconn != (RCommand*)0) {
1979+
// check status of remote shell and prep it for further usage
1980+
// transferring output files, etc.
1981+
(void)remoteconn->expect1("\r\n+go+$");
1982+
if (remoteconn->isOpen())
1983+
logMessage("Benchmark", "eccejobmonitor done--"
1984+
"remote shell connection open");
1985+
else
1986+
logMessage("Benchmark", "eccejobmonitor done--"
1987+
"remote shell connection is not available");
1988+
}
19491989
}
19501990

19511991

0 commit comments

Comments
 (0)