From d1e35a3f7c1b4fcad7593f9d5136e2cf51d7e112 Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Mon, 16 May 2022 16:45:16 -0400
Subject: [PATCH] configs: Fix module loader nil pointer panic

In configurations which have already been initialized, updating the
source of a non-root module call to an invalid value could cause a nil
pointer panic. This commit fixes the bug and adds test coverage.
---
 internal/configs/configload/loader_load.go    |  7 +++++--
 .../configload/loader_snapshot_test.go        | 21 +++++++++++++++++++
 internal/configs/configload/loader_test.go    |  2 +-
 .../.terraform/modules/modules.json           |  1 +
 .../foo/bar/main.tf                           |  3 +++
 .../already-installed-now-invalid/foo/main.tf |  3 +++
 .../already-installed-now-invalid/root.tf     |  3 +++
 7 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 internal/configs/configload/testdata/already-installed-now-invalid/.terraform/modules/modules.json
 create mode 100644 internal/configs/configload/testdata/already-installed-now-invalid/foo/bar/main.tf
 create mode 100644 internal/configs/configload/testdata/already-installed-now-invalid/foo/main.tf
 create mode 100644 internal/configs/configload/testdata/already-installed-now-invalid/root.tf

diff --git a/internal/configs/configload/loader_load.go b/internal/configs/configload/loader_load.go
index 9ae440274035..1ca26814a273 100644
--- a/internal/configs/configload/loader_load.go
+++ b/internal/configs/configload/loader_load.go
@@ -60,8 +60,11 @@ func (l *Loader) moduleWalkerLoad(req *configs.ModuleRequest) (*configs.Module,
 
 	var diags hcl.Diagnostics
 
-	// Check for inconsistencies between manifest and config
-	if req.SourceAddr.String() != record.SourceAddr {
+	// Check for inconsistencies between manifest and config.
+
+	// We ignore a nil SourceAddr here, which represents a failure during
+	// configuration parsing, and will be reported in a diagnostic elsewhere.
+	if req.SourceAddr != nil && req.SourceAddr.String() != record.SourceAddr {
 		diags = append(diags, &hcl.Diagnostic{
 			Severity: hcl.DiagError,
 			Summary:  "Module source has changed",
diff --git a/internal/configs/configload/loader_snapshot_test.go b/internal/configs/configload/loader_snapshot_test.go
index 24e41bafc462..cf1b9b26f7a4 100644
--- a/internal/configs/configload/loader_snapshot_test.go
+++ b/internal/configs/configload/loader_snapshot_test.go
@@ -1,6 +1,7 @@
 package configload
 
 import (
+	"os"
 	"path/filepath"
 	"reflect"
 	"testing"
@@ -71,6 +72,26 @@ module "child_b" {
 
 }
 
+func TestLoadConfigWithSnapshot_invalidSource(t *testing.T) {
+	fixtureDir := filepath.Clean("testdata/already-installed-now-invalid")
+
+	old, _ := os.Getwd()
+	os.Chdir(fixtureDir)
+	defer os.Chdir(old)
+
+	loader, err := NewLoader(&Config{
+		ModulesDir: ".terraform/modules",
+	})
+	if err != nil {
+		t.Fatalf("unexpected error from NewLoader: %s", err)
+	}
+
+	_, _, diags := loader.LoadConfigWithSnapshot(".")
+	if !diags.HasErrors() {
+		t.Error("LoadConfigWithSnapshot succeeded; want errors")
+	}
+}
+
 func TestSnapshotRoundtrip(t *testing.T) {
 	fixtureDir := filepath.Clean("testdata/already-installed")
 	loader, err := NewLoader(&Config{
diff --git a/internal/configs/configload/loader_test.go b/internal/configs/configload/loader_test.go
index 7b3483b4a354..396a449b419f 100644
--- a/internal/configs/configload/loader_test.go
+++ b/internal/configs/configload/loader_test.go
@@ -14,7 +14,7 @@ func assertNoDiagnostics(t *testing.T, diags hcl.Diagnostics) bool {
 
 func assertDiagnosticCount(t *testing.T, diags hcl.Diagnostics, want int) bool {
 	t.Helper()
-	if len(diags) != 0 {
+	if len(diags) != want {
 		t.Errorf("wrong number of diagnostics %d; want %d", len(diags), want)
 		for _, diag := range diags {
 			t.Logf("- %s", diag)
diff --git a/internal/configs/configload/testdata/already-installed-now-invalid/.terraform/modules/modules.json b/internal/configs/configload/testdata/already-installed-now-invalid/.terraform/modules/modules.json
new file mode 100644
index 000000000000..32a4ace575da
--- /dev/null
+++ b/internal/configs/configload/testdata/already-installed-now-invalid/.terraform/modules/modules.json
@@ -0,0 +1 @@
+{"Modules":[{"Key":"","Source":"","Dir":"."},{"Key":"foo","Source":"./foo","Dir":"foo"},{"Key":"foo.bar","Source":"./bar","Dir":"foo/bar"}]}
\ No newline at end of file
diff --git a/internal/configs/configload/testdata/already-installed-now-invalid/foo/bar/main.tf b/internal/configs/configload/testdata/already-installed-now-invalid/foo/bar/main.tf
new file mode 100644
index 000000000000..48b5e2e06730
--- /dev/null
+++ b/internal/configs/configload/testdata/already-installed-now-invalid/foo/bar/main.tf
@@ -0,0 +1,3 @@
+output "hello" {
+  value = "Hello from foo/bar"
+}
diff --git a/internal/configs/configload/testdata/already-installed-now-invalid/foo/main.tf b/internal/configs/configload/testdata/already-installed-now-invalid/foo/main.tf
new file mode 100644
index 000000000000..9fba57235c20
--- /dev/null
+++ b/internal/configs/configload/testdata/already-installed-now-invalid/foo/main.tf
@@ -0,0 +1,3 @@
+module "bar" {
+  source = "${path.module}/bar"
+}
diff --git a/internal/configs/configload/testdata/already-installed-now-invalid/root.tf b/internal/configs/configload/testdata/already-installed-now-invalid/root.tf
new file mode 100644
index 000000000000..020494e84d65
--- /dev/null
+++ b/internal/configs/configload/testdata/already-installed-now-invalid/root.tf
@@ -0,0 +1,3 @@
+module "foo" {
+  source  = "./foo"
+}