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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Chris Broadfoot <[email protected]>

Author: bradfitz

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

http2/transport_test.go

@@ -391,15 +391,47 @@ 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
 		noContentLen bool
 	}{
 		{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)},