[release-branch.go1.20] mime/multipart: avoid excessive copy buffer allocations in...

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 #59270

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: I44ef17c4b4964cdac2858317275594194801fee3
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802398
Run-TryBot: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/481989
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go
index 41bc886d1679d5840dd76a194500f5d23b7c0b7c..69b8b0399da57fadb5026c5d778c101d3de4d872 100644 (file)
--- a/src/mime/multipart/formdata.go
+++ b/src/mime/multipart/formdata.go
@@ -85,6 +85,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 {
@@ -148,14 +149,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 8a862be717415edae2780aa51b1e9a4d04955971..60f55b4a27fa021098ad519227a0c3642ddc31de 100644 (file)
--- a/src/mime/multipart/formdata_test.go
+++ b/src/mime/multipart/formdata_test.go
@@ -368,3 +368,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()
+                                       }
+
+                               })
+                       }
+               })
+       }
+}