Subprocess error handling: Initialise $? to -1

When system(3perl) fails due to syscall error, it sets only $!.  When
it succeeds it sets only $? and sometimes trashes $!.  Conversely,
close of a popened filehandle always sets both in all cases.

Document this in a comment.

So when using system and relying on $?/$! (rather than looking at
system's return value), such as when about to use failedcmd, it's
necessary to initialise $? to -1.

Fix the three call sites where system might be followed by failedcmd
but this wasn't done.

diff --git a/Debian/Dgit.pm b/Debian/Dgit.pm
index 2555812c46abcdd1c617e718b0cb1c5643e8e0ba..aa0c5a3d675cbf1d7418c222a2e5a9f7a6819182 100644 (file)
--- a/Debian/Dgit.pm
+++ b/Debian/Dgit.pm
@@ -186,8 +186,19 @@ sub waitstatusmsg () {
 }
 
 sub failedcmd {
+    # Expects $!,$? as set by close - see below.
+    # To use with system(), set $?=-1 first.
+    #
+    # Actual behaviour of perl operations:
+    #   success              $!==0       $?==0       close of piped open
+    #   program failed       $!==0       $? >0       close of piped open
+    #   syscall failure      $! >0       $?=-1       close of piped open
+    #   failure              $! >0       unchanged   close of something else
+    #   success              trashed     $?==0       system
+    #   program failed       trashed     $? >0       system
+    #   syscall failure      $! >0       unchanged   system
     { local ($!); printcmd \*STDERR, _us().": failed command:", @_ or die $!; };
-    if ($!) {
+    if ($? < 0) {
        fail "failed to fork/exec: $!";
     } elsif ($?) {
        fail "subprocess ".waitstatusmsg();

diff --git a/dgit b/dgit
index 54a1c154f30afa153951a5c6e7087e0a6f6b08bf..1007d81698ee7a4d40a2ebce667131dbaddc9f2d 100755 (executable)
--- a/dgit
+++ b/dgit
@@ -398,7 +398,7 @@ our ($dscdata,$dscurl,$dsc,$dsc_checked,$skew_warning_vsn);
 
 sub runcmd {
     debugcmd "+",@_;
-    $!=0; $?=0;
+    $!=0; $?=-1;
     failedcmd @_ if system @_;
 }
 
@@ -1782,9 +1782,9 @@ sub check_not_dirty () {
 
     my @cmd = (@git, qw(diff --quiet HEAD));
     debugcmd "+",@cmd;
-    $!=0; $?=0; system @cmd;
-    return if !$! && !$?;
-    if (!$! && $?==256) {
+    $!=0; $?=-1; system @cmd;
+    return if !$?;
+    if ($?==256) {
        fail "working tree is dirty (does not match HEAD)";
     } else {
        failedcmd @cmd;
@@ -2015,7 +2015,7 @@ END
     my $diffopt = $debuglevel>0 ? '--exit-code' : '--quiet';
     my @diffcmd = (@git, qw(diff), $diffopt, $tree);
     debugcmd "+",@diffcmd;
-    $!=0; $?=0;
+    $!=0; $?=-1;
     my $r = system @diffcmd;
     if ($r) {
        if ($r==256) {