From 41c45c44a96793c470fd9899290bdf4efc88fca6 Mon Sep 17 00:00:00 2001
From: Matthias Frei <matthias.frei@inf.ethz.ch>
Date: Mon, 30 May 2022 15:09:25 +0200
Subject: [PATCH] lint: detect captured loop var in go/defer statement (#4202)

Detect loop variable captured by function in go or defer statement.
The loop variable is reassigned when stepping in the loop, and so
by the time the go routine is run, the value may have changed.

Fix the single occurrence of the pattern.

Update semgrep, as the rules did not appear to work as expected with the
older version.
With this newer version, we now need an (empty) .semgrepignore file, as
by default semgrep ignores all _test.go files, which seems undesirable.
The docker command for semgrep is also changed to explicitly invoke
`semgrep`, as directed by a deprecation warning.

fixes #2164.
---
 .semgrepignore                         |  1 +
 gateway/control/remotemonitor.go       |  1 +
 tools/lint.sh                          |  4 +--
 tools/lint/semgrep/loopvar_capture.yml | 46 ++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 .semgrepignore
 create mode 100644 tools/lint/semgrep/loopvar_capture.yml

diff --git a/.semgrepignore b/.semgrepignore
new file mode 100644
index 0000000000..fea3012e24
--- /dev/null
+++ b/.semgrepignore
@@ -0,0 +1 @@
+# Empty .semgrepignore; the default one skips all _test.go.
diff --git a/gateway/control/remotemonitor.go b/gateway/control/remotemonitor.go
index 31ae12e08a..4b014b1f86 100644
--- a/gateway/control/remotemonitor.go
+++ b/gateway/control/remotemonitor.go
@@ -120,6 +120,7 @@ func (rm *RemoteMonitor) process(ctx context.Context, ias []addr.IA) {
 	logger := log.FromCtx(ctx)
 	newWatchers := make(map[addr.IA]watcherEntry)
 	for _, ia := range ias {
+		ia := ia
 		we, ok := rm.currentWatchers[ia]
 		if ok {
 			// Watcher for the remote IA exists. Move it to the new map of
diff --git a/tools/lint.sh b/tools/lint.sh
index 2d2654f9f3..10d79c15fa 100755
--- a/tools/lint.sh
+++ b/tools/lint.sh
@@ -89,8 +89,8 @@ md_lint() {
 semgrep_lint() {
     lint_header "semgrep"
     lint_step "custom rules"
-    docker run --rm -v "${PWD}:/src" returntocorp/semgrep@sha256:8b0735959a6eb737aa945f4d591b6db23b75344135d74c3021b7d427bd317a66 \
-        --config=/src/tools/lint/semgrep --error
+    docker run --rm -v "${PWD}:/src" returntocorp/semgrep@sha256:3bef9d533a44e6448c43ac38159d61fad89b4b57f63e565a8a55ca265273f5ba \
+       semgrep --config=/src/tools/lint/semgrep --error
 }
 
 openapi_lint() {
diff --git a/tools/lint/semgrep/loopvar_capture.yml b/tools/lint/semgrep/loopvar_capture.yml
new file mode 100644
index 0000000000..07aa39658d
--- /dev/null
+++ b/tools/lint/semgrep/loopvar_capture.yml
@@ -0,0 +1,46 @@
+rules:
+  - id: loopvar_capture
+    patterns:
+      - pattern-either:
+        - pattern-inside: |
+            for $X := ...; ...; <... $X ...> { ... }
+        - pattern-inside: |
+            for $X := range $SLICE { ... }
+        - pattern-inside: |
+            for $X, ... := range $SLICE { ... }
+        - pattern-inside: |
+            for ..., $X := range $SLICE { ... }
+      - pattern-either:
+        - pattern: |
+            go func(...){ ...; <... $X ...>; ... }(...)
+        - pattern: |
+            defer func(...){ ...; <... $X ...>; ... }(...)
+        - pattern: |
+            $ERRGROUP.Go(func() error { ...; <... $X ...>; ...})
+
+      # These additional patterns are a workaround to exclude certain false
+      # positives, which appear to be caused by semgrep not fully handling
+      # multi-variable assignment.
+      # These two patterns exclude this case:
+      #   for i, x := range values {
+      #      i, x := i, x
+      #      go func() { fmt.Println(i,x) }()
+      #   }
+      - pattern-not-inside: |
+            for ... { $X, ... :=  $X, ...;  ... }
+      - pattern-not-inside: |
+            for ... { ..., $X :=  ..., $X; ... }
+      # This pattern excludes odd matches of the _ variable,
+      # for example
+      #   for _, x := range values {
+      #      go func() { v, _ := foo() }()
+      #   }
+      - metavariable-pattern:
+          metavariable: $X
+          patterns:
+            - pattern-not: _
+    message: |
+      Captured loop variable `$X` in go or defer statement
+    severity: WARNING
+    languages:
+      - go