Skip to content

fix(analyzer): per-package rule instantiation eliminates concurrent map crash#1589

Merged
ccojocar merged 1 commit intosecurego:masterfrom
ravisastryk:fix/1586-per-package-rule-instantiation
Mar 9, 2026
Merged

fix(analyzer): per-package rule instantiation eliminates concurrent map crash#1589
ccojocar merged 1 commit intosecurego:masterfrom
ravisastryk:fix/1586-per-package-rule-instantiation

Conversation

@ravisastryk
Copy link
Copy Markdown
Contributor

Problem

Analyzer.Process fans out to gosec.concurrency goroutines. Each goroutine calls checkRules(pkg)ast.Walkrule.Match on the same shared rule instances registered once in LoadRules. Rules that maintain per-package state in struct fields — specifically readfile (G304) with its cleanedVar and joinedVar maps — are written and read concurrently across goroutines, producing a fatal map race:

fatal error: concurrent map read and map write
goroutine N: rules.(*readfile).Match → trackCleanAssign → r.cleanedVar[v] = …  (WRITE)
goroutine M: rules.(*readfile).Match → isFilepathClean  → _, ok := r.cleanedVar[v] (READ)

There is also a pre-existing secondary bug: the maps are never cleared between packages, so a variable resolved as "cleaned" in package foo remains in cleanedVar when package bar is later analysed, silently suppressing findings.

Approach

This fix is a direct elaboration of Option C from issue #1586 — creating a fresh rule instance per invocation of checkRules - with two refinements:

  1. Granularity is per package walk, not per goroutine. A single goroutine processes multiple packages sequentially; per-goroutine instances would still share state across those packages.
  2. Mechanism reuses existing RuleBuilder functions. LoadRules already receives map[string]RuleBuilder. Storing these and re-invoking them in checkRules produces a complete, fresh RuleSet without duplicating any constructor logic and without modifying any rule file.

Changes (analyzer.go only — no rule files modified)

Location Change
Analyzer struct Add ruleBuilders map[string]RuleBuilder and ruleSuppressed map[string]bool
LoadRules() Save the incoming arguments before the existing registration loop (loop unchanged)
buildPackageRuleset() New private method — re-invokes every stored builder, returns a fresh RuleSet
checkRules() Calls buildPackageRuleset(), passes result into astVisitor via new ruleset *RuleSet field
astVisitor struct Add ruleset *RuleSet field
activeRuleset() New helper on astVisitor — returns the package-local ruleset, falls back to shared gosec.ruleset for direct CheckRules callers (backward-compatible)
Visit() Uses activeRuleset() instead of gosec.ruleset directly
Reset() Nils out ruleBuilders and ruleSuppressed

Properties

  • No public API changes. CheckRules, LoadRules, Process, Report, Reset all have identical signatures.
  • Backward compatible. Direct callers of the public CheckRules method (without going through Process) continue to use the shared gosec.ruleset via the activeRuleset() fallback.
  • No synchronisation inside rules. Each concurrent worker owns completely independent rule instances — no mutex, no sync.Map, no coordination needed now or in the future.
  • Fixes cross-package contamination as a side-effect. Each package walk starts with fresh maps, so state from one package cannot suppress findings in another.
  • Future-proof. Any rule that needs per-package mutable state is automatically isolated with no extra work required from the rule author.

…ap crash

Analyzer.Process fans out package walks across goroutines that previously
shared a single rule set. Rules with mutable per-package state — specifically
readfile (G304), whose cleanedVar/joinedVar maps are written and read across
goroutines — raced fatally under concurrency. The fix stores the RuleBuilder
functions after LoadRules and calls buildPackageRuleset() at the start of each
checkRules invocation, giving every concurrent worker its own freshly allocated
rule instances with no shared mutable state and no locks required. Stale map
entries no longer leak across package boundaries, closing a secondary
false-negative bug as a side-effect.

fixes: 1586
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.83%. Comparing base (c709ed8) to head (50ca06c).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
+ Coverage   80.72%   80.83%   +0.10%     
==========================================
  Files         104      104              
  Lines        9882     9901      +19     
==========================================
+ Hits         7977     8003      +26     
+ Misses       1418     1412       -6     
+ Partials      487      486       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ravisastryk ravisastryk marked this pull request as ready for review March 9, 2026 03:33
@ccojocar ccojocar merged commit cbf46b8 into securego:master Mar 9, 2026
9 checks passed
@ravisastryk ravisastryk deleted the fix/1586-per-package-rule-instantiation branch March 9, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants