code review 4432083: mime/multipart: fix regression from previous ReadSlice ...

mime/multipart: fix regression from previous ReadSlice change

The previous change to make multipart use ReadSlice out of
paranoia broke multipart to not deal with large lines in
the bodies.

We should only be paranoid about long lines in the header
sections.

Fixes http://code.google.com/p/camlistore/issues/detail?id=4

[bradfitz, 2011-05-01 02:28:12]

Hello agl (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-05-01 02:29:02]

Whoops, that was for adg, not agl. Can't type.

[adg, 2011-05-01 02:41:51]

LGTM

Can't believe the process we went through to get here...

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
File src/pkg/mime/multipart/multipart.go (right):

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:133: peek, err := bp.mr.bufReader.Peek(4096)
// TODO: add buffer size accessor
TODO(bradfitz) ?

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:142: // Look for the "\r\n--boundary" and
return up to it, or if it's not found
Reword:

Search the peek buffer for "\r\n--boundary". If found, consume everything up to
the boundary. If not, consume only as much of the peek buffer as cannot hold the
boundary string.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:152: } else if unexpectedEof {
comment:

If we've run out of peek buffer and the boundary wasn't found (and can't
possibly fit), we must have hit the end of the file unexpectedly.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:161: if err == os.EOF && !foundBoundary {
comment:

If the boundary hasn't been reached there's more to read, so don't pass through
an EOF from the buffer.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:178: nlBoundary, dashBoundaryDash,
dashBoundary []byte
s/nlBoundary/nlDashBoundary/g

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:203: if hasPrefixThenNewline(line,
mr.dashBoundaryDash) {
I think we only use this function once - if that's the case, maybe it should be
isBoundaryAndNewline without the second argument.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:205: // TODO: should return an os.EOF error
here, not using nil for errors
TODO(bradfitz)

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:218: if bytes.Equal(line, []byte("\r\n")) {
comment:

boundaries are preceded by \r\n

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:238: if bytes.HasSuffix(line,
[]byte("\r\n")) {
Do these []byte conversions allocate each time? If they do, we should use
globals instead.

[bradfitz, 2011-05-01 02:49:27]

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
File src/pkg/mime/multipart/multipart.go (right):

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:133: peek, err := bp.mr.bufReader.Peek(4096)
// TODO: add buffer size accessor
On 2011/05/01 02:41:51, adg wrote:
> TODO(bradfitz) ?

Done.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:142: // Look for the "\r\n--boundary" and
return up to it, or if it's not found
On 2011/05/01 02:41:51, adg wrote:
> Reword:
> 
> Search the peek buffer for "\r\n--boundary". If found, consume everything up
to
> the boundary. If not, consume only as much of the peek buffer as cannot hold
the
> boundary string. 

Done.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:152: } else if unexpectedEof {
On 2011/05/01 02:41:51, adg wrote:
> comment:
> 
> If we've run out of peek buffer and the boundary wasn't found (and can't
> possibly fit), we must have hit the end of the file unexpectedly.

Done.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:178: nlBoundary, dashBoundaryDash,
dashBoundary []byte
On 2011/05/01 02:41:51, adg wrote:
> s/nlBoundary/nlDashBoundary/g

Done.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:203: if hasPrefixThenNewline(line,
mr.dashBoundaryDash) {
On 2011/05/01 02:41:51, adg wrote:
> I think we only use this function once - if that's the case, maybe it should
be
> isBoundaryAndNewline without the second argument.

Will do this later with the TODO.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:205: // TODO: should return an os.EOF error
here, not using nil for errors
On 2011/05/01 02:41:51, adg wrote:
> TODO(bradfitz)

Done.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:218: if bytes.Equal(line, []byte("\r\n")) {
On 2011/05/01 02:41:51, adg wrote:
> comment:
> 
> boundaries are preceded by \r\n

Done.

http://codereview.appspot.com/4432083/diff/4001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:238: if bytes.HasSuffix(line,
[]byte("\r\n")) {
On 2011/05/01 02:41:51, adg wrote:
> Do these []byte conversions allocate each time? If they do, we should use
> globals instead.

I would hope the compiler does this.

[bradfitz, 2011-05-01 02:54:39]

*** Submitted as http://code.google.com/p/go/source/detail?r=698b5ea9c782 ***

mime/multipart: fix regression from previous ReadSlice change

The previous change to make multipart use ReadSlice out of
paranoia broke multipart to not deal with large lines in
the bodies.

We should only be paranoid about long lines in the header
sections.

Fixes http://code.google.com/p/camlistore/issues/detail?id=4

R=adg
CC=golang-dev
http://codereview.appspot.com/4432083