x/net/http2: reads client request body after close

[artem-anisimov-0x7f]

Go version

5095d0cf1463414ad99ced9d5032eae6175f5ac5

Output of go env in your module/workspace:

irrelevant to the bug

What did you do?

In a program of mine I have a reader that happens to validate that no accesses are made to it after it is closed. If this reader is passed as Request.Body to a HTTP/2 client, it sometimes detects a use-after-close if the request context is canceled half-way through. Here is a reproducer that can be added as a test to x/net/http2. While writing it, I found it out that there are in fact two failure modes in the HTTP/2 transport:

1. RoundTrip() returns an error, but cs.doRequest() keeps reading from the request body,
2. RoundTrip() keeps reading from the request body after it calls to closeReqBodyLocked().

Here is the reproducer. On my machine, 4 runs of every scenario are enough to hit the race. You may need to increase that number.

func TestNoReadsFromBodyAfterRoundTripFailed(t *testing.T) {
        ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {
                io.Copy(w, r.Body)
        })
        tr := &Transport{TLSClientConfig: tlsConfigInsecure}

        const (
                reqLen     = 1024 * 1024
                nrAttempts = 4
        )

        t.Run("user-of-transport-closes", func(t *testing.T) {
                for i := 0; i < nrAttempts; i++ {
                        ctx, cancel := context.WithCancel(context.Background())
                        r := &useAfterCloseValidatingReader{len: reqLen, cancel: cancel}

                        req, _ := http.NewRequestWithContext(ctx, "POST", ts.URL, r)
                        req.ContentLength = reqLen

                        resp, _ := tr.RoundTrip(req)
                        if resp != nil && resp.Body != nil {
                                resp.Body.Close()
                        }
                        r.Close()
                }
        })

        t.Run("transport-closes-by-itself", func(t *testing.T) {
                for i := 0; i < nrAttempts; i++ {
                        ctx, cancel := context.WithCancel(context.Background())
                        r := &useAfterCloseValidatingReader{len: reqLen, cancel: cancel}

                        req, _ := http.NewRequestWithContext(ctx, "POST", ts.URL, r)
                        req.ContentLength = reqLen

                        resp, _ := tr.RoundTrip(req)
                        if resp != nil && resp.Body != nil {
                                resp.Body.Close()
                        }
                        // tr closes r by itself
                }
        })
}

type useAfterCloseValidatingReader struct {
        len    int
        cancel func()
        closed atomic.Bool
}

func (r *useAfterCloseValidatingReader) Read(p []byte) (n int, err error) {
        if r.closed.Load() {
                // this runs in a goroutine different from the one where the test runs,
                // so a panic is the only option
                panic("Read() after Close()")
        }

        go r.cancel()

        if r.len == 0 {
                return 0, io.EOF
        }

        if len(p) > r.len {
                p = p[:r.len]
        }
        clear(p)

        r.len -= len(p)
        return len(p), nil
}

func (r *useAfterCloseValidatingReader) Close() error {
        r.closed.Store(true)
        return nil
}

What did you see happen?

HTTP/2 transport keeps reading from a closed Request.Body.

What did you expect to see?

HTTP/2 transport must make no reads from Request.Body after RoundTrip() returns.

[gabyhelp]

Related Issues

• net/http: Request body is not closed by Transport.RoundTrip on some net.Conn errors #49621 (closed)
• x/net/http2: indefinite hangs when closing response body #48908
• net/http: Re-connect with upgraded HTTP2 connection fails to send Request.body #32441 (closed)
• net/http: req.Body.Close called multiple times in http1, sometimes not called by the time Do returns on error #26408 (closed)
• net/http, x/net/http2: client loses the request body when retrying after HTTP2 server close connection gracefully(GOAWAY) #39086 (closed)
• x/net/http2: RoundTrip does not close request body after early errors #48341 (closed)
• net/http: request.Body.Close can be called more than once #40382 (closed)
• net/http: client doesn't check for canceled context (delegates it to the Transport) #46206

Related Code Changes

• net/http: avoid race when closing request body
• net/http: rewind request body on ErrSkipAltProtocol

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

so it only checks that the body was closed after it receives an error from read: https://go.googlesource.com/net/+/5095d0cf1463414ad99ced9d5032eae6175f5ac5/http2/transport.go#1908

which seems fine? NewRequestWithContext states that https://pkg.go.dev/net/http#NewRequestWithContext

If the provided body is also an io.Closer, the returned [Request.Body] is set to body and will be closed (possibly asynchronously) by the Client methods Do, Post, and PostForm, and Transport.RoundTrip.

note the "possibly asynchronously"

it could check before every attempt to read, but that feels like a possibly high performance hit for something that should be fine.

[artem-anisimov-0x7f]

I can convince myself that it is fine to return an error from RoundTrip(), and then close the reader some time later.

However, we have a different scenario here. The transport closes the reader, and keeps reading from it. There is no way asynchronicity can be an excuse for such behaviour.

[seankhliao]

it closes the reader expecting that closed readers return an error that allows it to stop copying (similar to how you might interrupt an ongoing io.Copy)

[xaurx]

@seankhliao this is unbelievably poor requirement and I'm sure incorrect one:

1. This means anybody can have a race:

