Skip to content

Commit

Permalink
lint: detect captured loop var in go/defer statement (#4202)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
matzf authored May 30, 2022
1 parent 7357f03 commit 41c45c4
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 2 deletions.
1 change: 1 addition & 0 deletions .semgrepignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# Empty .semgrepignore; the default one skips all _test.go.
1 change: 1 addition & 0 deletions gateway/control/remotemonitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tools/lint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
46 changes: 46 additions & 0 deletions tools/lint/semgrep/loopvar_capture.yml
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 41c45c4

Please sign in to comment.