net/http/httputil: wrap ReverseProxy's outbound request body so Close is a noop

Within ReverseProxy, we are currently sending a clone of our inbound
request (from client) as our outbound request (to upstream). However,
the clone of the request has a shallow copy of the request body. As a
result, when the outbound request body is closed, the inbound request
body (i.e. the outbound request body of the client) will also be closed.

This causes an unfortunate effect where we would infinitely hang when a
client sends a request with a 100-continue header via a ReverseProxy,
but the ReverseProxy fails to make a connection to the upstream server.

When this happens, the ReverseProxy's outbound request body would be
closed, which in turns also closes the client's request body.
Internally, when we close a request body, we would try to consume and
discard the content. Since the client has yet to actually send the body
content (due to 100-continue header) though, an infinite hang occurs.

To prevent this, we make sure that closing an outbound request body from
a ReverseProxy is a noop.

For #75933

Change-Id: I52dc7247f689f35a6e93d1f32b2d003d90e9d2c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/722160
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 9d8784cd2bc7a9c879c380e854723a05df461ed3..7c220319c4d70c082c49770dc7054ea7f6c67427 100644 (file)
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -21,6 +21,7 @@ import (
        "net/url"
        "strings"
        "sync"
+       "sync/atomic"
        "time"
 
        "golang.org/x/net/http/httpguts"
@@ -435,6 +436,18 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
                outreq.Body = nil // Issue 16036: nil Body for http.Transport retries
        }
        if outreq.Body != nil {
+               // Wrap the body in a reader where Close does nothing. This is done
+               // because p.Transport.RoundTrip would close the reverse proxy's
+               // outbound request body if it fails to connect to upstream. If we do
+               // not wrap the body, when we close the reverse proxy's outbound
+               // request, it will also close the reverse proxy's inbound request body
+               // (i.e. the client's outbound request body). This is because
+               // http.(*Request).Clone creates a shallow copy of the body. This can
+               // cause an infinite hang in cases where the body is not yet received
+               // from the client (e.g. 100-continue requests): Close, which
+               // internally tries to consume the body content, would be called too
+               // early and would hang.
+               outreq.Body = &noopCloseReader{readCloser: outreq.Body}
                // Reading from the request body after returning from a handler is not
                // allowed, and the RoundTrip goroutine that reads the Body can outlive
                // this handler. This can lead to a crash if the handler panics (see
@@ -941,3 +954,20 @@ func ishex(c byte) bool {
        }
        return false
 }
+
+type noopCloseReader struct {
+       readCloser io.ReadCloser
+       closed     atomic.Bool
+}
+
+func (ncr *noopCloseReader) Close() error {
+       ncr.closed.Store(true)
+       return nil
+}
+
+func (ncr *noopCloseReader) Read(p []byte) (int, error) {
+       if ncr.closed.Load() {
+               return 0, errors.New("ReverseProxy does an invalid Read on closed Body")
+       }
+       return ncr.readCloser.Read(p)
+}

diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 62c93fb261a6e5b5782501517fe08e4eba9b59b0..3cca6eda992670f0d9e6161d57830eeb8c435dd6 100644 (file)
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -2149,6 +2149,44 @@ func TestReverseProxyHijackCopyError(t *testing.T) {
        proxyHandler.ServeHTTP(rw, req)
 }
 
+// https://go.dev/issue/75933.
+func TestReverseProxyInvalidUpstream100ContinueDoNotHang(t *testing.T) {
+       proxy := ReverseProxy{
+               Transport: &http.Transport{DisableKeepAlives: true, ExpectContinueTimeout: time.Second * 60},
+               Director: func(request *http.Request) {
+                       request.URL.Scheme = "http"
+                       request.URL.Host = "doesnotexist:12345" // non-existent upstream
+               },
+       }
+       handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+               proxy.ServeHTTP(w, r)
+       })
+       upstreamServer := httptest.NewServer(handler)
+       defer upstreamServer.Close()
+
+       conn, err := net.Dial("tcp", upstreamServer.Listener.Addr().String())
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer conn.Close()
+
+       requestBody := `{"test": "data"}`
+       initialRequest := fmt.Sprintf("POST %s/test-expect HTTP/1.1\r\n"+
+               "Host: %s\r\n"+
+               "Content-Type: application/json\r\n"+
+               "Content-Length: %d\r\n"+
+               "Expect: 100-continue\r\n"+
+               "\r\n", upstreamServer.URL, upstreamServer.Listener.Addr().String(), len(requestBody))
+
+       if _, err := conn.Write([]byte(initialRequest)); err != nil {
+               log.Fatal(err)
+       }
+       buff := make([]byte, 1024)
+       if _, err := conn.Read(buff); err != nil {
+               log.Fatal(err)
+       }
+}
+
 type testResponseWriter struct {
        h           http.Header
        writeHeader func(int)