• http.Do() returns
• application wants to retry request or reuse body for another request
• app calls fd.Seek(0)
• issues another http.Do() -- meanwhile old request goroutine STILL calls fd.Read() somewhere in background.
• RESULT: incorrect 2nd request content.

2. Imagine you essentially require to wait all reads to complete in Close().
3. Similar argument doesn't work with OS file descriptors as well. File descriptors may be reused, so essentially you may end up reading from the wrong file.

[seankhliao]

http.Client.Do returning is not sufficient: http supports bidirectional communications, returning only means a connection has been established.
Readers are generally expected to return an error after closing.
An os.File isn't valid past Close, and will return an appropriate error.

[artem-anisimov-0x7f]

I do not get your reasoning.

In my example there is no bidirectional communication. There is not even a http.Response returned from http.Do(). There is absolutely no way for me know that http.Client is done with Request.Body and when it may be reused for a retry in a way that does not trigger a race described by @xaurx.

[neild]

The http.Request.Body documentation states:

Body must allow Read to be called concurrently with Close. In particular, calling Close should unblock a Read waiting for input.

A concurrent call means that Close may be called before, during, or after a Read call. (It is not possible to write a program which calls Close to interrupt an in-progress read that will not potentially call Read after Close.) The io.ReadCloser used as a request body must support this. Read calls after Close should return an error (this requirement isn't stated explicitly, but I think it's implicit in the concurrency requirements).

This implies that a request body cannot be reused across requests. If the request body contains some resource (such as an *io.File) which will be reused in future requests, I would expect the request body to be a single-use type that contains a reference to the reusable resource.

[artem-anisimov-0x7f]

This implies that a request body cannot be reused across requests. If the request body contains some resource (such as an *io.File) which will be reused in future requests, I would expect the request body to be a single-use type that contains a reference to the reusable resource.

Ok, but how do I know when I may create another single-use wrapper around a reusable reader? It is only possible when the first single-use wrapper is done with reads. But, when http.Client.Do() returns an error, a read may still be in progress, and the API gives me no way to wait for it to complete.

An example of a read that runs after an error is returned from http.Client.Do():

func TestNoReadsFromBodyAfterRoundTripFailed1(t *testing.T) { run test
        ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {
                io.Copy(w, r.Body)
        })
        tr := &Transport{TLSClientConfig: tlsConfigInsecure}

        const (
                reqLen     = 64 * 1024 * 1024
                nrAttempts = 1024
        )

        for i := 0; i < nrAttempts; i++ {
                ctx, cancel := context.WithCancel(context.Background())
                r := &readAfterErrorReturnValidatingReader{len: reqLen, cancel: cancel}

                req, _ := http.NewRequestWithContext(ctx, "POST", ts.URL, r)
                req.ContentLength = reqLen

                resp, err := tr.RoundTrip(req)
                if err != nil {
                        r.returnedErr.Store(true)
                }
                if resp != nil && resp.Body != nil {
                        resp.Body.Close()
                }
        }
}

type readAfterErrorReturnValidatingReader struct {
        len         int
        cancel      func()
        closed      atomic.Bool
        returnedErr atomic.Bool
}

func (r *readAfterErrorReturnValidatingReader) Read(p []byte) (n int, err error) {
        if r.closed.Load() {
                return 0, errors.New("Read() after Close()")
        }

        go r.cancel()
        runtime.Gosched()

        if r.returnedErr.Load() {
                panic("Read() in progress after an error returned from RoundTrip()")
        }

        if r.len == 0 {
                return 0, io.EOF
        }

        if len(p) > r.len {
                p = p[:r.len]
        }
        clear(p)

        r.len -= len(p)
        return len(p), nil
}

func (r *readAfterErrorReturnValidatingReader) Close() error {
        r.closed.Store(true)
        return nil
}

Btw., the documentation only tells that Close() may be called asynchronously. It does not mention that a read may run in the background after http.Client.Do() returns an error.

[neild]

Ok, but how do I know when I may create another single-use wrapper around a reusable reader? It is only possible when the first single-use wrapper is done with reads. But, when http.Client.Do() returns an error, a read may still be in progress, and the API gives me no way to wait for it to complete.

Arrange for the wrapper (the io.ReadCloser used as the request body) to stop using the reusable reader. For example, you could have the ReadCloser return errors from any Read calls that follow a Close, and Close it before reusing the underlying, reusable reader.

[artem-anisimov-0x7f]

have the ReadCloser return errors from any Read calls that follow a Close, and Close it before reusing the underlying, reusable reader.

This is broken. As I showed in #71782 (comment), it is possible that a read from the request body be in progress after http.Client.Do() returns an error. So, to avoid races between the original and a retried requests, a wrapper must do two things:

1. have the ReadCloser return errors from any Read calls that follow a Close,
2. have Close wait for in-progress reads to complete.

Every user of http.Client who needs retries has to implement such reader wrapper to avoid races. It is unreasonable to force this upon users when http.Client itself can guarantee that it does not access Request.Body after an error return from http.Client.Do().

[neild]

The client could wait for any reads from the request body to complete before returning an error from RoundTrip. However:

1. On a successful return from RoundTrip, it would still be possible for a body.Close call to be followed by a body.Read, so request bodies would still need to handle this case.
2. This behavior has been present for over a decade, and we're certain to break people depending on it if we change it now.