fix(fuzz): prevent concurrent map writes in MultiPartForm.Decode#7029
Open
yusei-wy wants to merge 2 commits intoprojectdiscovery:devfrom
Open
fix(fuzz): prevent concurrent map writes in MultiPartForm.Decode#7029yusei-wy wants to merge 2 commits intoprojectdiscovery:devfrom
yusei-wy wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
Contributor
WalkthroughThis pull request addresses a concurrent map writes fatal error in multipart form data fuzzing by replacing singleton decoder reuse with per-instance creation. It introduces an encoder tracking mechanism in Value to store decoder references and adds test coverage for concurrent multipart decoding scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
MultiPartForm is the only stateful DataFormat (stores boundary and filesMetadata) but was shared as a singleton via dataformat.Get(). When multiple goroutines fuzz multipart/form-data bodies in parallel, concurrent writes to filesMetadata cause fatal error: concurrent map writes. Create a fresh MultiPartForm instance per parseBody call instead of reusing the singleton, and store it on Value.encoder for re-encoding. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yusei-wy
force-pushed
the
fix/multipart-concurrent-map-writes
branch
from
2b25f40 to
3397c07
Compare
Neo - PR Security ReviewNo security issues found Highlights
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/fuzz/component/value.go (1)
167-171: Optionally guard encoder selection to avoid stale encoder reuse.As a defensive check, only use
v.encoderwhen it matches the activev.dataFormat.🛡️ Defensive tweak
func (v *Value) encode(data dataformat.KV) (string, error) { - if v.encoder != nil { + if v.encoder != nil && v.encoder.Name() == v.dataFormat { return v.encoder.Encode(data) } return dataformat.Encode(data, v.dataFormat) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/component/value.go` around lines 167 - 171, The encode method currently unconditionally uses v.encoder when non-nil which can reuse an encoder for the wrong format; change Value.encode to only call v.encoder.Encode(data) if v.encoder is non-nil and its format matches v.dataFormat (e.g. via a Format()/Kind() method or comparable identifier on the encoder); otherwise fall back to dataformat.Encode(data, v.dataFormat) and optionally clear or replace v.encoder to avoid future stale reuse. Ensure you reference Value.encode, v.encoder and v.dataFormat when making the guard.pkg/fuzz/dataformat/multipart_test.go (1)
388-390: Strengthen assertions to verify decoded multipart content, not only non-nil KV.Right now this can pass even if decode returns an unexpected structure. Add at least one field/metadata assertion in each goroutine.
💡 Proposed test-strengthening diff
kv, err := mpf.Decode(body) assert.NoError(t, err) assert.NotNil(t, kv) + fileValues, ok := kv.Get("file").([]interface{}) + assert.True(t, ok, "expected file field to decode as []interface{}") + assert.Len(t, fileValues, 1) + assert.Equal(t, "file content", fileValues[0]) + + metadata, exists := mpf.GetFileMetadata("file") + assert.True(t, exists, "expected file metadata to be present") + assert.Equal(t, "test.txt", metadata.Filename)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/dataformat/multipart_test.go` around lines 388 - 390, The test currently only checks that mpf.Decode(body) returns no error and a non-nil kv; update each goroutine after kv is obtained (the Decode call in the test using mpf.Decode and variable kv) to assert specific expected content from the decoded multipart structure — e.g., check a known field key/value or metadata entry exists and equals the expected string (or that a particular part count/field name is present) so the test fails if the structure is incorrect rather than merely non-nil.lib/multipart_race_test.go (1)
122-127: Assert callback activity so the regression test proves execution depth.Right now the test can pass without confirming any result events were produced.
✅ Suggested assertion hardening
import ( "context" "fmt" "net/http" "net/http/httptest" "net/url" "os" + "sync/atomic" "testing" @@ - err = ne.ExecuteCallbackWithCtx(context.Background(), func(event *output.ResultEvent) { + var resultCount atomic.Int32 + err = ne.ExecuteCallbackWithCtx(context.Background(), func(event *output.ResultEvent) { + resultCount.Add(1) t.Logf("Result: %s", event.TemplateID) }) - if err != nil { - t.Errorf("ExecuteCallbackWithCtx error: %v", err) - } + require.NoError(t, err) + require.Greater(t, resultCount.Load(), int32(0), "expected at least one callback event")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/multipart_race_test.go` around lines 122 - 127, The test currently calls ne.ExecuteCallbackWithCtx(...) without verifying the callback ran; modify the test to record callback activity (e.g., increment an atomic counter or send a value on a channel from inside the callback passed to ExecuteCallbackWithCtx) and after ExecuteCallbackWithCtx returns assert that at least one ResultEvent was observed (counter > 0 or channel received). Use the existing ResultEvent parameter in the callback to optionally validate TemplateID if desired and fail the test with t.Fatalf/t.Errorf when no events were produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/multipart_race_test.go`:
- Around line 122-127: The test currently calls ne.ExecuteCallbackWithCtx(...)
without verifying the callback ran; modify the test to record callback activity
(e.g., increment an atomic counter or send a value on a channel from inside the
callback passed to ExecuteCallbackWithCtx) and after ExecuteCallbackWithCtx
returns assert that at least one ResultEvent was observed (counter > 0 or
channel received). Use the existing ResultEvent parameter in the callback to
optionally validate TemplateID if desired and fail the test with
t.Fatalf/t.Errorf when no events were produced.
In `@pkg/fuzz/component/value.go`:
- Around line 167-171: The encode method currently unconditionally uses
v.encoder when non-nil which can reuse an encoder for the wrong format; change
Value.encode to only call v.encoder.Encode(data) if v.encoder is non-nil and its
format matches v.dataFormat (e.g. via a Format()/Kind() method or comparable
identifier on the encoder); otherwise fall back to dataformat.Encode(data,
v.dataFormat) and optionally clear or replace v.encoder to avoid future stale
reuse. Ensure you reference Value.encode, v.encoder and v.dataFormat when making
the guard.
In `@pkg/fuzz/dataformat/multipart_test.go`:
- Around line 388-390: The test currently only checks that mpf.Decode(body)
returns no error and a non-nil kv; update each goroutine after kv is obtained
(the Decode call in the test using mpf.Decode and variable kv) to assert
specific expected content from the decoded multipart structure — e.g., check a
known field key/value or metadata entry exists and equals the expected string
(or that a particular part count/field name is present) so the test fails if the
structure is incorrect rather than merely non-nil.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/multipart_race_test.gopkg/fuzz/component/body.gopkg/fuzz/component/value.gopkg/fuzz/dataformat/multipart_test.go
dwisiswant0
requested changes
Feb 25, 2026
lib/multipart_race_test.go
Outdated
Member
There was a problem hiding this comment.
Just put this in the existing test file.
Contributor
Author
There was a problem hiding this comment.
Done. Moved to lib/sdk_test.go.
Self-contained test using httptest server with 20 concurrent multipart targets via DASTMode() + LoadTargetsWithHttpData. Fails on dev branch with DATA RACE, passes with fix applied. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yusei-wy
force-pushed
the
fix/multipart-concurrent-map-writes
branch
from
3397c07 to
b80d637
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/sdk_test.go (1)
170-175: Add one assertion that callback path was actually exercised.At Line 170-Line 175, the test only checks returned error. A zero-event execution would still pass and weaken regression signal. Track callback count and assert it is > 0.
✅ Suggested strengthening diff
import ( "context" "fmt" "log" "net/http" "net/http/httptest" "net/url" "os" + "sync/atomic" "testing" "time" @@ - err = ne.ExecuteCallbackWithCtx(context.Background(), func(event *output.ResultEvent) { + var callbackCount int32 + err = ne.ExecuteCallbackWithCtx(context.Background(), func(event *output.ResultEvent) { + atomic.AddInt32(&callbackCount, 1) t.Logf("Result: %s", event.TemplateID) }) if err != nil { t.Errorf("ExecuteCallbackWithCtx error: %v", err) } + require.Greater(t, atomic.LoadInt32(&callbackCount), int32(0), "expected at least one result callback") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sdk_test.go` around lines 170 - 175, The test currently calls ne.ExecuteCallbackWithCtx but only checks the returned error; add a counter to assert the callback was actually invoked. Declare a counter (e.g., var cbCount int32) before calling ne.ExecuteCallbackWithCtx, increment it inside the callback passed to ExecuteCallbackWithCtx (use atomic.AddInt32 to be safe for concurrent invocations), then after the call assert with t.Fatalf or t.Errorf that atomic.LoadInt32(&cbCount) > 0 so the output.ResultEvent callback path was exercised. Ensure references use the existing ne.ExecuteCallbackWithCtx and output.ResultEvent symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/sdk_test.go`:
- Around line 170-175: The test currently calls ne.ExecuteCallbackWithCtx but
only checks the returned error; add a counter to assert the callback was
actually invoked. Declare a counter (e.g., var cbCount int32) before calling
ne.ExecuteCallbackWithCtx, increment it inside the callback passed to
ExecuteCallbackWithCtx (use atomic.AddInt32 to be safe for concurrent
invocations), then after the call assert with t.Fatalf or t.Errorf that
atomic.LoadInt32(&cbCount) > 0 so the output.ResultEvent callback path was
exercised. Ensure references use the existing ne.ExecuteCallbackWithCtx and
output.ResultEvent symbols.
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
2 participants
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.
Summary
Fixes #7028
MultiPartFormis the only statefulDataFormatimplementation (it storesboundaryandfilesMetadata), but it was registered as a singleton viadataformat.Get()and shared across all goroutines. When multiple targets are scanned concurrently, parallel writes tom.filesMetadatacausefatal error: concurrent map writes.Fix: Create a fresh
MultiPartForminstance perparseBodycall instead of reusing the singleton.Changes
pkg/fuzz/component/body.goparseBodycreatesNewMultiPartForm()+ParseBoundary()for multipart instead of callingdataformat.Get()pkg/fuzz/component/value.goencoderfield toValueso the per-requestMultiPartForminstance is reused for re-encodingpkg/fuzz/dataformat/multipart_test.goTestMultiPartFormDecode_ConcurrentWithSeparateInstances(10 goroutines), boundary validation tests, file metadata testslib/multipart_race_test.goDASTMode()+LoadTargetsWithHttpData— fails ondev, passes with fixWhy the singleton is unsafe
Other
DataFormatimplementations (JSON, XML, Form, Raw) are stateless —DecodeandEncodeuse only local variables.MultiPartFormis different:ParseBoundary()writes tom.boundary(string field)Decode()writes tom.filesMetadata(map field) atmultipart.go:222When
executeTemplateWithTargetsspawns worker goroutines for N targets, each goroutine calls:Why 1 target does not trigger the race
executeTemplateWithTargetssends targets through an unbuffered channel (pkg/core/executors.go:95). With 1 target, only 1 worker picks it up → sequential execution → no concurrent access. The race requires N > 1 targets.Reproduction
Full reproduction details including template YAML, proxify input format, and SDK test code: #7028
CI lint note
The
golangci-lintjob reports 3 pre-existingstaticcheck QF1012errors inpkg/js/libs/ldap/utils.go. These are unrelated to this PR (zero diff on that file) and already present on thedevbranch:Test plan
go test -race ./pkg/fuzz/dataformat/...— all multipart tests pass including concurrent instance testgo test -race -run TestMultiPartForm_ConcurrentMapWrites_SDK ./lib/— SDK regression test passes (20 targets, no race)TestMultiPartFormComponentinbody_test.gocontinues to passmake test(includes-raceflag)make integration(includesfuzz-body-multipart-form-sqli.yaml)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests