From a625e3953219464fdd5611bb48bf87c927717295 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 18 Oct 2016 08:54:36 +0000 Subject: [PATCH] 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 --- http2/transport.go | 41 ++++++++++------------------------------- http2/transport_test.go | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 33 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index b939fed2e..129c8e02b 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -635,39 +635,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 - } - // Don't try to sniff the size if they're doing an expect - // request (Issue 16002): - if req.Header.Get("Expect") == "100-continue" { - return req.Body, -1 - } - - // 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 + return req.ContentLength } - // Body is actually zero bytes. - return nil, 0 + return -1 } func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) { @@ -691,8 +669,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 c8600331f..f0af30ac4 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -391,6 +391,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 @@ -398,8 +432,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)},