From 410d19ee5650d9c64ed4392cb046ab7fde0d86cc Mon Sep 17 00:00:00 2001
From: Damien Neil <dneil@google.com>
Date: Fri, 17 May 2024 14:29:29 -0700
Subject: [PATCH] http2: avoid racy access to clientStream.requestedGzip

clientStream.requestedGzip is set from clientStream.writeRequest,
and examined by clientConn.readLoop. I'm not sure if there's
any possible way for an actual data race to happen here in
practice, since the read loop should only examine the field
after the request is sent by writeRequest, but it's enough
for the race detector to complain.

Set the field in ClientConn.roundTrip instead, before
the clientStream has become accessible to any other goroutines.

No test, but a following CL has race detector failures without
this change.

Change-Id: Id30f1b95bcfcc35c213440e0e47cce3feaaff06d
Reviewed-on: https://go-review.googlesource.com/c/net/+/586245
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
---
 http2/transport.go      | 40 ++++++++++++++++++++--------------------
 http2/transport_test.go |  1 +
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/http2/transport.go b/http2/transport.go
index 040d9327c..2cd3c6ec7 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1301,6 +1301,26 @@ func (cc *ClientConn) roundTrip(req *http.Request, streamf func(*clientStream))
 		donec:                make(chan struct{}),
 	}
 
+	// TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere?
+	if !cc.t.disableCompression() &&
+		req.Header.Get("Accept-Encoding") == "" &&
+		req.Header.Get("Range") == "" &&
+		!cs.isHead {
+		// Request gzip only, not deflate. Deflate is ambiguous and
+		// not as universally supported anyway.
+		// See: https://zlib.net/zlib_faq.html#faq39
+		//
+		// Note that we don't request this for HEAD requests,
+		// due to a bug in nginx:
+		//   http://trac.nginx.org/nginx/ticket/358
+		//   https://golang.org/issue/5522
+		//
+		// We don't request gzip if the request is for a range, since
+		// auto-decoding a portion of a gzipped document will just fail
+		// anyway. See https://golang.org/issue/8923
+		cs.requestedGzip = true
+	}
+
 	go cs.doRequest(req, streamf)
 
 	waitDone := func() error {
@@ -1449,26 +1469,6 @@ func (cs *clientStream) writeRequest(req *http.Request, streamf func(*clientStre
 		streamf(cs)
 	}
 
-	// TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere?
-	if !cc.t.disableCompression() &&
-		req.Header.Get("Accept-Encoding") == "" &&
-		req.Header.Get("Range") == "" &&
-		!cs.isHead {
-		// Request gzip only, not deflate. Deflate is ambiguous and
-		// not as universally supported anyway.
-		// See: https://zlib.net/zlib_faq.html#faq39
-		//
-		// Note that we don't request this for HEAD requests,
-		// due to a bug in nginx:
-		//   http://trac.nginx.org/nginx/ticket/358
-		//   https://golang.org/issue/5522
-		//
-		// We don't request gzip if the request is for a range, since
-		// auto-decoding a portion of a gzipped document will just fail
-		// anyway. See https://golang.org/issue/8923
-		cs.requestedGzip = true
-	}
-
 	continueTimeout := cc.t.expectContinueTimeout()
 	if continueTimeout != 0 {
 		if !httpguts.HeaderValuesContainsToken(req.Header["Expect"], "100-continue") {
diff --git a/http2/transport_test.go b/http2/transport_test.go
index d875338a2..2171359ca 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -2895,6 +2895,7 @@ func TestRoundTripDoesntConsumeRequestBodyEarly(t *testing.T) {
 	cc := &ClientConn{
 		closed:      true,
 		reqHeaderMu: make(chan struct{}, 1),
+		t:           &Transport{},
 	}
 	_, err := cc.RoundTrip(req)
 	if err != errClientConnUnusable {