diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 4eacd6abb..5eb897799 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -118,23 +118,51 @@ END { release_build_locks() } sub sh { my ($cmd) = @_; + require POSIX; say "Running: $cmd" if $VERBOSE; + + # Do not let cancellation run between fork and publishing the process group. + my $blocked = POSIX::SigSet->new(POSIX::SIGINT(), POSIX::SIGTERM()); + my $oldmask = POSIX::SigSet->new(); + POSIX::sigprocmask(POSIX::SIG_BLOCK(), $blocked, $oldmask) + or die "Cannot block build cancellation signals: $!\n"; + my $pid = fork(); unless (defined $pid) { - warn "FATAL: cannot fork to run: $cmd\n"; + my $error = "$!"; + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) + or POSIX::_exit(127); + warn "FATAL: cannot fork to run $cmd: $error\n"; return 127; } unless ($pid) { - # _exit, not exit: the child must not run the parent's END block and release a lock - # the parent still holds. - require POSIX; + $SIG{INT} = $SIG{TERM} = 'DEFAULT'; + POSIX::setpgid(0, 0) or POSIX::_exit(127); + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) + or POSIX::_exit(127); exec('/bin/sh', '-c', $cmd) or POSIX::_exit(127); } - local $CURRENT_CHILD = $pid; - # waitpid returns -1 with EINTR when a signal arrives, and a build takes signals. + + local $CURRENT_CHILD = $pid; # also the command's process-group ID + # Both sides set the group, so neither depends on which side runs first. + # EACCES means the child already exec'd, after setting its group; ESRCH + # means it has already gone away. + unless (POSIX::setpgid($pid, $pid) || $!{EACCES} || $!{ESRCH}) { + warn "FATAL: cannot create build process group $pid: $!; retaining locks\n"; + kill 'KILL', $pid; + POSIX::_exit(127); + } + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) or do { + warn "FATAL: cannot restore signal mask: $!\n"; + cancel_build('TERM'); + POSIX::_exit(127); + }; + my $got; do { $got = waitpid($pid, 0) } while ($got == -1 && $!{EINTR}); - return $? >> 8; + my $status = $?; + return 127 if $got == -1; + return ($status & 127) ? 128 + ($status & 127) : $status >> 8; } # pod2usage reads the POD of the running program, so each builder keeps its own @@ -596,17 +624,31 @@ Returns: sub cancel_build { my ($sig) = @_; require POSIX; - if ($CURRENT_CHILD) { - kill $sig => $CURRENT_CHILD; - my $gone = 0; - for (1 .. 50) { - if (waitpid($CURRENT_CHILD, POSIX::WNOHANG()) > 0) { $gone = 1; last } - select undef, undef, undef, 0.1; - } - unless ($gone) { - kill 'KILL' => $CURRENT_CHILD; - waitpid($CURRENT_CHILD, 0); + # A second Ctrl-C must not interrupt cleanup and release the lock early. + local $SIG{INT} = 'IGNORE'; + local $SIG{TERM} = 'IGNORE'; + + if (my $pgid = $CURRENT_CHILD) { + my $reaped = 0; + for my $stop_signal ($sig, 'KILL') { + kill $stop_signal, -$pgid; + for (1 .. 50) { + unless ($reaped) { + my $got = waitpid($pgid, POSIX::WNOHANG()); + $reaped = 1 if $got == $pgid || ($got == -1 && $!{ECHILD}); + } + # The shell exiting is not enough: its workers may still exist. + if (!kill(0, -$pgid) && $!{ESRCH}) { + $CURRENT_CHILD = undef; + release_build_locks(); + return; + } + select undef, undef, undef, 0.1; + } } + # Never let END/DESTROY unlock a checkout whose workers may still run. + warn "FATAL: build process group $pgid has not disappeared; retaining locks\n"; + POSIX::_exit(1); } release_build_locks(); return; diff --git a/xCAT-test/unit/builddebs_lock_cancellation.t b/xCAT-test/unit/builddebs_lock_cancellation.t index b2258306c..37d9de525 100644 --- a/xCAT-test/unit/builddebs_lock_cancellation.t +++ b/xCAT-test/unit/builddebs_lock_cancellation.t @@ -121,4 +121,67 @@ my $ckout = tempdir(CLEANUP => 1); system("pkill -f '[s]leep $marker'") }; } +# --------------------------------------------------------------------------- +# 4. THE WHOLE PROCESS GROUP GOES, not just the shell. +# Killing /bin/sh does not kill what it started: dpkg-buildpackage leaves workers behind, and +# those keep writing the checkout after the lock would otherwise have been handed to the next +# build. The command runs in its own process group so cancellation can take all of it. +# --------------------------------------------------------------------------- +{ + my $tag = '661277'; # the worker, a grandchild of the build + my $started = "$lockdir/started4"; + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + XCAT::BuildUtils::install_build_cancellation(); + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + if (open(my $st, '>', $started)) { close $st } + # a shell that spawns a worker and waits: the worker is a GRANDCHILD of this process + XCAT::BuildUtils::sh("sleep $tag & sleep $tag"); + POSIX::_exit(0); + } + my $up = 0; + for (1 .. 100) { if (-e $started) { $up = 1; last } select undef, undef, undef, 0.1 } + ok($up, 'the build started'); + + my $workers = 0; + for (1 .. 100) { + $workers = `pgrep -f "[s]leep $tag" 2>/dev/null | wc -l`; chomp $workers; + last if $workers >= 2; + select undef, undef, undef, 0.1; + } + cmp_ok($workers, '>=', 2, 'the build has a worker of its own -- the control for the check below'); + + kill 'TERM' => $pid; + for (1 .. 100) { last if waitpid($pid, WNOHANG) > 0; select undef, undef, undef, 0.1 } + + my $left = 1; + for (1 .. 100) { + $left = `pgrep -f "[s]leep $tag" 2>/dev/null | wc -l`; chomp $left; + last if $left == 0; + select undef, undef, undef, 0.1; + } + is($left, 0, 'cancelling the build takes its workers with it, not just its shell') + or do { diag('a build worker outlived the cancellation and can still write the checkout'); + system("pkill -f '[s]leep $tag'") }; +} + +# --------------------------------------------------------------------------- +# 5. A command killed by a signal reports as killed, not as success. +# $? >> 8 is 0 for a signalled child, so a build stopped mid-way looked like it had worked. +# --------------------------------------------------------------------------- +{ + my $rc = -1; + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + my $got = XCAT::BuildUtils::sh("kill -TERM \$\$"); + POSIX::_exit($got); + } + waitpid($pid, 0); + $rc = $? >> 8; + is($rc, 128 + 15, 'a command killed by SIGTERM reports 128+15, not 0'); +} + + done_testing();