From d4c55e66d8c3a2f3382d264b08e3e3454a66355a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 18 Oct 2016 08:54:36 +0000 Subject: [PATCH] [release-branch.go1.7] http2: never Read from Request.Body in Transport to determine ContentLength Updates golang/go#17480 (fixes after vendor to std) Updates golang/go#17071 (a better fix than the original one) Change-Id: Ibb49d2cb825e28518e688b5c3e0952956a305050 Reviewed-on: https://go-review.googlesource.com/31326 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Chris Broadfoot Reviewed-on: https://go-review.googlesource.com/31361 Run-TryBot: Chris Broadfoot Reviewed-by: Brad Fitzpatrick --- http2/transport.go | 36 ++++++++++-------------------------- http2/transport_test.go | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index 57620395..7ea513e0 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -610,34 +610,17 @@ func checkConnHeaders(req *http.Request) error { return nil } -func bodyAndLength(req *http.Request) (body io.Reader, contentLen int64) { - body = req.Body - if body == nil { - return nil, 0 +// actualContentLength returns a sanitized version of +// req.ContentLength, where 0 actually means zero (not unknown) and -1 +// means unknown. +func actualContentLength(req *http.Request) int64 { + if req.Body == nil { + return 0 } if req.ContentLength != 0 { - return req.Body, req.ContentLength + return req.ContentLength } - - // We have a body but a zero content length. Test to see if - // it's actually zero or just unset. - var buf [1]byte - n, rerr := body.Read(buf[:]) - if rerr != nil && rerr != io.EOF { - return errorReader{rerr}, -1 - } - if n == 1 { - // Oh, guess there is data in this Body Reader after all. - // The ContentLength field just wasn't set. - // Stitch the Body back together again, re-attaching our - // consumed byte. - if rerr == io.EOF { - return bytes.NewReader(buf[:]), 1 - } - return io.MultiReader(bytes.NewReader(buf[:]), body), -1 - } - // Body is actually zero bytes. - return nil, 0 + return -1 } func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) { @@ -658,8 +641,9 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) { return nil, errClientConnUnusable } - body, contentLen := bodyAndLength(req) + body := req.Body hasBody := body != nil + contentLen := actualContentLength(req) // TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere? var requestedGzip bool diff --git a/http2/transport_test.go b/http2/transport_test.go index bfc54000..dcf7e59b 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -374,6 +374,40 @@ func randString(n int) string { return string(b) } +type panicReader struct{} + +func (panicReader) Read([]byte) (int, error) { panic("unexpected Read") } +func (panicReader) Close() error { panic("unexpected Close") } + +func TestActualContentLength(t *testing.T) { + tests := []struct { + req *http.Request + want int64 + }{ + // Verify we don't read from Body: + 0: { + req: &http.Request{Body: panicReader{}}, + want: -1, + }, + // nil Body means 0, regardless of ContentLength: + 1: { + req: &http.Request{Body: nil, ContentLength: 5}, + want: 0, + }, + // ContentLength is used if set. + 2: { + req: &http.Request{Body: panicReader{}, ContentLength: 5}, + want: 5, + }, + } + for i, tt := range tests { + got := actualContentLength(tt.req) + if got != tt.want { + t.Errorf("test[%d]: got %d; want %d", i, got, tt.want) + } + } +} + func TestTransportBody(t *testing.T) { bodyTests := []struct { body string @@ -381,8 +415,6 @@ func TestTransportBody(t *testing.T) { }{ {body: "some message"}, {body: "some message", noContentLen: true}, - {body: ""}, - {body: "", noContentLen: true}, {body: strings.Repeat("a", 1<<20), noContentLen: true}, {body: strings.Repeat("a", 1<<20)}, {body: randString(16<<10 - 1)},