[go] mime/multipart: don't read past the next boundary
Quentin Smith (Gerrit)
Quentin Smith would like Russ Cox and Brad Fitzpatrick to review mime/multipart: don't read past the next boundary.
mime/multipart: don't read past the next boundary Some multipart data arrives in a stream, where subsequent parts may not be ready yet. Read should return a complete part as soon as possible. Instead of reading in 4K blocks, we instead Peek(1) which reads whatever is available, and search for the boundary only in that data. Fixes #15431 Change-Id: Ie8c041b853f3e07f0f2a66fbf4bcab5fe9132a7c --- M src/mime/multipart/multipart.go M src/mime/multipart/multipart_test.go 2 files changed, 101 insertions(+), 4 deletions(-)
diff --git a/src/mime/multipart/multipart.go b/src/mime/multipart/multipart.go
index 205348c..d4b6a1f 100644
--- a/src/mime/multipart/multipart.go
+++ b/src/mime/multipart/multipart.go
@@ -171,7 +171,12 @@
// the read request. No need to parse more at the moment.
return p.buffer.Read(d)
}
- peek, err := p.mr.bufReader.Peek(peekBufferSize) // TODO(bradfitz): add buffer size accessor
+ // Make sure the buffer is big enough to potentially contain
+ // the separator, but don't block on reading beyond the next
+ // boundary.
+ p.mr.bufReader.Peek(len(p.mr.nlDashBoundary) + 1)
+ // Read anything we can, but at least enough for the separator.
+ peek, err := p.mr.bufReader.Peek(p.mr.bufReader.Buffered()) // 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
@@ -197,9 +202,6 @@
if idx, isEnd := p.mr.peekBufferSeparatorIndex(peek); idx != -1 {
nCopy = idx
foundBoundary = isEnd
- if !isEnd && nCopy == 0 {
- nCopy = 1 // make some progress.
- }
} else if safeCount := len(peek) - len(p.mr.nlDashBoundary); safeCount > 0 {
nCopy = safeCount
} else if unexpectedEOF {
@@ -207,6 +209,14 @@
// wasn't found (and can't possibly fit), we must have
// hit the end of the file unexpectedly.
return 0, io.ErrUnexpectedEOF
+ }
+ if nCopy == 0 && !foundBoundary {
+ // We don't have enough data to either populate the
+ // buffer or find the boundary; peek one byte further
+ // to ensure we make some progress.
+ if _, err := p.mr.bufReader.Peek(p.mr.bufReader.Buffered() + 1); err == io.EOF {
+ return 0, io.ErrUnexpectedEOF
+ }
}
if nCopy > 0 {
if _, err := io.CopyN(p.buffer, p.mr.bufReader, int64(nCopy)); err != nil {
@@ -392,6 +402,13 @@
if len(peek) > 1 && peek[0] == '\r' && peek[1] == '\n' {
return idx, true
}
+ // We found the prefix and a subsequent newline, but it's not
+ // actually the separator because there are other characters
+ // on the line.
+ if idx == 0 && len(peek) > 0 {
+ // Force the fake separator to be emitted
+ return len(mr.nlDashBoundary), false
+ }
return idx, false
}
diff --git a/src/mime/multipart/multipart_test.go b/src/mime/multipart/multipart_test.go
index 82a7f86..f02c6da 100644
--- a/src/mime/multipart/multipart_test.go
+++ b/src/mime/multipart/multipart_test.go
@@ -13,8 +13,10 @@
"net/textproto"
"os"
"reflect"
+ "runtime/trace"
"strings"
"testing"
+ "time"
)
func TestBoundaryLine(t *testing.T) {
@@ -120,6 +122,11 @@
}
func TestMultipartSlowInput(t *testing.T) {
+ // DONOTSUBMIT
+ go func() {
+ time.Sleep(5 * time.Second)
+ trace.Stop()
+ }()
bodyReader := strings.NewReader(testMultipartBody("\r\n"))
testMultipart(t, &slowReader{bodyReader}, false)
}
@@ -323,6 +330,79 @@
return s.r.Read(p[:1])
}
+type sentinalReader struct {
+ // done is closed when this reader is read from.
+ done chan struct{}
+}
+
+func (s *sentinalReader) Read([]byte) (int, error) {
+ if s.done != nil {
+ close(s.done)
+ s.done = nil
+ }
+ return 0, io.EOF
+}
+
+func TestMultipartStream(t *testing.T) {
+ testBody1 := `
+This is a multi-part message. This line is ignored.
+--MyBoundary
+foo-bar: baz
+
+Body
+--MyBoundary
+`
+ testBody2 := `foo-bar: bop
+
+Body 2
+--MyBoundary--
+`
+ done1 := make(chan struct{})
+ reader := NewReader(
+ io.MultiReader(
+ strings.NewReader(testBody1),
+ &sentinalReader{done1},
+ strings.NewReader(testBody2)),
+ "MyBoundary")
+ buf := new(bytes.Buffer)
+
+ part, err := reader.NextPart()
+ if part == nil || err != nil {
+ t.Error("Expected part1")
+ return
+ }
+ if x := part.Header.Get("foo-bar"); x != "baz" {
+ t.Errorf("part.Header.Get(%q) = %q, want %q", "foo-bar", x, "baz")
+ }
+ buf.Reset()
+ if _, err := io.Copy(buf, part); err != nil {
+ t.Errorf("part 1 copy: %v", err)
+ }
+
+ expectEq(t, "Body", buf.String(), "Value of first part")
+
+ select {
+ case <-done1:
+ t.Errorf("Reader read past second boundary")
+ default:
+ }
+
+ part, err = reader.NextPart()
+ if part == nil || err != nil {
+ t.Error("Expected part2")
+ return
+ }
+ if x := part.Header.Get("foo-bar"); x != "bop" {
+ t.Errorf("part.Header.Get(%q) = %q, want %q", "foo-bar", x, "bop")
+ }
+ buf.Reset()
+ if _, err := io.Copy(buf, part); err != nil {
+ t.Errorf("part 2 copy: %v", err)
+ }
+
+ expectEq(t, "Body 2", buf.String(), "Value of second part")
+}
+
func TestLineContinuation(t *testing.T) {
// This body, extracted from an email, contains headers that span multiple
// lines.
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on mime/multipart: don't read past the next boundary.
Patch Set 1: TryBots beginning. Status page: http://farmer.golang.org/try?commit=04cf3a09
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on mime/multipart: don't read past the next boundary.
Patch Set 1: TryBot-Result+1 TryBots are happy.
To view, visit this change. To unsubscribe, visit settings.
Russ Cox (Gerrit)
Russ Cox posted comments on mime/multipart: don't read past the next boundary.
Patch Set 1: (2 comments)
File src/mime/multipart/multipart.go:
Patch Set #1, Line 179:
peek, err := p.mr.bufReader.Peek(p.mr.bufReader.Buffered()) // TODO(bradfitz): add buffer size accessorcan probably delete the TODO now
Patch Set #1, Line 217:
if _, err := p.mr.bufReader.Peek(p.mr.bufReader.Buffered() + 1); err == io.EOF {This seems wrong. Why is +1 going to make the remaining code safe to execute? I would believe this if it was going to go back around the loop and retest all these conditions.
To view, visit this change. To unsubscribe, visit settings.
Blixt (Gerrit)
Blixt posted comments on mime/multipart: don't read past the next boundary.
Patch Set 1: (1 comment) Minor nit
File src/mime/multipart/multipart_test.go:
Patch Set #1, Line 333:
type sentinalReader struct {s/sentinal/sentinel/
To view, visit this change. To unsubscribe, visit settings.
Russ Cox (Gerrit)
mime/multipart: don't read past the next boundary
(5 comments)
The more I looked at this CL the more I agree with Brad that this code
really can't just be patched up more. The structure needs to be made
clearer. I sent CL 32092 to do that. Once that's in, we can make this a
test-only CL.
https://go-review.googlesource.com/#/c/32032/1/src/mime/multipart/multipart_test.go
File src/mime/multipart/multipart_test.go:
Line 125: // DONOTSUBMIT
I assume this is some kind of debugging?
If you are debugging a hang, you can type ctrl-\ at the hung process and
get stacks. No need for runtime/trace.
Line 333: type sentinalReader struct {
> s/sentinal/sentinel/
type didRead bool
func (r *didRead) Read([]byte) (int, error) {
*r = true
return 0, io.EOF
}
Line 360: done1 := make(chan struct{})
var tooFar bool
Line 364: &sentinalReader{done1},
(*didRead)(&tooFar),
Line 370: if part == nil || err != nil {
You can assume part != nil if err == nil.
Use t.Fatal instead of t.Error+return.
Show the actual errors returned.
Check all the returned results together so you can show them all in the
failure output.
Pull repetitive parts into a local helper func to make the final test logic
clearer. Tables are good too, but a table is overkill here.
When it makes sense (returned from something like Read), check returned
data in addition to (usually before) error.
Putting all that together:
readPart := func(hdr textproto.MIMEHeader, body string) {
part, err := reader.NextPart()
if err != nil {
t.Fatalf("NextPart: %v", err)
}
data, err := ioutil.ReadAll(part)
if !reflect.DeepEqual(part.Header, hdr) || string(data) != body {
t.Errorf("Part = %v, %q, want %v, %q", part.Header, data, hdr, body)
}
if err != nil {
t.Fatalf("reading Part: %v", err)
}
}
readPart(textproto.MIMEHeader{"foo-bar": {"baz"}}, "Body")
if tooFar {
t.Errorf("read too far during Part1")
}
readPart(textproto.MIMEHeader{"foo-bar": {"bop"}}, "Body 2")
--
https://go-review.googlesource.com/32032
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gerrit-HasComments: Yes
Quentin Smith (Gerrit)
Quentin Smith uploaded a new patch set:
https://go-review.googlesource.com/32032
mime/multipart: don't read past the next boundary
Some multipart data arrives in a stream, where subsequent parts may not
be ready yet. Read should return a complete part as soon as
possible. Instead of reading in 4K blocks, we instead Peek(1) which
reads whatever is available, and search for the boundary only in that
data.
Fixes #15431
Change-Id: Ie8c041b853f3e07f0f2a66fbf4bcab5fe9132a7c
---
M src/mime/multipart/multipart.go
M src/mime/multipart/multipart_test.go
Quentin Smith (Gerrit)
mime/multipart: don't read past the next boundary
(4 comments)
https://go-review.googlesource.com/#/c/32032/1/src/mime/multipart/multipart.go
File src/mime/multipart/multipart.go:
Line 217: if _, err := p.mr.bufReader.Peek(p.mr.bufReader.Buffered() +
1); err == io.EOF {
> execut
previously confirmed to be valid. So the remaining code is safe to execute
either way.
The +1 here forces another read from the underlying input stream, which
will cause us to find more in the bufReader to work with the next time Read
is called.
https://go-review.googlesource.com/#/c/32032/1/src/mime/multipart/multipart_test.go
File src/mime/multipart/multipart_test.go:
Line 125: // DONOTSUBMIT
> I assume this is some kind of debugging?
Stacks at hang did not contain enough information for me, which is why I
used the runtime trace.
Line 333: type sentinalReader struct {
Doesn't your didRead formulation have a possible data race?
Line 370: if part == nil || err != nil {
> You can assume part != nil if err == nil.
this file; I prefer matching the style over making this test more compact.
--
https://go-review.googlesource.com/32032
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Quentin Smith (Gerrit)
--
https://go-review.googlesource.com/32032
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Quentin Smith <que...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gobot Gobot (Gerrit)
mime/multipart: don't read past the next boundary
TryBots beginning. Status page: http://farmer.golang.org/try?commit=f5a08db2
Gobot Gobot (Gerrit)
TryBots are happy.
Russ Cox (Gerrit)
Russ Cox posted comments on mime/multipart: don't read past the next boundary.
Patch Set 1: (3 comments)
Yeah, whoops. I didn't mean to leave this in (obviously).
I'm curious: what extra information did the runtime trace have?
Patch Set #1, Line 333:
type sentinalReader struct {
Doesn't your didRead formulation have a possible data race?
What is the race you are concerned about? I'm pretty sure there's only one goroutine executing.
Patch Set #1, Line 370:
if part == nil || err != nil {
The tests I'm using here are exact copies of the tests used in the rest of
In general that's good but the rest of the file is not particularly ideal code. In addition to collapsing a few lines the formulation above also avoids a 2X copy-paste. Since you can't reuse the helpers in the rest of the file, might as well write clear code here.
To view, visit this change. To unsubscribe, visit settings.
Emmanuel Odeke (Gerrit)
Emmanuel Odeke posted comments on mime/multipart: don't read past the next boundary.
Patch Set 2: @quentinsmith CL https://go-review.googlesource.com/#/c/32092 was merged and the original issue was resolved by it, what's your view on @rsc's suggestion of making this CL a test only CL after rebasing from master?
To view, visit this change. To unsubscribe, visit settings.
Quentin Smith (Gerrit)
Quentin Smith uploaded patch set #3 to mime/multipart: test for overreading on a stream.
mime/multipart: test for overreading on a stream Some multipart data arrives in a stream, where subsequent parts may not be ready yet. Read should return a complete part as soon as possible. Fixes #15431 Change-Id: Ie8c041b853f3e07f0f2a66fbf4bcab5fe9132a7c --- M src/mime/multipart/multipart_test.go 1 file changed, 62 insertions(+), 0 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on mime/multipart: test for overreading on a stream.
Patch Set 3: TryBots beginning. Status page: http://farmer.golang.org/try?commit=96229694
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on mime/multipart: test for overreading on a stream.
Patch Set 3: TryBot-Result+1 TryBots are happy.
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick posted comments on mime/multipart: test for overreading on a stream.
Patch Set 3: (2 comments)
File src/mime/multipart/multipart_test.go:
Patch Set #3, Line 339:
func TestMultipartStream(t *testing.T) {this name alone doesn't say what it's testing. Use some combination of a better name, a comment above it, and a reference to the bug.
Patch Set #3, Line 364:
t.Fatalf("NextPart failed: %v", err)If this line fails, it won't be obvious if the caller was line 377 or line 385. See: https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures
To view, visit this change. To unsubscribe, visit settings.
Quentin Smith (Gerrit)
Quentin Smith uploaded patch set #4 to mime/multipart: test for overreading on a stream.
mime/multipart: test for overreading on a stream Some multipart data arrives in a stream, where subsequent parts may not be ready yet. Read should return a complete part as soon as possible. Fixes #15431 Change-Id: Ie8c041b853f3e07f0f2a66fbf4bcab5fe9132a7c --- M src/mime/multipart/multipart_test.go 1 file changed, 67 insertions(+), 0 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on mime/multipart: test for overreading on a stream.
Patch Set 4: TryBots beginning. Status page: http://farmer.golang.org/try?commit=58f3fbe2
To view, visit this change. To unsubscribe, visit settings.
Quentin Smith (Gerrit)
Quentin Smith posted comments on mime/multipart: test for overreading on a stream.
Patch Set 3: (4 comments)
I'm curious: what extra information did the runtime trace have?
It turned out to not have the right information, either. I was trying to debug an infinite loop and I was hoping the runtime trace would show the sequence of function calls. Unfortunately it only shows the stack at the end of each span.
Patch Set #1, Line 333:
close(s.done)
What is the race you are concerned about?
I didn't want to assume that mime/multipart had no concurrency.
Patch Set #1, Line 370:
data, err := ioutil.ReadAll(part)
In general that's good but the rest of the file is not particularly ideal c
Fair enough. The rest of the file could definitely use a cleanup at some point as well.
File src/mime/multipart/multipart_test.go:
Patch Set #3, Line 339:
func TestMultipartStream(t *testing.T) {this name alone doesn't say what it's testing. Use some combination of a be
Done
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on mime/multipart: test for overreading on a stream.
Patch Set 4: TryBot-Result+1 TryBots are happy.
To view, visit this change. To unsubscribe, visit settings.
Quentin Smith (Gerrit)
Quentin Smith posted comments on mime/multipart: test for overreading on a stream.
Patch Set 4: ping rsc / bradfitz
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick posted comments on mime/multipart: test for overreading on a stream.
Patch Set 4: Run-TryBot+1 Code-Review+2
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick merged mime/multipart: test for overreading on a stream.
mime/multipart: test for overreading on a stream Some multipart data arrives in a stream, where subsequent parts may not be ready yet. Read should return a complete part as soon as possible. Fixes #15431 Change-Id: Ie8c041b853f3e07f0f2a66fbf4bcab5fe9132a7c Reviewed-on: https://go-review.googlesource.com/32032 Run-TryBot: Quentin Smith <que...@golang.org> Run-TryBot: Brad Fitzpatrick <brad...@golang.org> Reviewed-by: Brad Fitzpatrick <brad...@golang.org> TryBot-Result: Gobot Gobot <go...@golang.org> --- M src/mime/multipart/multipart_test.go 1 file changed, 67 insertions(+), 0 deletions(-)
Approvals: Brad Fitzpatrick: Looks good to me, approved; Run TryBots Quentin Smith: Run TryBots Gobot Gobot: TryBots succeeded
diff --git a/src/mime/multipart/multipart_test.go b/src/mime/multipart/multipart_test.go
index d74ef61..7fbee90 100644
--- a/src/mime/multipart/multipart_test.go
+++ b/src/mime/multipart/multipart_test.go
@@ -324,6 +324,73 @@
return s.r.Read(p[:1])
}
+type sentinelReader struct {
+ // done is closed when this reader is read from.
+ done chan struct{}
+}
+
+func (s *sentinelReader) Read([]byte) (int, error) {
+ if s.done != nil {
+ close(s.done)
+ s.done = nil
+ }
+ return 0, io.EOF
+}
+
+// TestMultipartStreamReadahead tests that PartReader does not block
+// on reading past the end of a part, ensuring that it can be used on
+// a stream like multipart/x-mixed-replace. See golang.org/issue/15431
+func TestMultipartStreamReadahead(t *testing.T) {
+ testBody1 := `
+This is a multi-part message. This line is ignored.
+--MyBoundary
+foo-bar: baz
+
+Body
+--MyBoundary
+`
+ testBody2 := `foo-bar: bop
+
+Body 2
+--MyBoundary--
+`
+ done1 := make(chan struct{})
+ reader := NewReader(
+ io.MultiReader(
+ strings.NewReader(testBody1),
+ &sentinelReader{done1},
+ strings.NewReader(testBody2)),
+ "MyBoundary")
+
+ var i int
+ readPart := func(hdr textproto.MIMEHeader, body string) {
+ part, err := reader.NextPart()
+ if part == nil || err != nil {
+ t.Fatalf("Part %d: NextPart failed: %v", i, err)
+ }
+
+ if !reflect.DeepEqual(part.Header, hdr) {
+ t.Errorf("Part %d: part.Header = %v, want %v", i, part.Header, hdr)
+ }
+ data, err := ioutil.ReadAll(part)
+ expectEq(t, body, string(data), fmt.Sprintf("Part %d body", i))
+ if err != nil {
+ t.Fatalf("Part %d: ReadAll failed: %v", i, err)
+ }
+ i++
+ }
+
+ readPart(textproto.MIMEHeader{"Foo-Bar": {"baz"}}, "Body")
+
+ select {
+ case <-done1:
+ t.Errorf("Reader read past second boundary")
+ default:
+ }
+
+ readPart(textproto.MIMEHeader{"Foo-Bar": {"bop"}}, "Body 2")
+}
+
func TestLineContinuation(t *testing.T) {
// This body, extracted from an email, contains headers that span multiple
// lines.
To view, visit this change. To unsubscribe, visit settings.