code review 6739055: net/http: add missing error checking reading trailers

net/http: add missing error checking reading trailers

This is a simplified version of earlier versions of this CL
and now only fixes obviously incorrect things, without
changing the locking on bodyEOFReader.

I'd like to see if this is sufficient before changing the
locking.

Update issue 4191

[bradfitz, 2012-10-22 21:26:03]

Hello golang-dev@googlegroups.com,

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

[rsc, 2012-11-01 16:28:36]

LGTM

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go
File src/pkg/net/http/transfer.go (right):

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go#ne...
src/pkg/net/http/transfer.go:544: err = io.ErrUnexpectedEOF
Looks like readTrailer already does this for you?

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go#ne...
src/pkg/net/http/transfer.go:603: return err
Should this look for EOF and use the same error as line 583?

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer_test.go
File src/pkg/net/http/transfer_test.go (right):

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer_test....
src/pkg/net/http/transfer_test.go:1: package http
copyright

[bradfitz, 2012-11-13 21:46:46]

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

Please take another look.

[bradfitz, 2012-11-13 21:47:30]

PTAL

I removed the locking changes and updated the CL description.

I want to get this minimal safe CL in first, before any locking changes are
made.

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go
File src/pkg/net/http/transfer.go (right):

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go#ne...
src/pkg/net/http/transfer.go:544: err = io.ErrUnexpectedEOF
On 2012/11/01 16:28:36, rsc wrote:
> Looks like readTrailer already does this for you?

Done.

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go#ne...
src/pkg/net/http/transfer.go:603: return err
On 2012/11/01 16:28:36, rsc wrote:
> Should this look for EOF and use the same error as line 583?

Done.

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer_test.go
File src/pkg/net/http/transfer_test.go (right):

https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer_test....
src/pkg/net/http/transfer_test.go:1: package http
On 2012/11/01 16:28:36, rsc wrote:
> copyright

Done.

[bradfitz, 2012-11-14 05:11:47]

[+dustin]

Dustin, could you test this latest version of the patch?  It's only half of
the previous one you were happy with.  This is missing the locking changes,
but I hope they're unnecessary.  I'm hoping the trailer reading checks are
sufficient.

Any LGTMs on this version alone also welcome.  This version stands alone,
even if there's more to be done.


On Tue, Nov 13, 2012 at 1:47 PM, <bradfitz@golang.org> wrote:

> PTAL
>
> I removed the locking changes and updated the CL description.
>
> I want to get this minimal safe CL in first, before any locking changes
> are made.
>
>
> https://codereview.appspot.**com/6739055/diff/3/src/pkg/**
>
net/http/transfer.go<https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go>
> File src/pkg/net/http/transfer.go (right):
>
> https://codereview.appspot.**com/6739055/diff/3/src/pkg/**
>
net/http/transfer.go#**newcode544<https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go#newcode544>
> src/pkg/net/http/transfer.go:**544: err = io.ErrUnexpectedEOF
> On 2012/11/01 16:28:36, rsc wrote:
>
>> Looks like readTrailer already does this for you?
>>
>
> Done.
>
> https://codereview.appspot.**com/6739055/diff/3/src/pkg/**
>
net/http/transfer.go#**newcode603<https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer.go#newcode603>
> src/pkg/net/http/transfer.go:**603: return err
> On 2012/11/01 16:28:36, rsc wrote:
>
>> Should this look for EOF and use the same error as line 583?
>>
>
> Done.
>
> https://codereview.appspot.**com/6739055/diff/3/src/pkg/**
>
net/http/transfer_test.go<https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer_test.go>
> File src/pkg/net/http/transfer_**test.go (right):
>
> https://codereview.appspot.**com/6739055/diff/3/src/pkg/**
>
net/http/transfer_test.go#**newcode1<https://codereview.appspot.com/6739055/diff/3/src/pkg/net/http/transfer_test.go#newcode1>
> src/pkg/net/http/transfer_**test.go:1: package http
> On 2012/11/01 16:28:36, rsc wrote:
>
>> copyright
>>
>
> Done.
>
>
https://codereview.appspot.**com/6739055/<https://codereview.appspot.com/6739...
>

[dfc, 2012-11-14 05:31:22]

LGTM. Please fix the description

Update issue 4191.

[bradfitz, 2012-11-14 06:38:28]

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

net/http: add missing error checking reading trailers

This is a simplified version of earlier versions of this CL
and now only fixes obviously incorrect things, without
changing the locking on bodyEOFReader.

I'd like to see if this is sufficient before changing the
locking.

Update issue 4191

R=golang-dev, rsc, dave
CC=golang-dev
http://codereview.appspot.com/6739055