mime/multipart: avoid excessive copy buffer allocations in ReadForm

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

Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802453
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Change-Id: I732bd2e1e7467918cac8ab9d65d089272ba4656f
Reviewed-on: https://go-review.googlesource.com/c/go/+/482075
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Bypass: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>

diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go
index e62727dbb13b9c639990e327ec39f352683e6a4f..902bb10e57b4e0a3380612198dbc2a7ae2831864 100644 (file)
--- a/src/mime/multipart/formdata.go
+++ b/src/mime/multipart/formdata.go
@@ -88,6 +88,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 {
@@ -151,14 +152,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 9b3f9ec392428f693c9e258481f5848e64587140..e0ceb6f7aa786c3998649c90cf76eddc9016b467 100644 (file)
--- a/src/mime/multipart/formdata_test.go
+++ b/src/mime/multipart/formdata_test.go
@@ -399,3 +399,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()
+                                       }
+
+                               })
+                       }
+               })
+       }
+}