code review 194074: Fix of issue 566.

Fix of issue 566. Increase header line limit. Current limit too low
for www.techcrunch.com e.g.

[petar-m, 2010-01-26 02:54:03]

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

I'd like you to review this change.

[rsc1, 2010-01-29 02:26:03]

switching to rsc

[rsc1, 2010-01-29 02:28:44]

http://codereview.appspot.com/194074/diff/3001/12
File src/pkg/http/request.go (right):

http://codereview.appspot.com/194074/diff/3001/12#newcode25
src/pkg/http/request.go:25: maxLineLength  = 4096 // assumed <
bufio.DefaultBufSize
// assumed <= bufio.defaultBufSize

http://codereview.appspot.com/194074/diff/3001/12#newcode141
src/pkg/http/request.go:141: 
drop blank line

http://codereview.appspot.com/194074/diff/3001/12#newcode142
src/pkg/http/request.go:142: if req.Host == "" {
This is a semantic change; should update the comment above
that says which fields get used.  Also, instead of editing req,
which might be problematic for the caller, I'd suggest
just maintaining a local variable host.

[petar-m, 2010-01-29 16:18:54]

take another look.

http://codereview.appspot.com/194074/diff/3001/12
File src/pkg/http/request.go (right):

http://codereview.appspot.com/194074/diff/3001/12#newcode25
src/pkg/http/request.go:25: maxLineLength  = 4096 // assumed <
bufio.DefaultBufSize
We need to discuss this more carefully.

First, why is this comment here?

The bufio.Reader is passed onto ReadRequest by the user. So ReadRequest has no
control over its size. Unfortunately, if the buffer is shorter than the line
length, readLine will never recognize the line. This is a problem. 

It seems to me that ReadRequest should be able to "resize" the
buffer to its needs if it is too small.

What do you think?

On 2010/01/29 02:28:44, rsc1 wrote:
> // assumed <= bufio.defaultBufSize
> 
>

http://codereview.appspot.com/194074/diff/3001/12#newcode141
src/pkg/http/request.go:141: 
On 2010/01/29 02:28:44, rsc1 wrote:
> drop blank line
> 

Done.

http://codereview.appspot.com/194074/diff/3001/12#newcode142
src/pkg/http/request.go:142: if req.Host == "" {
On 2010/01/29 02:28:44, rsc1 wrote:
> This is a semantic change; should update the comment above
> that says which fields get used.  Also, instead of editing req,
> which might be problematic for the caller, I'd suggest
> just maintaining a local variable host.
> 

Done.

[rsc, 2010-01-29 19:04:45]

> First, why is this comment here?
>
> The bufio.Reader is passed onto ReadRequest by the user. So ReadRequest
> has no control over its size. Unfortunately, if the buffer is shorter
> than the line length, readLine will never recognize the line. This is a
> problem.
>
> It seems to me that ReadRequest should be able to "resize" the
> buffer to its needs if it is too small.
>
> What do you think?

It can't resize the buffer.  That just seems antisocial, editing
a data structure that doesn't belong to it.

The point of the limit is to avoid a server sending back an
infinitely long line and running the program out of memory.
Using ReadSlice was a cheap way to do this with the
bufio interface.  If the limit needs to be bigger, we should
probably change the current ReadBytes in bufio to

func (*Reader) ReadBytesLimit(delim byte, max int)

that limits the number of bytes returned.  Then we can
put ReadBytes back as a call to ReadBytesLimit(delim, -1)
and http no longer depends on the buffer size.

For now though I'd suggest fixing the comment and
finishing this CL and then the bufio change can be a
separate one.

Russ

[petar_csail.mit.edu, 2010-01-30 00:47:33]

OK, uploaded the CL.

P


On Fri, Jan 29, 2010 at 2:04 PM, Russ Cox <rsc@golang.org> wrote:
>> First, why is this comment here?
>>
>> The bufio.Reader is passed onto ReadRequest by the user. So ReadRequest
>> has no control over its size. Unfortunately, if the buffer is shorter
>> than the line length, readLine will never recognize the line. This is a
>> problem.
>>
>> It seems to me that ReadRequest should be able to "resize" the
>> buffer to its needs if it is too small.
>>
>> What do you think?
>
> It can't resize the buffer.  That just seems antisocial, editing
> a data structure that doesn't belong to it.
>
> The point of the limit is to avoid a server sending back an
> infinitely long line and running the program out of memory.
> Using ReadSlice was a cheap way to do this with the
> bufio interface.  If the limit needs to be bigger, we should
> probably change the current ReadBytes in bufio to
>
> func (*Reader) ReadBytesLimit(delim byte, max int)
>
> that limits the number of bytes returned.  Then we can
> put ReadBytes back as a call to ReadBytesLimit(delim, -1)
> and http no longer depends on the buffer size.
>
> For now though I'd suggest fixing the comment and
> finishing this CL and then the bufio change can be a
> separate one.
>
> Russ
>

[rsc1, 2010-01-30 00:49:58]

LGTM

[rsc, 2010-01-30 00:51:45]

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

http: increase header line limit, let req.Host override req.URL.Host

Fixes issue 566.

R=rsc
CC=golang-dev
http://codereview.appspot.com/194074

Committer: Russ Cox <rsc@golang.org>