code review 4662059: mime/multipart: parse LF-delimited messages, not just CRLF

mime/multipart: parse LF-delimited messages, not just CRLF

Against the spec, but appear in the wild.

Fixes issue 1966

[bradfitz, 2011-06-27 23:20:06]

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

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

[adg, 2011-06-28 00:37:38]

Maybe it can be simpler with a little more state.

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

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:84: nlDashBoundary:   b[:len(b)-2],
nl: []byte("\r\n"),

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:145: searchFor := bp.mr.nlDashBoundary
revert the changes to this function

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:191: noCRMode bool
insteaed of this, why not:

nl []byte

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:238: if mr.noCRMode {
if bytes.Equal(line, mr.nl) {

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:263: if bytes.HasSuffix(line,
[]byte("\r\n")) {
if bytes.HasSuffix(line, mr.nl) {

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:268: if mr.partsRead == 0 || mr.noCRMode {
if mr.partsRead == 0 && bytes.HasSuffix(line, []byte("\n") {
    if onlyHorizontalWhitespace(line[len(mr.dashBoundary) : len(line)-1]) {
        mr.nl = mr.nl[1:]
        mr.nlDashBoundary = mr.nlDashBoundary[1:]
        return true
    }
}
return false

[bradfitz, 2011-06-28 03:35:36]

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

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:84: nlDashBoundary:   b[:len(b)-2],
On 2011/06/28 00:37:38, adg wrote:
> nl: []byte("\r\n"),

Done.

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:84: nlDashBoundary:   b[:len(b)-2],
On 2011/06/28 00:37:38, adg wrote:
> nl: []byte("\r\n"),

Done.

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:145: searchFor := bp.mr.nlDashBoundary
On 2011/06/28 00:37:38, adg wrote:
> revert the changes to this function

Done.

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:191: noCRMode bool
On 2011/06/28 00:37:38, adg wrote:
> insteaed of this, why not:
> 
> nl []byte

Done.

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:238: if mr.noCRMode {
On 2011/06/28 00:37:38, adg wrote:
> if bytes.Equal(line, mr.nl) {

Done.

http://codereview.appspot.com/4662059/diff/1003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:268: if mr.partsRead == 0 || mr.noCRMode {
On 2011/06/28 00:37:38, adg wrote:
> if mr.partsRead == 0 && bytes.HasSuffix(line, []byte("\n") {
>     if onlyHorizontalWhitespace(line[len(mr.dashBoundary) : len(line)-1]) {
>         mr.nl = mr.nl[1:]
>         mr.nlDashBoundary = mr.nlDashBoundary[1:]
>         return true
>     }
> }
> return false

Done, except can't consume the slices each time.  Only the first time.

[bradfitz, 2011-06-28 03:35:37]

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

Please take another look.

[adg, 2011-06-28 03:48:01]

Am I missing something?

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

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:239: var lf = []byte("\n")
why do this here and not for the []byte constants elsewhere?

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:251: if bytes.HasSuffix(line,
[]byte("\r\n")) {
Why can't this be

if bytes.HasSuffix(line, mr.nl) {
    return onlyHorizontalWhitespace(line[len(mr.dashBoundary) :
len(line)-len(mr.nl)])
}

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:256: if mr.partsRead == 0 ||
bytes.HasSuffix(line, lf) {
Then this could be &&

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:258: if mr.partsRead == 0 {
and you can drop this if statement (but keep its body).

[bradfitz, 2011-06-28 04:51:32]

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

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:239: var lf = []byte("\n")
On 2011/06/28 03:48:01, adg wrote:
> why do this here and not for the []byte constants elsewhere?

because the compiler can't yet tell that bytes.HasSuffix doesn't mutate it, so
it results in an allocation each time otherwise.  just verified that's still the
case with 6g -S.

But yes, I should also do it for the crlf case below.  Both done, and moved to
the top of the file.

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:251: if bytes.HasSuffix(line,
[]byte("\r\n")) {
On 2011/06/28 03:48:01, adg wrote:
> Why can't this be
> 
> if bytes.HasSuffix(line, mr.nl) {
>     return onlyHorizontalWhitespace(line[len(mr.dashBoundary) :
> len(line)-len(mr.nl)])
> }

Done.

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:256: if mr.partsRead == 0 ||
bytes.HasSuffix(line, lf) {
On 2011/06/28 03:48:01, adg wrote:
> Then this could be &&

Done.

http://codereview.appspot.com/4662059/diff/2004/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:258: if mr.partsRead == 0 {
On 2011/06/28 03:48:01, adg wrote:
> and you can drop this if statement (but keep its body).

Done.

[bradfitz, 2011-06-28 04:51:40]

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

Please take another look.

[adg, 2011-06-28 04:55:20]

LGTM

Nice.

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

http://codereview.appspot.com/4662059/diff/5003/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart.go:27: var lf = []byte("\n")
maybe put
// TODO(bradfitz): inline these once the compiler is smarter

[bradfitz, 2011-06-28 04:59:57]

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

mime/multipart: parse LF-delimited messages, not just CRLF

Against the spec, but appear in the wild.

Fixes issue 1966

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

[bradfitz, 2011-06-28 05:00:24]

Done.

Thanks for helping clean this up.

On Mon, Jun 27, 2011 at 9:55 PM, <adg@golang.org> wrote:

> LGTM
>
> Nice.
>
>
> http://codereview.appspot.com/**4662059/diff/5003/src/pkg/**
>
mime/multipart/multipart.go<http://codereview.appspot.com/4662059/diff/5003/src/pkg/mime/multipart/multipart.go>
>
> File src/pkg/mime/multipart/**multipart.go (right):
>
> http://codereview.appspot.com/**4662059/diff/5003/src/pkg/**
>
mime/multipart/multipart.go#**newcode27<http://codereview.appspot.com/4662059/diff/5003/src/pkg/mime/multipart/multipart.go#newcode27>
> src/pkg/mime/multipart/**multipart.go:27: var lf = []byte("\n")
> maybe put
> // TODO(bradfitz): inline these once the compiler is smarter
>
>
>
http://codereview.appspot.com/**4662059/<http://codereview.appspot.com/4662059/>
>