Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: race condition when writing trailers #48340

Closed
neild opened this issue Sep 12, 2021 · 2 comments
Closed

x/net/http2: race condition when writing trailers #48340

neild opened this issue Sep 12, 2021 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Sep 12, 2021

(*ClientConn).encodeTrailers encodes into a per-conn buffer (cc.hbuf). Trailers are encoded with cc.mu held, but written with only cc.wmu held. If another request on the same ClientConn uses the buffer while the trailers are being written, a race occurs.

https://go.googlesource.com/net/+/refs/heads/master/http2/transport.go#1417

Reproduction below.

func TestTransportFrameBufferReuse(t *testing.T) {
        st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {}, optOnlyServer)
        defer st.Close()

        tr := &Transport{TLSClientConfig: tlsConfigInsecure}
        defer tr.CloseIdleConnections()

        filler := hex.EncodeToString([]byte(randString(2048)))
        var wg sync.WaitGroup
        defer wg.Wait()
        for i := 0; i < 10; i++ {
                wg.Add(1)
                go func() {
                        defer wg.Done()
                        req, err := http.NewRequest("POST", st.ts.URL, strings.NewReader(filler))
                        if err != nil {
                                t.Fatal(err)
                        }
                        req.Header.Set("Big", filler)
                        req.Trailer = make(http.Header)
                        req.Trailer.Set("Big", filler)
                        res, err := tr.RoundTrip(req)
                        if err != nil {
                                t.Fatal(err)
                        }
                        if got, want := res.StatusCode, 200; got != want {
                                t.Errorf("StatusCode = %v; want %v", got, want)
                        }
                        if res != nil && res.Body != nil {
                                res.Body.Close()
                        }
                }()
        }
}
@gopherbot
Copy link

Change https://golang.org/cl/349490 mentions this issue: http2: fix off-by-one error in client check for max concurrent streams

@cagedmantis cagedmantis added this to the Unreleased milestone Sep 13, 2021
@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 13, 2021
@gopherbot
Copy link

Change https://golang.org/cl/349594 mentions this issue: http2: avoid blocking while holding ClientConn.mu

dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
Operations which examine the state of a ClientConn--notably,
the connection pool's check to see if a conn is available to
take a new request--need to acquire mu. Blocking while holding mu,
such as when writing to the network, blocks these operations.

Remove blocking operations from the mutex.
Perform network writes with only ClientConn.wmu held.
Clarify that wmu guards the per-conn HPACK encoder and buffer.

Add a new mutex guarding request creation, covering the critical
section starting with allocating a new stream ID and continuing
until the stream is created.

Fix a locking issue where trailers were written from the HPACK
buffer with only wmu held, but headers were encoded into the buffer
with only mu held. (Now both encoding and writes occur with wmu
held.)

Fixes golang/go#32388.
Fixes golang/go#48340.

Change-Id: Ibb313424ed2f32c1aeac4645b76aedf227b597a3
Reviewed-on: https://go-review.googlesource.com/c/net/+/349594
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>
@golang golang locked and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants