From 361a4e99acc95fb74bc4f140930f5e3f5442925e Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 16 Mar 2023 14:18:04 -0700 Subject: [PATCH] [release-branch.go1.19] mime/multipart: avoid excessive copy buffer allocations in ReadForm # AWS EKS Backported To: go-1.16.15-eks Backported On: Wed, 5 Apr 2023 Backported By: kodurub@amazon.com Backported From: release-branch.go1.19 Source Commit: https://github.com/golang/go/commit/ef41a4e2face45e580c5836eaebd51629fc23f15 When copying form data to disk with io.Copy, allocate only one copy buffer and reuse it rather than creating two buffers per file (one from io.multiReader.WriteTo, and a second one from os.File.ReadFrom). Thanks to Jakob Ackermann (@das7pad) for reporting this issue. For CVE-2023-24536 For #59153 For #59269 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802453 Run-TryBot: Damien Neil Reviewed-by: Julie Qiu Reviewed-by: Roland Shoemaker Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802395 Run-TryBot: Roland Shoemaker Reviewed-by: Damien Neil Change-Id: Ie405470c92abffed3356913b37d813e982c96c8b Reviewed-on: https://go-review.googlesource.com/c/go/+/481983 Run-TryBot: Michael Knyszek TryBot-Result: Gopher Robot Auto-Submit: Michael Knyszek Reviewed-by: Matthew Dempsky --- src/mime/multipart/formdata.go | 15 +++++++-- src/mime/multipart/formdata_test.go | 49 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go index d6898c6ce1..5f31120c51 100644 --- a/src/mime/multipart/formdata.go +++ b/src/mime/multipart/formdata.go @@ -83,6 +83,7 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { maxMemoryBytes = math.MaxInt64 } } + var copyBuf []byte for { p, err := r.nextPart(false, maxMemoryBytes) if err == io.EOF { @@ -146,14 +147,22 @@ func (r *Reader) readForm(maxMemory int64) (_ *Form, err error) { } } numDiskFiles++ - size, err := io.Copy(file, io.MultiReader(&b, p)) + if _, err := file.Write(b.Bytes()); err != nil { + return nil, err + } + if copyBuf == nil { + copyBuf = make([]byte, 32*1024) // same buffer size as io.Copy uses + } + // os.File.ReadFrom will allocate its own copy buffer if we let io.Copy use it. + type writerOnly struct{ io.Writer } + remainingSize, err := io.CopyBuffer(writerOnly{file}, p, copyBuf) if err != nil { return nil, err } fh.tmpfile = file.Name() - fh.Size = size + fh.Size = int64(b.Len()) + remainingSize fh.tmpoff = fileOff - fileOff += size + fileOff += fh.Size if !combineFiles { if err := file.Close(); err != nil { return nil, err diff --git a/src/mime/multipart/formdata_test.go b/src/mime/multipart/formdata_test.go index 43475c3cd7..f6622dad47 100644 --- a/src/mime/multipart/formdata_test.go +++ b/src/mime/multipart/formdata_test.go @@ -369,3 +369,52 @@ func testReadFormManyFiles(t *testing.T, distinct bool) { t.Fatalf("temp dir contains %v files; want 0", len(names)) } } + +func BenchmarkReadForm(b *testing.B) { + for _, test := range []struct { + name string + form func(fw *Writer, count int) + }{{ + name: "fields", + form: func(fw *Writer, count int) { + for i := 0; i < count; i++ { + w, _ := fw.CreateFormField(fmt.Sprintf("field%v", i)) + fmt.Fprintf(w, "value %v", i) + } + }, + }, { + name: "files", + form: func(fw *Writer, count int) { + for i := 0; i < count; i++ { + w, _ := fw.CreateFormFile(fmt.Sprintf("field%v", i), fmt.Sprintf("file%v", i)) + fmt.Fprintf(w, "value %v", i) + } + }, + }} { + b.Run(test.name, func(b *testing.B) { + for _, maxMemory := range []int64{ + 0, + 1 << 20, + } { + var buf bytes.Buffer + fw := NewWriter(&buf) + test.form(fw, 10) + if err := fw.Close(); err != nil { + b.Fatal(err) + } + b.Run(fmt.Sprintf("maxMemory=%v", maxMemory), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + fr := NewReader(bytes.NewReader(buf.Bytes()), fw.Boundary()) + form, err := fr.ReadForm(maxMemory) + if err != nil { + b.Fatal(err) + } + form.RemoveAll() + } + + }) + } + }) + } +} -- 2.39.1