fix(runner): add mutex to testAndSet to prevent race condition#2439
fix(runner): add mutex to testAndSet to prevent race condition#2439Mzack9999 merged 2 commits intoprojectdiscovery:devfrom
Conversation
testAndSet performs a non-atomic read-then-write sequence: 1. seen(k) - check if key exists 2. setSeen(k) - set the key Without synchronization, two concurrent goroutines can both pass the seen() check before either calls setSeen(), causing duplicate processing of the same target. This race can occur in processTargets() where multiple goroutines call testAndSet() for discovered TLS SubjectAN/SubjectCN values. The fix adds a sync.Mutex to serialize testAndSet operations.
Neo - PR Security ReviewNo security issues found Highlights
Comment |
WalkthroughAdded a mutex field to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mzack9999
left a comment
There was a problem hiding this comment.
The PR should point to the dev branch
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runner/runner.go (1)
1705-1714:⚠️ Potential issue | 🟠 MajorDon't disable
CSPProbeon the shared scan options.
processItemcommonly passes a shared*ScanOptionsintoprocess, so this write is visible to sibling goroutines. The first response that reaches this branch can turn CSP expansion off for unrelated in-flight targets, and the read/write onscanopts.CSPProbeitself races. Clone the options for the recursive follow-up and clear the flag on that clone only.Suggested fix
if scanopts.CSPProbe && result.CSPData != nil { - scanopts.CSPProbe = false + childScanopts := scanopts.Clone() + childScanopts.CSPProbe = false domains := result.CSPData.Domains domains = append(domains, result.CSPData.Fqdns...) for _, tt := range domains { if !r.testAndSet(tt) { continue } - r.process(tt, wg, hp, protocol, scanopts, output) + r.process(tt, wg, hp, protocol, childScanopts, output) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runner/runner.go` around lines 1705 - 1714, The code currently clears the shared flag scanopts.CSPProbe before spawning recursive work, causing a data race and turning off CSP expansion for other goroutines; instead, create a shallow copy of the ScanOptions (e.g., newOpts := *scanopts; newOpts.CSPProbe = false) and pass &newOpts into r.process so only the recursive follow-up sees the flag cleared, leaving the original scanopts unmodified; update the branch handling CSP expansion (the block that reads scanopts.CSPProbe and calls r.process) to use the cloned options when invoking r.process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@runner/runner.go`:
- Around line 1705-1714: The code currently clears the shared flag
scanopts.CSPProbe before spawning recursive work, causing a data race and
turning off CSP expansion for other goroutines; instead, create a shallow copy
of the ScanOptions (e.g., newOpts := *scanopts; newOpts.CSPProbe = false) and
pass &newOpts into r.process so only the recursive follow-up sees the flag
cleared, leaving the original scanopts unmodified; update the branch handling
CSP expansion (the block that reads scanopts.CSPProbe and calls r.process) to
use the cloned options when invoking r.process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb4fbdaf-34fe-4960-9a4f-c1fc2dbf4afc
📒 Files selected for processing (2)
runner/runner.gorunner/runner_test.go
Summary
Fixes a race condition in
testAndSetwhere the read-check-then-write sequence is not atomic.Problem
testAndSetperforms:seen(k)- check if key existssetSeen(k)- set the keyWithout synchronization, two concurrent goroutines can both pass the
seen()check before either callssetSeen(), causing duplicate processing of the same target.Impact
This race occurs in
processTargets()where multiple goroutines calltestAndSet()for discovered TLS SubjectAN/SubjectCN values (lines 1692, 1697, 1706, 1746, 1751).When the race triggers:
Fix
Add a
sync.Mutexto the Runner struct to serializetestAndSetoperations.Summary by CodeRabbit
Bug Fixes
Tests