[PATCH common v2 2/2] cmd: run: kill the command when run dies

[Kefu Chai <k.chai@proxmox.com>]

If a signal handler of the caller dies while the command runs, run()
leaves the command running. So 'pvesm alloc' on an RBD pool whose
placement groups never become active fails with

  'storage-<id>'-locked command timed out - aborting

and releases the lock, while 'rbd create' keeps running.

Kill and reap the command after the eval if it was not reaped yet,
which replaces the kill and waitpid calls on the error paths, and drop
the evals around the callbacks that only rethrew. A killed 'rbd create'
may still leave a partial image behind.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 src/PVE/Cmd.pm   | 58 ++++++++++++++++++------------------------------
 test/cmd-test.pl | 40 ++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 38 deletions(-)

diff --git a/src/PVE/Cmd.pm b/src/PVE/Cmd.pm
index 4b470a1..edff112 100644
--- a/src/PVE/Cmd.pm
+++ b/src/PVE/Cmd.pm
@@ -102,6 +102,7 @@ sub run($cmd, %param) {
     my $oldtimeout;
     my $armed_at;
     my $pid;
+    my $reaped;
     my $exitcode = -1;
 
     my $outfunc;
@@ -226,51 +227,30 @@ sub run($cmd, %param) {
             foreach my $h (@handles) {
                 my $buf = '';
                 my $count = sysread($h, $buf, 4096);
-                if (!defined($count)) {
-                    my $err = $!;
-                    kill(9, $pid);
-                    waitpid($pid, 0);
-                    die $err;
-                }
+                die $! if !defined($count);
                 $select->remove($h) if !$count;
                 if ($h eq $reader) {
                     if ($outfunc || $logfunc) {
-                        eval {
-                            while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) {
-                                my $line = $outlog . $1;
-                                $outlog = '';
-                                &$outfunc($line) if $outfunc;
-                                &$logfunc($line) if $logfunc;
-                            }
-                            $outlog .= $buf;
-                        };
-                        my $err = $@;
-                        if ($err) {
-                            kill(9, $pid);
-                            waitpid($pid, 0);
-                            die $err;
+                        while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) {
+                            my $line = $outlog . $1;
+                            $outlog = '';
+                            &$outfunc($line) if $outfunc;
+                            &$logfunc($line) if $logfunc;
                         }
+                        $outlog .= $buf;
                     } elsif (!$quiet) {
                         print $buf;
                         *STDOUT->flush();
                     }
                 } elsif ($h eq $error) {
                     if ($errfunc || $logfunc) {
-                        eval {
-                            while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) {
-                                my $line = $errlog . $1;
-                                $errlog = '';
-                                &$errfunc($line) if $errfunc;
-                                &$logfunc($line) if $logfunc;
-                            }
-                            $errlog .= $buf;
-                        };
-                        my $err = $@;
-                        if ($err) {
-                            kill(9, $pid);
-                            waitpid($pid, 0);
-                            die $err;
+                        while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) {
+                            my $line = $errlog . $1;
+                            $errlog = '';
+                            &$errfunc($line) if $errfunc;
+                            &$logfunc($line) if $logfunc;
                         }
+                        $errlog .= $buf;
                     } elsif (!$quiet) {
                         print STDERR $buf;
                         *STDERR->flush();
@@ -285,7 +265,7 @@ sub run($cmd, %param) {
         &$errfunc($errlog) if $errfunc && $errlog;
         &$logfunc($errlog) if $logfunc && $errlog;
 
-        waitpid($pid, 0);
+        $reaped = waitpid($pid, 0); # same statement, so no signal handler runs in between
 
         if ($? == -1) {
             die "failed to execute\n";
@@ -309,6 +289,12 @@ sub run($cmd, %param) {
 
     alarm(0) if $armed_at;
 
+    # whatever made us die, also a signal handler of the caller, the command must not outlive us
+    if ($pid && !$reaped) {
+        kill(9, $pid);
+        waitpid($pid, 0);
+    }
+
     if ($errmsg && $laststderr) {
         # the caller's alarm can fire during the flush, and the callback can
         # die on its own, so do not let it replace the error we already have
@@ -334,8 +320,6 @@ sub run($cmd, %param) {
 
     if ($err) {
         if ($pid && ($err eq "got timeout\n")) {
-            kill(9, $pid);
-            waitpid($pid, 0);
             die "command '$cmdstr' failed: $err";
         }
 
diff --git a/test/cmd-test.pl b/test/cmd-test.pl
index 518b369..86df8eb 100755
--- a/test/cmd-test.pl
+++ b/test/cmd-test.pl
@@ -10,6 +10,7 @@ use lib '../src';
 
 use PVE::Cmd;
 
+use POSIX ();
 use Test::More;
 use Time::HiRes ();
 
@@ -112,9 +113,46 @@ for my $test ($caller_alarm_tests->@*) {
     like(join('', @warned), qr/boom/, 'run warns about the dying errfunc');
 }
 
+# run must not leave its command behind when it dies, whatever makes it die
+#
+# name  - when run dies
+# param - parameters for run
+# alarm - seconds until the caller's alarm handler dies
+# err   - expected error
+my $reap_tests = [
+    { name => 'its own timeout fires', param => { timeout => 1 }, err => qr/got timeout/ },
+    {
+        name => 'outfunc dies',
+        param => { outfunc => sub { die "outfunc failed\n" } },
+        err => qr/outfunc failed/,
+    },
+    { name => "the caller's alarm handler dies", alarm => 1, err => qr/caller timed out/ },
+];
+for my $test ($reap_tests->@*) {
+    my $pid;
+    my %param = ($test->{param} // {})->%*;
+    my $outfunc = delete($param{outfunc}) // sub { };
+    eval {
+        local $SIG{ALRM} = sub { die "caller timed out\n" };
+        alarm($test->{alarm}) if $test->{alarm};
+        PVE::Cmd::run(
+            ['sh', '-c', 'echo $$; exec sleep 30'],
+            outfunc => sub { $pid //= $_[0]; $outfunc->(@_) },
+            %param,
+        );
+    };
+    my $err = $@;
+    alarm(0);
+    like($err, $test->{err}, "run dies when $test->{name}");
+    ok(
+        waitpid($pid, POSIX::WNOHANG) == -1 && !kill(0, $pid),
+        "run reaps the command when $test->{name}",
+    );
+}
+
 # TODO:
 # - cover pipe_socket (needs a socket peer)
-# - cover timeouts and the errfunc/logfunc callbacks
+# - cover the errfunc/logfunc callbacks
 # - more tests
 
 done_testing();
-- 
2.47.3