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>

diff --git a/src/mime/multipart/multipart.go b/src/mime/multipart/multipart.go
index eeec97467bfeee1cfbeecd28076d30afb13e5045..3b746a5e15756c5281b212017f54775cdf20cfd8 100644 (file)
--- 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 32cec57f92645a460315517de9fb7021f314f50c..d06bb4159aaf0de2148ab3f7102da9aa93828705 100644 (file)
--- 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",