From 1bc5c718b428d814c275ec22cb962e436b8ada6b Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Thu, 15 Sep 2022 10:35:08 -0700
Subject: [PATCH] Data race fixes in tests and a new semgrep rule (#14594)

* test: don't use loop vars in goroutines

fixes a data race in the test

* test: copy objects in statestore before mutating

fixes data race in test

* test: @lgfa29's segmgrep rule for loops/goroutines

Found 2 places where we were improperly using loop variables inside
goroutines.
---
 .semgrep/loopclosure.yml            | 28 ++++++++++++++++++++++++++++
 drivers/docker/driver_test.go       |  3 ++-
 nomad/client_agent_endpoint_test.go |  3 ++-
 nomad/client_alloc_endpoint_test.go |  4 ++--
 4 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 .semgrep/loopclosure.yml

diff --git a/.semgrep/loopclosure.yml b/.semgrep/loopclosure.yml
new file mode 100644
index 000000000..967376127
--- /dev/null
+++ b/.semgrep/loopclosure.yml
@@ -0,0 +1,28 @@
+rules:
+  - id: loopclosure
+    patterns:
+      - pattern-inside: |
+          for $A, $B := range $C {
+            ...
+          }
+      - pattern-inside: |
+          go func() {
+            ...
+          }()
+      - pattern-not-inside: |
+          go func(..., $B, ...) {
+            ...
+          }(..., $B, ...)
+      - pattern-not-inside: |
+          go func() {
+            ...
+            for ... {
+              ...
+            }
+            ...
+          }()
+      - pattern: $B
+    message: Loop variable $B used inside goroutine
+    languages:
+      - go
+    severity: WARNING
diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go
index 8a76b01eb..2e9e396c2 100644
--- a/drivers/docker/driver_test.go
+++ b/drivers/docker/driver_test.go
@@ -2979,7 +2979,8 @@ func TestDockerDriver_StopSignal(t *testing.T) {
 		},
 	}
 
-	for _, c := range cases {
+	for i := range cases {
+		c := cases[i]
 		t.Run(c.name, func(t *testing.T) {
 			taskCfg := newTaskConfig(c.variant, []string{"sleep", "9901"})
 
diff --git a/nomad/client_agent_endpoint_test.go b/nomad/client_agent_endpoint_test.go
index 2b7578600..1a7eb75de 100644
--- a/nomad/client_agent_endpoint_test.go
+++ b/nomad/client_agent_endpoint_test.go
@@ -228,7 +228,8 @@ func TestMonitor_Monitor_RemoteServer(t *testing.T) {
 		},
 	}
 
-	for _, tc := range cases {
+	for i := range cases {
+		tc := cases[i]
 		t.Run(tc.desc, func(t *testing.T) {
 			require := require.New(t)
 
diff --git a/nomad/client_alloc_endpoint_test.go b/nomad/client_alloc_endpoint_test.go
index 68cbfe276..5bd2e2a40 100644
--- a/nomad/client_alloc_endpoint_test.go
+++ b/nomad/client_alloc_endpoint_test.go
@@ -162,7 +162,7 @@ func TestClientAllocations_GarbageCollectAll_OldNode(t *testing.T) {
 	// Test for an old version error
 	node := mock.Node()
 	node.Attributes["nomad.version"] = "0.7.1"
-	require.Nil(state.UpsertNode(nstructs.MsgTypeTestSetup, 1005, node))
+	require.Nil(state.UpsertNode(nstructs.MsgTypeTestSetup, 1005, node.Copy()))
 
 	req := &nstructs.NodeSpecificRequest{
 		NodeID:       node.ID,
@@ -546,7 +546,7 @@ func TestClientAllocations_Stats_OldNode(t *testing.T) {
 	// Test for an old version error
 	node := mock.Node()
 	node.Attributes["nomad.version"] = "0.7.1"
-	require.Nil(state.UpsertNode(nstructs.MsgTypeTestSetup, 1005, node))
+	require.Nil(state.UpsertNode(nstructs.MsgTypeTestSetup, 1005, node.Copy()))
 
 	alloc := mock.Alloc()
 	alloc.NodeID = node.ID