code review 5272049: textproto: prevent long lines in HTTP headers from caus...

textproto: prevent long lines in HTTP headers from causing HTTP 400 responses.

This fixes the issue without an extra copy in the average case.

[msolo, 2011-10-15 06:12:31]

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

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

[ality, 2011-10-15 07:58:12]

You can avoid the nesting by doing this instead:

func (r *Reader) readLineSlice() ([]byte, os.Error) {
	r.closeDot()
	var line []byte
	for {
		l, more, err := r.R.ReadLine()
		if err != nil {
			return line, err
		}
		// Avoid the copy if the first call produced a full line.
		if line == nil && !more {
			return l, nil
		}
		line = append(line, l...)
		if !more {
			break
		}
	}
	return line, nil
}

I found myself writing a very simliar function
so many times and eventually factored it out
into my personal utility library (calling it
util.Getline).

Cheers,
  Anthony

[msolo, 2011-10-18 06:51:12]

Hello golang-dev@googlegroups.com, ality@pbrane.org (cc: bradfitz@golang.org,
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-10-18 13:33:09]

http://codereview.appspot.com/5272049/diff/6001/src/pkg/net/textproto/reader_...
File src/pkg/net/textproto/reader_test.go (right):

http://codereview.appspot.com/5272049/diff/6001/src/pkg/net/textproto/reader_...
src/pkg/net/textproto/reader_test.go:154: t.Fatalf("ReadMIMEHeader: %v, %v; want
%v", m, err, want)
this won't fail with a pretty or useful error message. we'll get scores of exes.
 as fun as that is, it should probably just print the lengths of the fields.  I
guess that'd involve not using DeepEqual and instead comparing the len(map) and
len(m["Cookie"]) first before comparing the cookie value.

[msolo, 2011-10-18 18:51:55]

Hello golang-dev@googlegroups.com, ality@pbrane.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-10-18 19:01:33]

http://codereview.appspot.com/5272049/diff/13001/src/pkg/net/textproto/reader...
File src/pkg/net/textproto/reader_test.go (right):

