fix(secmem-lint): match receivers and params by object, not by name - #69
Open
deadpoets wants to merge 1 commit into
Open
fix(secmem-lint): match receivers and params by object, not by name#69deadpoets wants to merge 1 commit into
deadpoets wants to merge 1 commit into
Conversation
Two checks were keyed on the wrong thing. R1 (reentrancy) required the receiver to be a plain identifier, so it silently did nothing for s.buf.WithBytes(...) — and a struct field is how most programs hold a buffer. Receivers are now compared as identity chains, covering s.buf and s.inner.buf. Index expressions and calls are still left alone: bufs[i] and bufs[j] look alike and need not be the same buffer. The goroutine-capture check matched borrowed params by name, so a goroutine declaring its own b was reported as leaking the borrowed slice. It now resolves each identifier to its types.Object. Fixtures for both were confirmed to fail against the unfixed analyzer: two missing diagnostics, one unexpected. Review findings secmem-lint/reentrancy.go:27 and analyzer.go:231.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both remaining secmem-lint findings were one root cause: the pass matched by
name and AST shape where it should match by
types.Object.reentrancy.go:27(MEDIUM) — R1 required the receiver to be a plainidentifier, so it silently did nothing for
s.buf.WithBytes(...). A structfield is how most programs hold a buffer, so the check was off for the common
case. Receivers are now compared as identity chains, covering
s.bufands.inner.buf.analyzer.go:231(LOW) — the goroutine-capture scan matched borrowedparams by name, so a goroutine declaring its own
bwas reported as leakingthe borrowed slice.
Index expressions and calls are deliberately still skipped:
bufs[i]andbufs[j]are written alike and need not be the same buffer, and a falsepositive here blocks a build.
Verified against the unfixed analyzer
New fixtures fail on the old code exactly as intended — two missing diagnostics
and one unexpected:
With the fix: analyzer tests,
go vet, golangci-lint, and the core +secmem-crypto dogfood all clean.
This clears the last MEDIUM from the review; 18 rows remain, all LOW/INFO.