fix: prevent concurrent map writes in MultiPartForm.Decode#7240
fix: prevent concurrent map writes in MultiPartForm.Decode#7240VoidChecksum wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
MultiPartForm stores mutable state (boundary, filesMetadata) but was registered as a singleton shared across all goroutines. When scanning multiple targets concurrently, this caused fatal concurrent map writes. Return a fresh MultiPartForm instance from dataformat.Get() to ensure each goroutine operates on its own state. Fixes projectdiscovery#7028
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughGet(name) now returns a fresh MultiPartForm instance when name == "multipart/form-data"; Encode/Decode use Get(...) and error if Get returns nil. Added concurrent tests to exercise Get/Decode/Encode from multiple goroutines to prevent concurrent map-write panics. 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)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate fatal error: concurrent map writes during concurrent multipart/form-data fuzzing by avoiding shared mutable MultiPartForm instances across goroutines.
Changes:
- Update
dataformat.Get()to return a freshMultiPartForminstance formultipart/form-data. - Add a regression test intended to cover concurrent multipart access.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/fuzz/dataformat/dataformat.go |
Special-cases Get() to return a new MultiPartForm per call to avoid shared mutable state. |
pkg/fuzz/dataformat/multipart_test.go |
Adds a concurrency regression test intended to prevent reintroduction of the crash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| func Get(name string) DataFormat { | ||
| if name == MultiPartFormDataFormat { | ||
| return NewMultiPartForm() | ||
| } | ||
| return dataformats[name] | ||
| } |
| // TestMultiPartFormConcurrentDecode verifies that Get("multipart/form-data") | ||
| // returns independent instances so concurrent goroutines do not trigger a | ||
| // fatal "concurrent map writes" panic (regression test for #7028). | ||
| func TestMultiPartFormConcurrentDecode(t *testing.T) { |
| kv := mapsutil.NewOrderedMap[string, any]() | ||
| kv.Set("key", "value") | ||
| // Encode writes to the instance's filesMetadata; running this | ||
| // concurrently across separate instances must not race. | ||
| _, _ = format.Encode(KVOrderedMap(&kv)) |
![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/fuzz/dataformat/dataformat.go (1)
96-103:⚠️ Potential issue | 🔴 CriticalUpdate
Encode()to useGet()for multipart/form-data to avoid concurrent map writes.
Encode()is called with "multipart/form-data" from the body decode path (body.go:91 → value.go:143, 157), but it directly accessesdataformats[dataformat]instead of usingGet(). This bypasses the fresh-instance logic that prevents concurrent writes tofilesMetadata, contradicting the design documented inGet()'s comments (issue#7028). The function should apply the same fresh-instance pattern for multipart/form-data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/dataformat/dataformat.go` around lines 96 - 103, Encode currently reads dataformats directly and bypasses the fresh-instance logic in Get(), causing concurrent map writes for "multipart/form-data"; update Encode(data KV, dataformat string) to call Get(dataformat) and use the returned encoder instance (handling returned error) especially when dataformat == "multipart/form-data" so the fresh filesMetadata instance is used, then call encoder.Encode(data) on that instance and propagate any error.
🤖 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/fuzz/dataformat/dataformat.go`:
- Around line 96-103: Encode currently reads dataformats directly and bypasses
the fresh-instance logic in Get(), causing concurrent map writes for
"multipart/form-data"; update Encode(data KV, dataformat string) to call
Get(dataformat) and use the returned encoder instance (handling returned error)
especially when dataformat == "multipart/form-data" so the fresh filesMetadata
instance is used, then call encoder.Encode(data) on that instance and propagate
any error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1501040-c2b5-4dc1-a217-fec22dc0d8ce
📒 Files selected for processing (2)
pkg/fuzz/dataformat/dataformat.gopkg/fuzz/dataformat/multipart_test.go
…ency safety Address review feedback: - Encode() now uses Get() to obtain fresh MultiPartForm instances, preventing concurrent map writes through the package-level API path - Rename TestMultiPartFormConcurrentDecode → TestMultiPartFormConcurrentDecodeEncode and exercise ParseBoundary + Decode + Encode with a real multipart body - Add TestMultiPartFormConcurrentPackageLevelEncode to cover the package-level Encode() path - Add note on Decode() explaining why singleton is currently safe (MultiPartForm.IsType() returns false)
Summary
MultiPartFormstores mutable state (boundarystring andfilesMetadatamap) but was registered as a singleton indataformat.init()and returned viadataformat.Get()to every goroutine. When scanning multiple targets concurrently, this causedfatal error: concurrent map writes— a Go runtime error that cannot be caught byrecover().Root Cause
Multiple goroutines writing to
m.boundaryandm.filesMetadatasimultaneously triggers the fatal error.Fix
Return a fresh
MultiPartForminstance fromdataformat.Get()when the requested format ismultipart/form-data, since it's the onlyDataFormatimplementation with mutable state. All other formats remain singletons (they are stateless).Changes
pkg/fuzz/dataformat/dataformat.go: Return fresh instance for multipart/form-datapkg/fuzz/dataformat/multipart_test.go: Add concurrent access regression test (100 goroutines)Test plan
TestMultiPartFormConcurrentDecode— 100 concurrent goroutines callingGet()+Encode()without panicgo test ./pkg/fuzz/dataformat/... -race— verify no data races detectedFixes #7028
Summary by CodeRabbit
Bug Fixes
Tests