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

Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802453
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Julie Qiu <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802395
Run-TryBot: Roland Shoemaker <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Change-Id: Ie405470c92abffed3356913b37d813e982c96c8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/481983
Run-TryBot: Michael Knyszek <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Michael Knyszek <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>

Author: neild

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

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()
+					}
+
+				})
+			}
+		})
+	}
+}