From e7502454b473f88e6d9f349fb44eae97c0096b91 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Tue, 30 May 2017 13:59:16 -0400
Subject: [PATCH] check for nil state in s3 client Get

The S3 client can return (nil, nil) when the remote state doesn't exist.
The caused a nil pointer dereference when checking the payload.MD5
against the expected value.

This can happen if the remote state was manually removed, but the digest
entry was left in the DynamoDB table.
---
 backend/remote-state/s3/client.go      | 14 +++++++++++---
 backend/remote-state/s3/client_test.go | 19 ++++++++++++++++---
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/backend/remote-state/s3/client.go b/backend/remote-state/s3/client.go
index ffa40d3df..25ff52930 100644
--- a/backend/remote-state/s3/client.go
+++ b/backend/remote-state/s3/client.go
@@ -58,11 +58,19 @@ func (c *RemoteClient) Get() (payload *remote.Payload, err error) {
 			return nil, err
 		}
 
+		// If the remote state was manually removed the payload will be nil,
+		// but if there's still a digest entry for that state we will still try
+		// to compare the MD5 below.
+		var digest []byte
+		if payload != nil {
+			digest = payload.MD5
+		}
+
 		// verify that this state is what we expect
 		if expected, err := c.getMD5(); err != nil {
 			log.Printf("[WARNING] failed to fetch state md5: %s", err)
-		} else if len(expected) > 0 && !bytes.Equal(expected, payload.MD5) {
-			log.Printf("[WARNING] state md5 mismatch: expected '%x', got '%x'", expected, payload.MD5)
+		} else if len(expected) > 0 && !bytes.Equal(expected, digest) {
+			log.Printf("[WARNING] state md5 mismatch: expected '%x', got '%x'", expected, digest)
 
 			if testChecksumHook != nil {
 				testChecksumHook()
@@ -74,7 +82,7 @@ func (c *RemoteClient) Get() (payload *remote.Payload, err error) {
 				continue
 			}
 
-			return nil, fmt.Errorf(errBadChecksumFmt, payload.MD5)
+			return nil, fmt.Errorf(errBadChecksumFmt, digest)
 		}
 
 		break
diff --git a/backend/remote-state/s3/client_test.go b/backend/remote-state/s3/client_test.go
index d3cb693b6..d93711c9f 100644
--- a/backend/remote-state/s3/client_test.go
+++ b/backend/remote-state/s3/client_test.go
@@ -167,6 +167,8 @@ func TestRemoteClient_clientMD5(t *testing.T) {
 		"lock_table": bucketName,
 	}).(*Backend)
 
+	createS3Bucket(t, b.s3Client, bucketName)
+	defer deleteS3Bucket(t, b.s3Client, bucketName)
 	createDynamoDBTable(t, b.dynClient, bucketName)
 	defer deleteDynamoDBTable(t, b.dynClient, bucketName)
 
@@ -263,8 +265,8 @@ func TestRemoteClient_stateChecksum(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// put the old state in place of the new, without updating the checksum
-	if err := client2.Put(oldState.Bytes()); err != nil {
+	// put an empty state in place to check for panics during get
+	if err := client2.Put([]byte{}); err != nil {
 		t.Fatal(err)
 	}
 
@@ -278,7 +280,18 @@ func TestRemoteClient_stateChecksum(t *testing.T) {
 	consistencyRetryTimeout = 0
 	consistencyRetryPollInterval = 0
 
-	// fetching the state through client1 should now error out due to a
+	// fetching an empty state through client1 should now error out due to a
+	// mismatched checksum.
+	if _, err := client1.Get(); !strings.HasPrefix(err.Error(), errBadChecksumFmt[:80]) {
+		t.Fatalf("expected state checksum error: got %s", err)
+	}
+
+	// put the old state in place of the new, without updating the checksum
+	if err := client2.Put(oldState.Bytes()); err != nil {
+		t.Fatal(err)
+	}
+
+	// fetching the wrong state through client1 should now error out due to a
 	// mismatched checksum.
 	if _, err := client1.Get(); !strings.HasPrefix(err.Error(), errBadChecksumFmt[:80]) {
 		t.Fatalf("expected state checksum error: got %s", err)