From 821b54921a3cba5d853b531d4b03527c01bfc9b4 Mon Sep 17 00:00:00 2001
From: mpl <mathieu.lonjaret@gmail.com>
Date: Sun, 4 Oct 2015 23:32:26 +0200
Subject: [PATCH] mime/multipart: fix peekBufferSeparatorIndex edge case

The case fixed by this change happens when, in func (pr partReader)
Read, the Peek happens to read so that peek looks like:

  "somedata\r\n--Boundary\r"

peekBufferSeparatorIndex was returning (-1, false) because it didn't
find the trailing '\n'.

This was wrong because:

1) It didn't match the documentation: as "\r\n--Boundary" was found, it
should return the index of that pattern, not -1.

2) It lead to an nCopy cut such as:
  "somedata\r| |\n--Boundary\r" instead of "somedata| |\r\n--Boundary\r"
which made the subsequent Read miss the boundary, and eventually end
with a "return 0, io.ErrUnexpectedEOF" case, as reported in:

https://github.com/camlistore/camlistore/issues/642

Change-Id: I1ba78a741bc0c7719e160add9cca932d10f8a615
Reviewed-on: https://go-review.googlesource.com/15269
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
---
 src/mime/multipart/multipart.go      |  2 +-
 src/mime/multipart/multipart_test.go | 79 ++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/src/mime/multipart/multipart.go b/src/mime/multipart/multipart.go
index eeec97467b..3b746a5e15 100644
--- a/src/mime/multipart/multipart.go
+++ b/src/mime/multipart/multipart.go
@@ -366,7 +366,7 @@ func (mr *Reader) peekBufferSeparatorIndex(peek []byte) (idx int, isEnd bool) {
 	peek = skipLWSPChar(peek)
 	// Don't have a complete line after the peek.
 	if bytes.IndexByte(peek, '\n') == -1 {
-		return -1, false
+		return idx, false
 	}
 	if len(peek) > 0 && peek[0] == '\n' {
 		return idx, true
diff --git a/src/mime/multipart/multipart_test.go b/src/mime/multipart/multipart_test.go
index 32cec57f92..d06bb4159a 100644
--- a/src/mime/multipart/multipart_test.go
+++ b/src/mime/multipart/multipart_test.go
@@ -664,6 +664,38 @@ Content-Type: application/octet-stream
 			},
 		},
 	},
+	// Context: https://github.com/camlistore/camlistore/issues/642
+	// If the file contents in the form happens to have a size such as:
+	// size = peekBufferSize - (len("\n--") + len(boundary) + len("\r") + 1), (modulo peekBufferSize)
+	// then peekBufferSeparatorIndex was wrongly returning (-1, false), which was leading to an nCopy
+	// cut such as:
+	// "somedata\r| |\n--Boundary\r" (instead of "somedata| |\r\n--Boundary\r"), which was making the
+	// subsequent Read miss the boundary.
+	{
+		name: "safeCount off by one",
+		sep:  "08b84578eabc563dcba967a945cdf0d9f613864a8f4a716f0e81caa71a74",
+		in: strings.Replace(`--08b84578eabc563dcba967a945cdf0d9f613864a8f4a716f0e81caa71a74
+Content-Disposition: form-data; name="myfile"; filename="my-file.txt"
+Content-Type: application/octet-stream
+
+`, "\n", "\r\n", -1) +
+			strings.Repeat("A", peekBufferSize-(len("\n--")+len("08b84578eabc563dcba967a945cdf0d9f613864a8f4a716f0e81caa71a74")+len("\r")+1)) +
+			strings.Replace(`
+--08b84578eabc563dcba967a945cdf0d9f613864a8f4a716f0e81caa71a74
+Content-Disposition: form-data; name="key"
+
+val
+--08b84578eabc563dcba967a945cdf0d9f613864a8f4a716f0e81caa71a74--
+`, "\n", "\r\n", -1),
+		want: []headerBody{
+			{textproto.MIMEHeader{"Content-Type": {`application/octet-stream`}, "Content-Disposition": {`form-data; name="myfile"; filename="my-file.txt"`}},
+				strings.Repeat("A", peekBufferSize-(len("\n--")+len("08b84578eabc563dcba967a945cdf0d9f613864a8f4a716f0e81caa71a74")+len("\r")+1)),
+			},
+			{textproto.MIMEHeader{"Content-Disposition": {`form-data; name="key"`}},
+				"val",
+			},
+		},
+	},
 
 	roundTripParseTest(),
 }
@@ -704,6 +736,53 @@ Cases:
 	}
 }
 
+func partsFromReader(r *Reader) ([]headerBody, error) {
+	got := []headerBody{}
+	for {
+		p, err := r.NextPart()
+		if err == io.EOF {
+			return got, nil
+		}
+		if err != nil {
+			return nil, fmt.Errorf("NextPart: %v", err)
+		}
+		pbody, err := ioutil.ReadAll(p)
+		if err != nil {
+			return nil, fmt.Errorf("error reading part: %v", err)
+		}
+		got = append(got, headerBody{p.Header, string(pbody)})
+	}
+}
+
+func TestParseAllSizes(t *testing.T) {
+	const maxSize = 5 << 10
+	var buf bytes.Buffer
+	body := strings.Repeat("a", maxSize)
+	bodyb := []byte(body)
+	for size := 0; size < maxSize; size++ {
+		buf.Reset()
+		w := NewWriter(&buf)
+		part, _ := w.CreateFormField("f")
+		part.Write(bodyb[:size])
+		part, _ = w.CreateFormField("key")
+		part.Write([]byte("val"))
+		w.Close()
+		r := NewReader(&buf, w.Boundary())
+		got, err := partsFromReader(r)
+		if err != nil {
+			t.Errorf("For size %d: %v", size, err)
+			continue
+		}
+		if len(got) != 2 {
+			t.Errorf("For size %d, num parts = %d; want 2", size, len(got))
+			continue
+		}
+		if got[0].body != body[:size] {
+			t.Errorf("For size %d, got unexpected len %d: %q", size, len(got[0].body), got[0].body)
+		}
+	}
+}
+
 func roundTripParseTest() parseTest {
 	t := parseTest{
 		name: "round trip",
-- 
2.48.1