From 49e77398e4006927e2ebf384195d29624364438f Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:56:11 -0300 Subject: [PATCH] fix(xcat-core): supervise() lets SIGCHLD back in before the caller has the pid supervise() blocks SIGCHLD across the fork but unblocks it before returning, and the caller installs the pid afterwards: ($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { ... } ...; so the assignment is outside the blocked region -- the same unprotected window that existed before e0b0ac6, moved from xcatd into the helper that was meant to make it impossible to get wrong. ssl_reaper matches the dead child against $pid_MON and folds the death into $mon_respawn; a monitor dying in that gap is compared against a pid still holding 0, missed, and the caller then overwrites both with a pid that no longer exists. !$pid_MON never fires again, so the respawn loop never runs and xcatiport stays dead until xcatd is restarted -- the failure this PR exists to remove. Have supervise() install them itself, which is why `state` and `pid` are now passed by reference: the pacing state is recorded and the pid assigned while SIGCHLD is still blocked, and only then is it unblocked, so there is no point at which a reaper can run and see either of them stale. Nothing is left for the caller to do afterwards, so both call sites become plain statements that read $pid_MON when they need it. The child unblocks before running its body, as it did when the unblock sat ahead of the fork's branch. The new pid is returned as well, for a caller that wants it inline. Verified on a live MN (xcat54-mn, AlmaLinux 10.2, xCAT 2.19.0): the startup fork produces a monitor holding xcatiport 3002; killing it is recovered in 5s, killing the replacement at once in 11s -- the backoff -- and killing one that had served past the healthy interval is recovered in 1s, with the port reclaimed and xcatd active throughout. The unit test's window subtest, red in the preceding commit, now passes. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 45 +++++++++++++++++--------- xCAT-server/sbin/xcatd | 14 ++++---- xCAT-test/unit/xcatd_monitor_respawn.t | 8 ++--- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index e770bce0e..43a8f73ae 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -125,10 +125,10 @@ sub reported { # The one impure sub. Everything above only does arithmetic; this actually forks. # Fork a child and keep the pacing straight while doing it. Takes the child's body as a -# block, then `state`, `pid` and `now`, and hands back the state and the new pid: +# block, then `state` and `pid` -- REFERENCES to the caller's own variables -- and `now`: # -# ($state, $pid) = xCAT::RespawnUtils::supervise { ...child... } -# state => $state, pid => $pid, now => time(); +# xCAT::RespawnUtils::supervise { ...child... } +# state => \$state, pid => \$pid, now => time(); # # The (&@) prototype is what allows the leading block. It needs this module loaded with # `use`, not `require`: under `require` the sub is unknown when the call is compiled, the @@ -136,36 +136,49 @@ sub reported { # # Two orderings in here are easy to get wrong and are the reason this is not left to callers. # The attempt is recorded before the fork, because the child can die and be reaped before -# fork() returns to us. And SIGCHLD is blocked across the fork and the assignment, because a -# reaper that matches on the pid would otherwise compare against a stale one, miss the death, -# and leave the caller believing a dead child is still alive. +# fork() returns to us. And the pid and the state are installed in the caller's variables +# while SIGCHLD is still blocked -- which is why they are passed by reference rather than +# handed back as a return value. The reaper matches the dead child against that pid and folds +# the death into that state; had the caller assigned them from a return value, the assignment +# would land after the signal was let back in, so a child dying in the gap would be compared +# against a pid still holding 0, missed, and the caller would then write a dead pid back over +# the reaper's work -- believing a dead child alive, and never respawning it. # # The block is only ever entered in the child and is not expected to return; if it does, the # child exits quietly rather than falling back into the parent's code. Passing a live `pid` -# is a no-op, so a caller that forgets to check is not punished with a second child. +# is a no-op, so a caller that forgets to check is not punished with a second child. The new +# pid is also returned, for a caller that wants it inline. sub supervise (&@) { my ($child, %arg) = @_; - my ($state, $pid, $now) = @arg{qw(state pid now)}; + my ($stateref, $pidref, $now) = @arg{qw(state pid now)}; - return ($state, $pid) if $pid; # already running; nothing to do + return $$pidref if $$pidref; # already running; nothing to do require POSIX; require xCAT::Utils; - $state = forked($state, $now); - my $mask = POSIX::SigSet->new(POSIX::SIGCHLD()); POSIX::sigprocmask(POSIX::SIG_BLOCK(), $mask); - $pid = xCAT::Utils->xfork; - POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); - return (exited($state, $now), 0) unless defined $pid; # could not fork: back off + $$stateref = forked($$stateref, $now); + my $pid = xCAT::Utils->xfork; - unless ($pid) { + unless (defined $pid) { # could not fork: count it and back off + $$stateref = exited($$stateref, $now); + $$pidref = 0; + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + return 0; + } + + unless ($pid) { # child: it must not go on to serve with SIGCHLD blocked + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); $child->(); POSIX::_exit(0); } - return ($state, $pid); + + $$pidref = $pid; # in place before the reaper can run, or it matches a stale pid + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + return $pid; } 1; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 74c0f6293..0ca56274a 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -1203,15 +1203,17 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); -# supervise() records the attempt and blocks SIGCHLD across the fork; this monitor's uptime -# counts towards the pacing too, so an unrelated death much later is retried promptly. -($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { +# supervise() records the attempt, blocks SIGCHLD across the fork, and installs $pid_MON and +# $mon_respawn itself while it is still blocked -- which is why they are passed by reference; +# see the sub. This monitor's uptime counts towards the pacing too, so an unrelated death much +# later is retried promptly. +xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($udpctl); $udpctl = 0; do_installm_service; xexit(0); -} state => $mon_respawn, pid => $pid_MON, now => time(); +} state => \$mon_respawn, pid => \$pid_MON, now => time(); unless ($pid_MON) { xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); @@ -1494,7 +1496,7 @@ until ($quit) { . " still retrying xcatiport $sport every $mon_respawn->{max_interval} seconds"); $mon_respawn = xCAT::RespawnUtils::reported($mon_respawn); } - ($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($listener); @@ -1507,7 +1509,7 @@ until ($quit) { close($chwritepipe); do_installm_service; xexit(0); - } state => $mon_respawn, pid => $pid_MON, now => time(); + } state => \$mon_respawn, pid => \$pid_MON, now => time(); if ($pid_MON) { xCAT::MsgUtils->trace(0, "I", diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 8f1cc0b11..fe7a36751 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -223,10 +223,10 @@ subtest 'the pid and the pacing are in place before SIGCHLD is let back in' => s } select( undef, undef, undef, 0.2 ); # it is gone, and its SIGCHLD is pending, not delivered - ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { sleep 3600; # a monitor that stays up; this one is about the parent } - state => $pace, pid => $mon_pid, now => time(); + state => \$pace, pid => \$mon_pid, now => time(); push @spawned, $mon_pid if $mon_pid; ok( $ran, 'the pending SIGCHLD was delivered while supervise() was still running' ); @@ -291,7 +291,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { # Driven through supervise() -- the same call xcatd makes -- so this exercises the # real fork-and-account sequence rather than a copy of it here. - ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { close($holder) if $holder; # never hold the port from inside a child my $sock = IO::Socket::INET->new( LocalAddr => '127.0.0.1', @@ -303,7 +303,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does sleep 3600; # bound the port and serve } - state => $pace, pid => $mon_pid, now => $now; + state => \$pace, pid => \$mon_pid, now => $now; die "supervise did not fork" unless $mon_pid; $mon_forked_at = $now;