http://codereview.appspot.com/5272049/diff/13001/src/pkg/net/textproto/reader...
src/pkg/net/textproto/reader_test.go:151: t.Fatalf("ReadMIMEHeader: failed with
err %v", err)
you could remove "failed with err" here.  it'll be obvious from context.

http://codereview.appspot.com/5272049/diff/13001/src/pkg/net/textproto/reader...
src/pkg/net/textproto/reader_test.go:155: t.Fatalf("ReadMIMEHeader: %v bytes,
%v; want %v bytes", len(cookie), len(sdata))
three %v but 2 values

[bradfitz, 2011-10-18 19:29:04]

http://codereview.appspot.com/5272049/diff/13001/src/pkg/net/textproto/reader.go
File src/pkg/net/textproto/reader.go (right):

http://codereview.appspot.com/5272049/diff/13001/src/pkg/net/textproto/reader...
src/pkg/net/textproto/reader.go:57: return line, err
bufio.Reader.ReadLine is documented to say:

"ReadLine either returns a non-nil line or it returns an error, never both."

In the old code, readLineSlice also had this property.

In the new code, line might be non-nil from a previous call to ReadLine +
append.

You should probably just:

return nil, err

[msolo, 2011-10-18 20:32:57]

Hello golang-dev@googlegroups.com, ality@pbrane.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[ality, 2011-10-18 21:13:20]

bradfitz@golang.org once said:
> src/pkg/net/textproto/reader.go:57: return line, err
> bufio.Reader.ReadLine is documented to say:
> 
> "ReadLine either returns a non-nil line or it returns
> an error, never both."
> 
> In the old code, readLineSlice also had this property.
> 
> In the new code, line might be non-nil from a previous
> call to ReadLine + append.
> 
> You should probably just:
> 
> return nil, err

If you do that, you'll also need to test for os.EOF.

  Anthony

[msolo, 2011-10-18 22:54:39]

On Tue, Oct 18, 2011 at 2:13 PM, Anthony Martin <ality@pbrane.org> wrote:
> bradfitz@golang.org once said:
>> src/pkg/net/textproto/reader.go:57: return line, err
>> bufio.Reader.ReadLine is documented to say:
>>
>> "ReadLine either returns a non-nil line or it returns
>> an error, never both."

I think this documentation is actually wrong - I get an empty byte
array and os.EOF when I reach the end using the bufio code. I remember
hitting this once before.

Actually, I think the code I have here in textproto conforms to the
intent of the documentation in bufio more than the reference API.

>>
>> In the old code, readLineSlice also had this property.
>>
>> In the new code, line might be non-nil from a previous
>> call to ReadLine + append.
>>
>> You should probably just:
>>
>> return nil, err
>
> If you do that, you'll also need to test for os.EOF.
>
>  Anthony
>

[msolo, 2011-10-19 21:49:21]

Any further comment or is this good to go?

On Tue, Oct 18, 2011 at 3:54 PM, Mike Solomon <msolo@gmail.com> wrote:
> On Tue, Oct 18, 2011 at 2:13 PM, Anthony Martin <ality@pbrane.org> wrote:
>> bradfitz@golang.org once said:
>>> src/pkg/net/textproto/reader.go:57: return line, err
>>> bufio.Reader.ReadLine is documented to say:
>>>
>>> "ReadLine either returns a non-nil line or it returns
>>> an error, never both."
>
> I think this documentation is actually wrong - I get an empty byte
> array and os.EOF when I reach the end using the bufio code. I remember
> hitting this once before.
>
> Actually, I think the code I have here in textproto conforms to the
> intent of the documentation in bufio more than the reference API.
>
>>>
>>> In the old code, readLineSlice also had this property.
>>>
>>> In the new code, line might be non-nil from a previous
>>> call to ReadLine + append.
>>>
>>> You should probably just:
>>>
>>> return nil, err
>>
>> If you do that, you'll also need to test for os.EOF.
>>
>>  Anthony
>>
>

[bradfitz, 2011-10-19 22:29:25]

I was going to look into the claimed bufio doc bug first before I was
confident.

Did you want to document / test that first?
On Oct 19, 2011 2:49 PM, "Mike Solomon" <msolo@gmail.com> wrote:

> Any further comment or is this good to go?
>
> On Tue, Oct 18, 2011 at 3:54 PM, Mike Solomon <msolo@gmail.com> wrote:
> > On Tue, Oct 18, 2011 at 2:13 PM, Anthony Martin <ality@pbrane.org>
> wrote:
> >> bradfitz@golang.org once said:
> >>> src/pkg/net/textproto/reader.go:57: return line, err
> >>> bufio.Reader.ReadLine is documented to say:
> >>>
> >>> "ReadLine either returns a non-nil line or it returns
> >>> an error, never both."
> >
> > I think this documentation is actually wrong - I get an empty byte
> > array and os.EOF when I reach the end using the bufio code. I remember
> > hitting this once before.
> >
> > Actually, I think the code I have here in textproto conforms to the
> > intent of the documentation in bufio more than the reference API.
> >
> >>>
> >>> In the old code, readLineSlice also had this property.
> >>>
> >>> In the new code, line might be non-nil from a previous
> >>> call to ReadLine + append.
> >>>
> >>> You should probably just:
> >>>
> >>> return nil, err
> >>
> >> If you do that, you'll also need to test for os.EOF.
> >>
> >>  Anthony
> >>
> >
>

[msolo, 2011-11-01 07:05:55]

Got a little side tracked so just picking this back up now.

This test demonstrates the issue with the bufio ReadLine function docs.

func TestReadLineDocs(t *testing.T) {
  r := bufio.NewReader(strings.NewReader(""))
  l, _, err := r.ReadLine()
  if l != nil && err != nil {
    t.Fatalf("ReadLine either returns a non-nil line or it returns an
error, never both, but it does... l: %v err:%v", l, err)
  }
}

This always errors out and my reading of that phrase in the docs makes
me expect this should pass.

In terms of the original patch, I'm confident it works as intended.

On Wed, Oct 19, 2011 at 3:29 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I was going to look into the claimed bufio doc bug first before I was
> confident.
>
> Did you want to document / test that first?
>
> On Oct 19, 2011 2:49 PM, "Mike Solomon" <msolo@gmail.com> wrote:
>>
>> Any further comment or is this good to go?
>>
>> On Tue, Oct 18, 2011 at 3:54 PM, Mike Solomon <msolo@gmail.com> wrote:
>> > On Tue, Oct 18, 2011 at 2:13 PM, Anthony Martin <ality@pbrane.org>
>> > wrote:
>> >> bradfitz@golang.org once said:
>> >>> src/pkg/net/textproto/reader.go:57: return line, err
>> >>> bufio.Reader.ReadLine is documented to say:
>> >>>
>> >>> "ReadLine either returns a non-nil line or it returns
>> >>> an error, never both."
>> >
>> > I think this documentation is actually wrong - I get an empty byte
>> > array and os.EOF when I reach the end using the bufio code. I remember
>> > hitting this once before.
>> >
>> > Actually, I think the code I have here in textproto conforms to the
>> > intent of the documentation in bufio more than the reference API.
>> >
>> >>>
>> >>> In the old code, readLineSlice also had this property.
>> >>>
>> >>> In the new code, line might be non-nil from a previous
>> >>> call to ReadLine + append.
>> >>>
>> >>> You should probably just:
>> >>>
>> >>> return nil, err
>> >>
>> >> If you do that, you'll also need to test for os.EOF.
>> >>
>> >>  Anthony
>> >>
>> >
>

[bradfitz, 2011-11-01 17:29:26]

LGTM (on original patch) and thanks for the bufio test.  Sent out a little
fix for that.

On Tue, Nov 1, 2011 at 12:05 AM, Mike Solomon <msolo@gmail.com> wrote:

> Got a little side tracked so just picking this back up now.
>
> This test demonstrates the issue with the bufio ReadLine function docs.
>
> func TestReadLineDocs(t *testing.T) {
>  r := bufio.NewReader(strings.NewReader(""))
>  l, _, err := r.ReadLine()
>  if l != nil && err != nil {
>    t.Fatalf("ReadLine either returns a non-nil line or it returns an
> error, never both, but it does... l: %v err:%v", l, err)
>  }
> }
>
> This always errors out and my reading of that phrase in the docs makes
> me expect this should pass.
>
> In terms of the original patch, I'm confident it works as intended.
>
> On Wed, Oct 19, 2011 at 3:29 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > I was going to look into the claimed bufio doc bug first before I was
> > confident.
> >
> > Did you want to document / test that first?
> >
> > On Oct 19, 2011 2:49 PM, "Mike Solomon" <msolo@gmail.com> wrote:
> >>
> >> Any further comment or is this good to go?
> >>
> >> On Tue, Oct 18, 2011 at 3:54 PM, Mike Solomon <msolo@gmail.com> wrote:
> >> > On Tue, Oct 18, 2011 at 2:13 PM, Anthony Martin <ality@pbrane.org>
> >> > wrote:
> >> >> bradfitz@golang.org once said:
> >> >>> src/pkg/net/textproto/reader.go:57: return line, err
> >> >>> bufio.Reader.ReadLine is documented to say:
> >> >>>
> >> >>> "ReadLine either returns a non-nil line or it returns
> >> >>> an error, never both."
> >> >
> >> > I think this documentation is actually wrong - I get an empty byte
> >> > array and os.EOF when I reach the end using the bufio code. I remember
> >> > hitting this once before.
> >> >
> >> > Actually, I think the code I have here in textproto conforms to the
> >> > intent of the documentation in bufio more than the reference API.
> >> >
> >> >>>
> >> >>> In the old code, readLineSlice also had this property.
> >> >>>
> >> >>> In the new code, line might be non-nil from a previous
> >> >>> call to ReadLine + append.
> >> >>>
> >> >>> You should probably just:
> >> >>>
> >> >>> return nil, err
> >> >>
> >> >> If you do that, you'll also need to test for os.EOF.
> >> >>
> >> >>  Anthony
> >> >>
> >> >
> >
>

[bradfitz, 2011-11-01 17:31:42]

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

textproto: prevent long lines in HTTP headers from causing HTTP 400 responses.

This fixes the issue without an extra copy in the average case.

R=golang-dev, ality, bradfitz
CC=golang-dev
http://codereview.appspot.com/5272049

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[ality, 2011-12-11 11:09:10]

Hey Mike, can you please hg sync so this can
be closed on the codereview server and removed
from everyone's "Issues CCd to me" list?

Thanks,
  Anthony

[rsc, 2011-12-12 21:15:56]

On Sun, Dec 11, 2011 at 06:09,  <ality@pbrane.org> wrote:
> Hey Mike, can you please hg sync so this can
> be closed on the codereview server and removed
> from everyone's "Issues CCd to me" list?

the way to do this w/o mike is to reply via
the code review interface and edit the r/cc list
in the process.  i usually just set CC=golang-dev.

[msolo, 2011-12-12 23:46:21]

I ran the sync this morning, hopefully that cleaned it up.

On Mon, Dec 12, 2011 at 1:16 PM,  <rsc@golang.org> wrote:
> https://codereview.appspot.com/5272049/