From bdad828f15c470cdd22c31acc29fc00c15d6a973 Mon Sep 17 00:00:00 2001
From: Tim Gross <tim@0x74696d.com>
Date: Thu, 1 Dec 2016 13:39:14 -0500
Subject: [PATCH] Fix null-pointer exception in error handling of RunAndWait

`Cmd.Process.Wait` does not guarantee that the `ProcessState` struct it returns
is non-nil if the error it returns is non-nil, because we might have an invalid
PID or a `waitpid` syscall error (ECHILD or EINTR).

We'll check ProcessState struct first b/c it might have the process error code
and this is going to be the common case, but if not then we'll bubble-up the
error. Expanded test coverage for invalid and pruned an unused error handling
branch.
---
 commands/commands.go      | 39 +++++++++++++++++++++------------------
 commands/commands_test.go | 20 +++++++++++++++-----
 2 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/commands/commands.go b/commands/commands.go
index 8827156c..a0780723 100644
--- a/commands/commands.go
+++ b/commands/commands.go
@@ -76,27 +76,31 @@ func RunAndWait(c *Command, fields log.Fields) (int, error) {
 	}
 	log.Debugf("%s.Cmd.Run", c.Name)
 	if err := c.Cmd.Start(); err != nil {
-		if exiterr, ok := err.(*exec.ExitError); ok {
-			if status, ok := exiterr.Sys().(syscall.WaitStatus); ok {
-				return status.ExitStatus(), err
-			}
-		}
-		// Should only happen if we misconfigure or there's some more
-		// serious problem with the underlying open/exec syscalls. But
-		// we'll let the lack of heartbeat tell us if something has gone
-		// wrong to that extent.
+		// the stdlib almost certainly won't include the underlying error
+		// code with this error (if any!). we can try to parse the various
+		// totally undocumented strings that come back but I'd rather do my
+		// own dentistry. a safe bet is that the end user has given us an
+		// invalid executable so we're going to return 127.
 		log.Errorln(err)
-		return 1, err
+		return 127, err
 	}
 	os.Setenv(
 		fmt.Sprintf("CONTAINERPILOT_%s_PID", strings.ToUpper(c.Name)),
 		fmt.Sprintf("%v", c.Cmd.Process.Pid),
 	)
-	state, err := c.Cmd.Process.Wait()
-	if err != nil || (state != nil && !state.Success()) {
-		if status, ok := state.Sys().(syscall.WaitStatus); ok {
+	waitStatus, err := c.Cmd.Process.Wait()
+	if waitStatus != nil && !waitStatus.Success() {
+		if status, ok := waitStatus.Sys().(syscall.WaitStatus); ok {
+			log.Debug(err)
 			return status.ExitStatus(), err
 		}
+	} else if err != nil {
+		if err.Error() == errNoChild {
+			log.Debugf(err.Error())
+			return 0, nil // process exited cleanly before we hit wait4
+		}
+		log.Errorf("%s exited with error: %v", c.Name, err)
+		return 1, err
 	}
 	log.Debugf("%s.RunAndWait end", c.Name)
 	return 0, nil
@@ -206,8 +210,10 @@ func (c *Command) waitForTimeout() error {
 		defer func() { quit <- 0 }()
 	}
 	log.Debugf("%s.run waiting for PID %d: ", c.Name, cmd.Process.Pid)
-	state, err := cmd.Process.Wait()
-	if err != nil {
+	waitStatus, err := cmd.Process.Wait()
+	if waitStatus != nil && !waitStatus.Success() {
+		return fmt.Errorf("%s exited with error", c.Name)
+	} else if err != nil {
 		if err.Error() == errNoChild {
 			log.Debugf(err.Error())
 			return nil // process exited cleanly before we hit wait4
@@ -215,9 +221,6 @@ func (c *Command) waitForTimeout() error {
 		log.Errorf("%s exited with error: %v", c.Name, err)
 		return err
 	}
-	if state != nil && !state.Success() {
-		return fmt.Errorf("%s exited with error", c.Name)
-	}
 
 	log.Debugf("%s.run complete", c.Name)
 	return nil
diff --git a/commands/commands_test.go b/commands/commands_test.go
index 586913e9..c5f2c67c 100644
--- a/commands/commands_test.go
+++ b/commands/commands_test.go
@@ -31,17 +31,19 @@ func BenchmarkRunAndWaitSuccess(b *testing.B) {
 }
 
 func TestRunAndWaitFailed(t *testing.T) {
-	defer func() {
-		if r := recover(); r != nil {
-			t.Errorf("Expected panic but did not.")
-		}
-	}()
 	cmd, _ := NewCommand("./testdata/test.sh failStuff --debug", "0")
 	if exitCode, _ := RunAndWait(cmd, nil); exitCode != 255 {
 		t.Errorf("Expected exit code 255 but got %d", exitCode)
 	}
 }
 
+func TestRunAndWaitInvalidCommand(t *testing.T) {
+	cmd, _ := NewCommand("./testdata/invalidCommand", "0")
+	if exitCode, _ := RunAndWait(cmd, nil); exitCode != 127 {
+		t.Errorf("Expected exit code 127 but got %d", exitCode)
+	}
+}
+
 func TestRunAndWaitForOutput(t *testing.T) {
 
 	cmd, _ := NewCommand("./testdata/test.sh doStuff --debug", "0")
@@ -110,6 +112,14 @@ func TestRunWithTimeoutFailed(t *testing.T) {
 	}
 }
 
+func TestRunWithTimeoutInvalidCommand(t *testing.T) {
+	cmd, _ := NewCommand("./testdata/invalidCommand", "100ms")
+	fields := log.Fields{"process": "test"}
+	if err := RunWithTimeout(cmd, fields); err == nil {
+		t.Errorf("Expected error but got nil")
+	}
+}
+
 func TestEmptyCommand(t *testing.T) {
 	if cmd, err := NewCommand("", "0"); cmd != nil || err == nil {
 		t.Errorf("Expected exit (nil, err) but got %s, %s", cmd, err)