net/http: panic due to racy read of persistConn after handler panic

[neild]

Let's start with the reproduction case:

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()
}

There might be something simpler, but this consistently panics for me on multiple systems. The above is a bit contrived, but the panic consistently occurs in some much less contrived scenarios with httputil.ReverseProxy, which likes to panic(ErrAbortHandler) when it encounters a problem copying a proxied response body.

The root cause is (I think) that we don't close the request body after a handler panic. We do close the *conn, returning its bufio.Reader and bufio.Writer to their sync.Pools. But the request body has an indirect reference to the *conn's reader, and while the handler has exited, some other goroutine may still be reading from it.

[gopherbot]

Change https://golang.org/cl/329922 mentions this issue: net/http: close request body after recovering from a handler panic

[FiloSottile]

I am not convinced this is a net/http issue. The Handler docs say that reading from Body after ServeHTTP has returned is not ok.

Returning signals that the request is finished; it is not valid to use the ResponseWriter or read from the Request.Body after or concurrently with the completion of the ServeHTTP call.

Given that, it sounds like the issue is that ReverseProxy will do that, rather than that net/http won't handle it. Is that correct?

[neild]

Reading from Body after a ServeHTTP Handler returns is not okay, but it should be "get a read error" not okay, not "panic the process" not okay.

There may be other fixes possible here--this is a complex interaction between the HTTP server and client, with RoundTripper in the middle--but I think the most robust defense is to ensure that use of a Body after a handler has returned is not unsafe.

Also, the Request.Body documentation states:

// The Server will close the request body. The ServeHTTP
// Handler does not need to.

The problem is that the Server does not close the request body if the Handler panics.

[FiloSottile]

Reading from Body after a ServeHTTP Handler returns is not okay, but it should be "get a read error" not okay, not "panic the process" not okay.

I've always interpreted "is not valid" as "might blow up", just like writing to the same memory from two concurrent goroutines is not valid. Why should this one be expected to fail gracefully?

The problem is that the Server does not close the request body if the Handler panics.

I feel like that's a problem, since there might be behavior that replies on the body getting closed, but I don't read the Request.Body documentation as "you can do the thing we said is not valid above because we promise to close the body and you can assume that cleanly interrupts the read".

[neild]

Why should this one be expected to fail gracefully?

Well, for one thing, it does fail gracefully if you don't panic in a handler.

For another, the way it fails is very bad: You can have two bodies reading from the same TCP connection at the same time. We detect the conflict and panic, but I'd much rather not let that happen. And it doesn't happen...unless you panic in a handler.

And for another, if you look at the RoundTripper implementation, it is very much not obvious to me that it is doing anything wrong: It makes an outbound request using an inbound request body, reads the response to that request, encounters an error, and panics with http.ErrAbortHandler. This all seems fine on the surface, but the outbound request is sneakily still holding on to the request body and may read from it again.

I suppose RoundTripper could close the request body itself (although the Request.Body documentation explicitly says "The ServeHTTP handler does not need to [close the request body]), but why not entirely eliminate this subtle class of bugs by fixing the one corner case where the request body doesn't get closed?

[neild]

I think the summary of my position here is that if we want to panic when something reads from a Request.Body after the handler has returned, we should do so with an intentional error that says exactly what you did, and we should do it regardless of whether the handler returned normally or panicked. (I think that swallowing HTTP handler panics is unfortunate, but the net/http package explicitly supports a panic as an acceptable way of exiting a handler.)

Currently, we do not panic with an intentional error (we mostly give a really confusing nil dereference after improperly recycling a cached object that still has a live reference via the request body, or occasionally an exciting error resulting from the recycled object now being in use on another connection), and we only do it if the handler panics.

Upgrading the existing error case (where you read from a Request.Body after a normal handler return) to a panic seems like a non-starter, so we should instead make the panic case an error.

[FiloSottile]

Fair enough, if we can consistently make a failure case more predictable/understandable/graceful, I am all for it. We can even change the docs to say something less scary than "is not valid", if you think we can be always graceful with it.

However, that's not a critical nor a security fix, so it's not something we should backport. Especially in code so delicate and complex, shipping a fix without the testing period of the beta and the rc is a real risk.

ReverseProxy is doing something that's not valid, and that leads to a panic in regular operations. That's a critical fix, maybe even a security fix. Unless you think fixing this in ReverseProxy is just too complicated, keeping the backported fix to net/http/httputil feels much safer to me, and then we can change net/http in Go 1.18.

[gopherbot]

Change https://golang.org/cl/333191 mentions this issue: net/http/httputil: close incoming ReverseProxy request body

[neild]

CL 333191 fixes the crash in ReverseProxy, although not in an entirely principled fashion--it ensures that the incoming Request.Body is always closed when ReverseProxy.ServeHTTP returns, but it is still possible that an in-flight request will read from the (safely closed) body after the handler returns.

I suppose an actual principled fix would be to wrap the incoming request body with something that lets us ensure the outgoing request RoundTripper will not read from the incoming request body after we're done, but I just counted nine(!!!) levels of wrapping on this request body already and am loath to add another one.

[FiloSottile]

@gopherbot, please open backport issues for CL 333191 as a potential security fix.

[gopherbot]

Backport issue(s) opened: #47473 (for 1.15), #47474 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.