[release-branch.go1.19] 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 #59269

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

diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go
index a7d4ca97f0484428188ec4b0c9fd8ba3330d4c0b..975dcb6b26db47b55fa0551a663cfc8e20e9fb7b 100644 (file)
--- a/src/mime/multipart/formdata.go
+++ b/src/mime/multipart/formdata.go
@@ -84,6 +84,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 {
@@ -147,14 +148,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 5cded7170c6b8308b96543d28e6925b9de314f0d..f5b56083b2377324da9735d659f28f5b71c821d6 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()
+                                       }
+
+                               })
+                       }
+               })
+       }
+}