mime/multipart: don't call Read on io.Reader after an error is seen

The io.Reader contract makes no promises about how a Reader should
behave after it returns its first error. Usually the errors are
sticky, but they don't have to be. A regression in zlib.Reader (bug
accidentally relied on sticky errors.

Minimal fix: wrap the user's provided Reader in a Reader which
guarantees stickiness. The minimal fix is less scary than touching
the multipart state machine.

Fixes #14676

Change-Id: I8dd8814b13ae5530824ae0e68529f788974264a5
Reviewed-on: https://go-review.googlesource.com/20297
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/22045

src/mime/multipart/formdata_test.go

@@ -88,3 +88,39 @@ Content-Disposition: form-data; name="textb"
 ` + textbValue + `
 --MyBoundary--
 `
+
+func TestReadForm_NoReadAfterEOF(t *testing.T) {
+	maxMemory := int64(32) << 20
+	boundary := `---------------------------8d345eef0d38dc9`
+	body := `
+-----------------------------8d345eef0d38dc9
+Content-Disposition: form-data; name="version"
+
+171
+-----------------------------8d345eef0d38dc9--`
+
+	mr := NewReader(&failOnReadAfterErrorReader{t: t, r: strings.NewReader(body)}, boundary)
+
+	f, err := mr.ReadForm(maxMemory)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Logf("Got: %#v", f)
+}
+
+// failOnReadAfterErrorReader is an io.Reader wrapping r.
+// It fails t if any Read is called after a failing Read.
+type failOnReadAfterErrorReader struct {
+	t      *testing.T
+	r      io.Reader
+	sawErr error
+}
+
+func (r *failOnReadAfterErrorReader) Read(p []byte) (n int, err error) {
+	if r.sawErr != nil {
+		r.t.Fatalf("unexpected Read on Reader after previous read saw error %v", r.sawErr)
+	}
+	n, err = r.r.Read(p)
+	r.sawErr = err
+	return
+}

src/mime/multipart/multipart.go

@@ -96,7 +96,7 @@ func (p *Part) parseContentDisposition() {
 func NewReader(r io.Reader, boundary string) *Reader {
 	b := []byte("\r\n--" + boundary + "--")
 	return &Reader{
-		bufReader:        bufio.NewReaderSize(r, peekBufferSize),
+		bufReader:        bufio.NewReaderSize(&stickyErrorReader{r: r}, peekBufferSize),
 		nl:               b[:2],
 		nlDashBoundary:   b[:len(b)-2],
 		dashBoundaryDash: b[2:],
@@ -104,6 +104,24 @@ func NewReader(r io.Reader, boundary string) *Reader {
 	}
 }
 
+// stickyErrorReader is an io.Reader which never calls Read on its
+// underlying Reader once an error has been seen. (the io.Reader
+// interface's contract promises nothing about the return values of
+// Read calls after an error, yet this package does do multiple Reads
+// after error)
+type stickyErrorReader struct {
+	r   io.Reader
+	err error
+}
+
+func (r *stickyErrorReader) Read(p []byte) (n int, _ error) {
+	if r.err != nil {
+		return 0, r.err
+	}
+	n, r.err = r.r.Read(p)
+	return n, r.err
+}
+
 func newPart(mr *Reader) (*Part, error) {
 	bp := &Part{
 		Header: make(map[string][]string),