Skip to content

Commit c2f3397

Browse files
authored
fix(fuzz): concurrent map writes in multipart form parsing (#7291)
1 parent 406ad1a commit c2f3397

3 files changed

Lines changed: 91 additions & 5 deletions

File tree

pkg/fuzz/component/body.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,18 +77,24 @@ func (b *Body) Parse(req *retryablehttp.Request) (bool, error) {
7777

7878
// parseBody parses a body with a custom decoder
7979
func (b *Body) parseBody(decoderName string, req *retryablehttp.Request) (bool, error) {
80-
decoder := dataformat.Get(decoderName)
80+
var decoder dataformat.DataFormat
8181
if decoderName == dataformat.MultiPartFormDataFormat {
82-
// set content type to extract boundary
83-
if err := decoder.(*dataformat.MultiPartForm).ParseBoundary(req.Header.Get("Content-Type")); err != nil {
82+
// multipart has per-request state (boundary, file metadata) so we need
83+
// a fresh instance to avoid concurrent writes to the global singleton
84+
mpf := dataformat.NewMultiPartForm()
85+
if err := mpf.ParseBoundary(req.Header.Get("Content-Type")); err != nil {
8486
return false, errors.Wrap(err, "could not parse boundary")
8587
}
88+
decoder = mpf
89+
} else {
90+
decoder = dataformat.Get(decoderName)
8691
}
8792
decoded, err := decoder.Decode(b.value.String())
8893
if err != nil {
8994
return false, errors.Wrap(err, "could not decode raw")
9095
}
9196
b.value.SetParsed(decoded, decoder.Name())
97+
b.value.encoder = decoder
9298
return true, nil
9399
}
94100

pkg/fuzz/component/body_test.go

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@ package component
22

33
import (
44
"bytes"
5+
"fmt"
56
"io"
67
"mime/multipart"
78
"strings"
9+
"sync"
810
"testing"
911

1012
"github.com/projectdiscovery/retryablehttp-go"
@@ -173,3 +175,72 @@ func TestMultiPartFormComponent(t *testing.T) {
173175
require.Contains(t, string(newBody), "username", "unexpected body content")
174176
require.Contains(t, string(newBody), "testuser", "unexpected body content")
175177
}
178+
179+
// each goroutine creates its own multipart body with a unique boundary
180+
// and parses it concurrently. before the fix this would crash with
181+
// "fatal error: concurrent map writes" because all goroutines shared
182+
// the same MultiPartForm singleton.
183+
func TestMultiPartFormConcurrentParse(t *testing.T) {
184+
const goroutines = 20
185+
var wg sync.WaitGroup
186+
wg.Add(goroutines)
187+
188+
errs := make(chan error, goroutines)
189+
190+
for i := 0; i < goroutines; i++ {
191+
go func(id int) {
192+
defer wg.Done()
193+
194+
formData := &bytes.Buffer{}
195+
writer := multipart.NewWriter(formData)
196+
_ = writer.WriteField("field", fmt.Sprintf("value-%d", id))
197+
contentType := writer.FormDataContentType()
198+
_ = writer.Close()
199+
200+
req, err := retryablehttp.NewRequest("POST", "https://example.com", bytes.NewReader(formData.Bytes()))
201+
if err != nil {
202+
errs <- fmt.Errorf("goroutine %d: new request: %w", id, err)
203+
return
204+
}
205+
req.Header.Set("Content-Type", contentType)
206+
207+
body := NewBody()
208+
parsed, err := body.Parse(req)
209+
if err != nil {
210+
errs <- fmt.Errorf("goroutine %d: parse: %w", id, err)
211+
return
212+
}
213+
if !parsed {
214+
errs <- fmt.Errorf("goroutine %d: body was not parsed", id)
215+
return
216+
}
217+
218+
_ = body.SetValue("field", fmt.Sprintf("fuzzed-%d", id))
219+
220+
rebuilt, err := body.Rebuild()
221+
if err != nil {
222+
errs <- fmt.Errorf("goroutine %d: rebuild: %w", id, err)
223+
return
224+
}
225+
226+
rebuiltBody, err := io.ReadAll(rebuilt.Body)
227+
if err != nil {
228+
errs <- fmt.Errorf("goroutine %d: read rebuilt body: %w", id, err)
229+
return
230+
}
231+
232+
expected := fmt.Sprintf("fuzzed-%d", id)
233+
if !strings.Contains(string(rebuiltBody), expected) {
234+
errs <- fmt.Errorf("goroutine %d: rebuilt body missing %q", id, expected)
235+
return
236+
}
237+
}(i)
238+
}
239+
240+
wg.Wait()
241+
close(errs)
242+
243+
for err := range errs {
244+
t.Error(err)
245+
}
246+
}

pkg/fuzz/component/value.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ type Value struct {
1919
data string
2020
parsed dataformat.KV
2121
dataFormat string
22+
encoder dataformat.DataFormat
2223
}
2324

2425
// NewValue returns a new value component
@@ -42,6 +43,7 @@ func (v *Value) Clone() *Value {
4243
data: v.data,
4344
parsed: v.parsed.Clone(),
4445
dataFormat: v.dataFormat,
46+
encoder: v.encoder,
4547
}
4648
}
4749

@@ -135,12 +137,19 @@ func (v *Value) Delete(key string) bool {
135137

136138
// Encode encodes the value into a string
137139
// using the dataformat and encoding
140+
func (v *Value) encode(data dataformat.KV) (string, error) {
141+
if v.encoder != nil {
142+
return v.encoder.Encode(data)
143+
}
144+
return dataformat.Encode(data, v.dataFormat)
145+
}
146+
138147
func (v *Value) Encode() (string, error) {
139148
toEncodeStr := v.data
140149
if v.parsed.OrderedMap != nil {
141150
// flattening orderedmap not supported
142151
if v.dataFormat != "" {
143-
dataformatStr, err := dataformat.Encode(v.parsed, v.dataFormat)
152+
dataformatStr, err := v.encode(v.parsed)
144153
if err != nil {
145154
return "", err
146155
}
@@ -154,7 +163,7 @@ func (v *Value) Encode() (string, error) {
154163
return "", err
155164
}
156165
if v.dataFormat != "" {
157-
dataformatStr, err := dataformat.Encode(dataformat.KVMap(nested), v.dataFormat)
166+
dataformatStr, err := v.encode(dataformat.KVMap(nested))
158167
if err != nil {
159168
return "", err
160169
}

0 commit comments

Comments
 (0)