multipart: fixes problem parsing mime/multipart of certain lengths

When parsing the multipart data, if the delimiter appears but doesn't
finish with -- or \n or \r\n, it assumes the data can be consumed. This
is incorrect when the peeking buffer finishes with --delimiter-

Fixes #12662

Change-Id: I329556a9a206407c0958289bf7a9009229120bb9
Reviewed-on: https://go-review.googlesource.com/14652
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/mime/multipart/multipart.go b/src/mime/multipart/multipart.go
index 6f65a55de276bd9bc02a32e51de535bfc2a70577..eeec97467bfeee1cfbeecd28076d30afb13e5045 100644 (file)
--- a/src/mime/multipart/multipart.go
+++ b/src/mime/multipart/multipart.go
@@ -25,6 +25,11 @@ import (
 
 var emptyParams = make(map[string]string)
 
+// This constant needs to be at least 76 for this package to work correctly.
+// This is because \r\n--separator_of_len_70- would fill the buffer and it
+// wouldn't be safe to consume a single byte from it.
+const peekBufferSize = 4096
+
 // A Part represents a single part in a multipart body.
 type Part struct {
        // The headers of the body, if any, with the keys canonicalized
@@ -91,7 +96,7 @@ func (p *Part) parseContentDisposition() {
 func NewReader(r io.Reader, boundary string) *Reader {
        b := []byte("\r\n--" + boundary + "--")
        return &Reader{
-               bufReader:        bufio.NewReader(r),
+               bufReader:        bufio.NewReaderSize(r, peekBufferSize),
                nl:               b[:2],
                nlDashBoundary:   b[:len(b)-2],
                dashBoundaryDash: b[2:],
@@ -148,7 +153,7 @@ func (pr partReader) Read(d []byte) (n int, err error) {
                // the read request.  No need to parse more at the moment.
                return p.buffer.Read(d)
        }
-       peek, err := p.mr.bufReader.Peek(4096) // TODO(bradfitz): add buffer size accessor
+       peek, err := p.mr.bufReader.Peek(peekBufferSize) // TODO(bradfitz): add buffer size accessor
 
        // Look for an immediate empty part without a leading \r\n
        // before the boundary separator.  Some MIME code makes empty
@@ -229,6 +234,7 @@ func (r *Reader) NextPart() (*Part, error) {
        expectNewPart := false
        for {
                line, err := r.bufReader.ReadSlice('\n')
+
                if err == io.EOF && r.isFinalBoundary(line) {
                        // If the buffer ends in "--boundary--" without the
                        // trailing "\r\n", ReadSlice will return an error
@@ -343,13 +349,17 @@ func (mr *Reader) peekBufferIsEmptyPart(peek []byte) bool {
 // peekBufferSeparatorIndex returns the index of mr.nlDashBoundary in
 // peek and whether it is a real boundary (and not a prefix of an
 // unrelated separator). To be the end, the peek buffer must contain a
-// newline after the boundary.
+// newline after the boundary or contain the ending boundary (--separator--).
 func (mr *Reader) peekBufferSeparatorIndex(peek []byte) (idx int, isEnd bool) {
        idx = bytes.Index(peek, mr.nlDashBoundary)
        if idx == -1 {
                return
        }
+
        peek = peek[idx+len(mr.nlDashBoundary):]
+       if len(peek) == 0 || len(peek) == 1 && peek[0] == '-' {
+               return idx, false
+       }
        if len(peek) > 1 && peek[0] == '-' && peek[1] == '-' {
                return idx, true
        }

diff --git a/src/mime/multipart/multipart_test.go b/src/mime/multipart/multipart_test.go
index 30452d1d458cd76885935d26866b132249461097..32cec57f92645a460315517de9fb7021f314f50c 100644 (file)
--- a/src/mime/multipart/multipart_test.go
+++ b/src/mime/multipart/multipart_test.go
@@ -616,6 +616,54 @@ html things
                        },
                },
        },
+       // Issue 12662: Check that we don't consume the leading \r if the peekBuffer
+       // ends in '\r\n--separator-'
+       {
+               name: "peek buffer boundary condition",
+               sep:  "00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db",
+               in: strings.Replace(`--00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db
+Content-Disposition: form-data; name="block"; filename="block"
+Content-Type: application/octet-stream
+
+`+strings.Repeat("A", peekBufferSize-65)+"\n--00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db--", "\n", "\r\n", -1),
+               want: []headerBody{
+                       {textproto.MIMEHeader{"Content-Type": {`application/octet-stream`}, "Content-Disposition": {`form-data; name="block"; filename="block"`}},
+                               strings.Repeat("A", peekBufferSize-65),
+                       },
+               },
+       },
+       // Issue 12662: Same test as above with \r\n at the end
+       {
+               name: "peek buffer boundary condition",
+               sep:  "00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db",
+               in: strings.Replace(`--00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db
+Content-Disposition: form-data; name="block"; filename="block"
+Content-Type: application/octet-stream
+
+`+strings.Repeat("A", peekBufferSize-65)+"\n--00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db--\n", "\n", "\r\n", -1),
+               want: []headerBody{
+                       {textproto.MIMEHeader{"Content-Type": {`application/octet-stream`}, "Content-Disposition": {`form-data; name="block"; filename="block"`}},
+                               strings.Repeat("A", peekBufferSize-65),
+                       },
+               },
+       },
+       // Issue 12662v2: We want to make sure that for short buffers that end with
+       // '\r\n--separator-' we always consume at least one (valid) symbol from the
+       // peekBuffer
+       {
+               name: "peek buffer boundary condition",
+               sep:  "aaaaaaaaaa00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db",
+               in: strings.Replace(`--aaaaaaaaaa00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db
+Content-Disposition: form-data; name="block"; filename="block"
+Content-Type: application/octet-stream
+
+`+strings.Repeat("A", peekBufferSize)+"\n--aaaaaaaaaa00ffded004d4dd0fdf945fbdef9d9050cfd6a13a821846299b27fc71b9db--", "\n", "\r\n", -1),
+               want: []headerBody{
+                       {textproto.MIMEHeader{"Content-Type": {`application/octet-stream`}, "Content-Disposition": {`form-data; name="block"; filename="block"`}},
+                               strings.Repeat("A", peekBufferSize),
+                       },
+               },
+       },
 
        roundTripParseTest(),
 }