[PATCH common 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 a6bb17d..581bb93 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;
@@ -223,51 +224,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();
@@ -282,7 +262,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";
@@ -306,6 +286,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) {
         &$errfunc(undef); # flush laststderr
     }
@@ -320,8 +306,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 191f9f5..1dbb0cc 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 ();
 
@@ -95,9 +96,46 @@ for my $test ($caller_alarm_tests->@*) {
     cmp_ok($after, '<', 2.5, "run $test->{name} does not delay the caller's alarm");
 }
 
+# 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