net/http: close request body after recovering from a handler panic

When recovering from a panic in a HTTP handler, close the request body
before closing the *conn, ensuring that the *conn's bufio.Reader is safe
to recycle.

Fixes #46866.

Change-Id: I3fe304592e3b423a0970727d68bc1229c3752939
Reviewed-on: https://go-review.googlesource.com/c/go/+/329922
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 5b113cff970dc19148c5ba4fe96020b54bfd0d5a..4d0ce5619fe1d6b8e5dc0632de2146a4656ffbf6 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1794,6 +1794,7 @@ func isCommonNetReadError(err error) bool {
 func (c *conn) serve(ctx context.Context) {
        c.remoteAddr = c.rwc.RemoteAddr().String()
        ctx = context.WithValue(ctx, LocalAddrContextKey, c.rwc.LocalAddr())
+       var inFlightResponse *response
        defer func() {
                if err := recover(); err != nil && err != ErrAbortHandler {
                        const size = 64 << 10
@@ -1801,7 +1802,14 @@ func (c *conn) serve(ctx context.Context) {
                        buf = buf[:runtime.Stack(buf, false)]
                        c.server.logf("http: panic serving %v: %v\n%s", c.remoteAddr, err, buf)
                }
+               if inFlightResponse != nil {
+                       inFlightResponse.cancelCtx()
+               }
                if !c.hijacked() {
+                       if inFlightResponse != nil {
+                               inFlightResponse.conn.r.abortPendingRead()
+                               inFlightResponse.reqBody.Close()
+                       }
                        c.close()
                        c.setState(c.rwc, StateClosed, runHooks)
                }
@@ -1926,7 +1934,9 @@ func (c *conn) serve(ctx context.Context) {
                // in parallel even if their responses need to be serialized.
                // But we're not going to implement HTTP pipelining because it
                // was never deployed in the wild and the answer is HTTP/2.
+               inFlightResponse = w
                serverHandler{c.server}.ServeHTTP(w, w.req)
+               inFlightResponse = nil
                w.cancelCtx()
                if c.hijacked() {
                        return

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index eeaa49264450de6785415669622801410d8abca9..0cdd946de4269a4e2fc50844d031944bcbc4303b 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -6512,3 +6512,32 @@ func TestCancelRequestWhenSharingConnection(t *testing.T) {
        close(r2c)
        wg.Wait()
 }
+
+func TestHandlerAbortRacesBodyRead(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+
+       ts := httptest.NewServer(HandlerFunc(func(rw ResponseWriter, req *Request) {
+               go io.Copy(io.Discard, req.Body)
+               panic(ErrAbortHandler)
+       }))
+       defer ts.Close()
+
+       var wg sync.WaitGroup
+       for i := 0; i < 2; i++ {
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       for j := 0; j < 10; j++ {
+                               const reqLen = 6 * 1024 * 1024
+                               req, _ := NewRequest("POST", ts.URL, &io.LimitedReader{R: neverEnding('x'), N: reqLen})
+                               req.ContentLength = reqLen
+                               resp, _ := ts.Client().Transport.RoundTrip(req)
+                               if resp != nil {
+                                       resp.Body.Close()
+                               }
+                       }
+               }()
+       }
+       wg.Wait()
+}