From 152b0dba06de05c5d2fa49d6ae11e12ee0d57397 Mon Sep 17 00:00:00 2001
From: Carolyn Van Slyck <me@carolynvanslyck.com>
Date: Fri, 18 Feb 2022 15:19:07 -0600
Subject: [PATCH] Fix panic when op.Err is unset in Docker driver

The op.Err field was added after the docker driver was first written. I
noticed that when the caller of the docker driver doesn't set op.Err it
causes a panic when we attach to the container's logs.

This fixes our defaulting of where we write errors so that we are never
writing to a nil writer accidentally.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
---
 driver/docker/docker.go                  |  9 ++++--
 driver/docker/docker_integration_test.go | 36 ++++++++++++++++++++----
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/driver/docker/docker.go b/driver/docker/docker.go
index 5cf99702..c6ece0cb 100644
--- a/driver/docker/docker.go
+++ b/driver/docker/docker.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"os"
 	unix_path "path"
 	"strconv"
 	"strings"
@@ -244,14 +245,18 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) {
 		return driver.OperationResult{}, fmt.Errorf("unable to retrieve logs: %v", err)
 	}
 	var (
-		stdout = op.Out
-		stderr = op.Err
+		stdout io.Writer = os.Stdout
+		stderr io.Writer = os.Stderr
 	)
 	if d.containerOut != nil {
 		stdout = d.containerOut
+	} else if op.Out != nil {
+		stdout = op.Out
 	}
 	if d.containerErr != nil {
 		stderr = d.containerErr
+	} else if op.Err != nil {
+		stderr = op.Err
 	}
 	go func() {
 		defer attach.Close()
diff --git a/driver/docker/docker_integration_test.go b/driver/docker/docker_integration_test.go
index eed8e394..08bce731 100644
--- a/driver/docker/docker_integration_test.go
+++ b/driver/docker/docker_integration_test.go
@@ -5,6 +5,7 @@ package docker
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"os"
 	"testing"
 
@@ -39,7 +40,7 @@ func TestDockerDriver_Run(t *testing.T) {
 		}
 	}
 
-	runDriverTest(t, image)
+	runDriverTest(t, image, false)
 }
 
 // Test running a bundle where the invocation image runs as a nonroot user
@@ -51,10 +52,10 @@ func TestDockerDriver_RunNonrootInvocationImage(t *testing.T) {
 		},
 	}
 
-	runDriverTest(t, image)
+	runDriverTest(t, image, false)
 }
 
-func runDriverTest(t *testing.T, image bundle.InvocationImage) {
+func runDriverTest(t *testing.T, image bundle.InvocationImage, skipValidations bool) {
 	op := &driver.Operation{
 		Installation: "example",
 		Action:       "install",
@@ -90,10 +91,12 @@ func runDriverTest(t *testing.T, image bundle.InvocationImage) {
 	}
 
 	docker := &Driver{}
-	docker.SetContainerOut(op.Out) // Docker driver writes container stdout to driver.containerOut.
-	docker.SetContainerOut(op.Err)
 	opResult, err := docker.Run(op)
 
+	if skipValidations {
+		return
+	}
+
 	require.NoError(t, err)
 	assert.Equal(t, "Install action\n\nListing inputs\ninput1\n\nGenerating outputs\nAction install complete for example\n", output.String())
 	assert.Equal(t, 2, len(opResult.Outputs), "Expecting two output files")
@@ -103,6 +106,29 @@ func runDriverTest(t *testing.T, image bundle.InvocationImage) {
 	}, opResult.Outputs)
 }
 
+func TestDockerDriver_NoOpErrSet(t *testing.T) {
+	// Validate that when op.Err is not set that we print to stderr
+	r, w, err := os.Pipe()
+	require.NoError(t, err)
+	origStderr := os.Stderr
+	defer func() {
+		os.Stderr = origStderr
+	}()
+	os.Stderr = w
+
+	image := bundle.InvocationImage{
+		BaseImage: defaultBaseImage,
+	}
+	image.Image += "-oops-this-does-not-exist"
+	runDriverTest(t, image, true)
+	w.Close()
+
+	var buf bytes.Buffer
+	_, err = io.Copy(&buf, r)
+	require.NoError(t, err)
+	assert.Contains(t, buf.String(), "Unable to find image 'carolynvs/example-outputs:v1.0.0-oops-this-does-not-exist'")
+}
+
 func TestDriver_Run_CaptureOutput(t *testing.T) {
 	image := bundle.InvocationImage{
 		BaseImage: bundle.BaseImage{