fix: add Snapshot method to prevent InFlight map race condition#7026
fix: add Snapshot method to prevent InFlight map race condition#7026n3integration wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces multiple per-field resume checks with a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
pkg/types/resume.go (1)
101-126:⚠️ Potential issue | 🟡 Minor
Compile()writesresumeInfofields without holdingresumeInfo's own lock — inconsistent withSnapshot()'s locking contract.
Compile()acquires onlyresumeCfg.Lock()(the outer struct lock) and writes directly toresumeInfo.SkipUnder,resumeInfo.DoAbove,resumeInfo.InFlight, andresumeInfo.Completed.Snapshot()acquiresresumeInfo.RLock()for its reads. Because these are orthogonal mutexes, a concurrentCompile()andSnapshot()call on the sameResumeInfoinstance are not synchronized with each other, which is a data race.Today this is safe only because
Compile()is called before execution begins. However, the locking discipline is inconsistent: anyone who introduces an execution path whereCompile()overlaps with active scanning will trigger a race thatSnapshot()'sRLockcannot prevent.Fix
Compile()to acquire eachresumeInfo's own write lock when mutating its fields:🔒 Proposed fix for `Compile()`
func (resumeCfg *ResumeCfg) Compile() { resumeCfg.Lock() defer resumeCfg.Unlock() for _, resumeInfo := range resumeCfg.ResumeFrom { + resumeInfo.Lock() if resumeInfo.Completed && len(resumeInfo.InFlight) > 0 { resumeInfo.InFlight = make(map[uint32]struct{}) } min := uint32(math.MaxUint32) max := uint32(0) for index := range resumeInfo.InFlight { if index < min { min = index } if index > max { max = index } } resumeInfo.Repeat = map[uint32]struct{}{} for index := range resumeInfo.InFlight { resumeInfo.Repeat[index] = struct{}{} } resumeInfo.SkipUnder = min resumeInfo.DoAbove = max + resumeInfo.Unlock() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/resume.go` around lines 101 - 126, Compile() currently mutates ResumeInfo fields under only resumeCfg's lock, causing races with Snapshot() which uses resumeInfo.RLock(); fix by acquiring the per-item write lock around all mutations: inside ResumeCfg.Compile(), for each resumeInfo in resumeCfg.ResumeFrom call resumeInfo.Lock() before changing resumeInfo.InFlight, resumeInfo.Repeat, resumeInfo.SkipUnder, resumeInfo.DoAbove, resumeInfo.Completed and call resumeInfo.Unlock() immediately after (do not use defer inside the loop); keep the outer resumeCfg.Lock() if desired but ensure the per-resumeInfo Lock/Unlock brackets all direct reads/writes to that resumeInfo to prevent data races.
🧹 Nitpick comments (1)
pkg/types/resume.go (1)
39-47: Consider named return values forSnapshotto self-document the return order.The four-position unnamed signature
(bool, uint32, uint32, bool)is opaque at call sites; callers (and future maintainers) must consult the implementation to know that the twobools meancompletedandisInFlight, not some other ordering.♻️ Proposed naming improvement
-func (resumeInfo *ResumeInfo) Snapshot(index uint32) (bool, uint32, uint32, bool) { +func (resumeInfo *ResumeInfo) Snapshot(index uint32) (completed bool, skipUnder uint32, doAbove uint32, isInFlight bool) { resumeInfo.RLock() defer resumeInfo.RUnlock() - completed := resumeInfo.Completed - skipUnder := resumeInfo.SkipUnder - doAbove := resumeInfo.DoAbove - _, isInFlight := resumeInfo.InFlight[index] - return completed, skipUnder, doAbove, isInFlight + completed = resumeInfo.Completed + skipUnder = resumeInfo.SkipUnder + doAbove = resumeInfo.DoAbove + _, isInFlight = resumeInfo.InFlight[index] + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/resume.go` around lines 39 - 47, The Snapshot function's unnamed four-value return makes call sites unclear; change its signature to use named return values (e.g., completed bool, skipUnder uint32, doAbove uint32, isInFlight bool) and update the function to assign those names from ResumeInfo.Completed, ResumeInfo.SkipUnder, ResumeInfo.DoAbove and the InFlight check, then use a bare return; reference the Snapshot method and the ResumeInfo fields Completed, SkipUnder, DoAbove and InFlight when making the change.
🤖 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 `@pkg/types/resume.go`:
- Around line 101-126: Compile() currently mutates ResumeInfo fields under only
resumeCfg's lock, causing races with Snapshot() which uses resumeInfo.RLock();
fix by acquiring the per-item write lock around all mutations: inside
ResumeCfg.Compile(), for each resumeInfo in resumeCfg.ResumeFrom call
resumeInfo.Lock() before changing resumeInfo.InFlight, resumeInfo.Repeat,
resumeInfo.SkipUnder, resumeInfo.DoAbove, resumeInfo.Completed and call
resumeInfo.Unlock() immediately after (do not use defer inside the loop); keep
the outer resumeCfg.Lock() if desired but ensure the per-resumeInfo Lock/Unlock
brackets all direct reads/writes to that resumeInfo to prevent data races.
---
Nitpick comments:
In `@pkg/types/resume.go`:
- Around line 39-47: The Snapshot function's unnamed four-value return makes
call sites unclear; change its signature to use named return values (e.g.,
completed bool, skipUnder uint32, doAbove uint32, isInFlight bool) and update
the function to assign those names from ResumeInfo.Completed,
ResumeInfo.SkipUnder, ResumeInfo.DoAbove and the InFlight check, then use a bare
return; reference the Snapshot method and the ResumeInfo fields Completed,
SkipUnder, DoAbove and InFlight when making the change.
n3integration
changed the title
1 participant
Proposed changes
There appears to be a race condition accessing the
InFlightmap within theResumeInfostruct that can lead to a runtime panic. This change wraps access to the struct's map usingRLock()/RUnlock()calls to serialize read/write access across goroutines.Proof
Checklist
Summary by CodeRabbit