golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.96k stars 17.66k forks source link

x/net/http2: data race in Transport on req.Header #21316

Closed tombergan closed 6 years ago

tombergan commented 7 years ago

Repro here: https://go-review.googlesource.com/c/29243/10/http2/transport_test.go

The relevant code from transport.go:

func (rl *clientConnReadLoop) endStreamError(cs *clientStream, err error) {
    var code func()
    if err == nil {
        err = io.EOF
        code = cs.copyTrailers
    }
    cs.bufPipe.closeWithErrorAndCode(err, code)

    // The above line closes resp.Body. At that point it should be safe for
    // callers to reuse the req. However, req.Header is read below, which
    // can race with reuses of req.

    delete(rl.activeRes, cs.ID)
    if isConnectionCloseRequest(cs.req) { // req.Header read here
        rl.closeWhenIdle = true
    }

    select {
    case cs.resc <- resAndError{err: err}:
    default:
    }
}

func isConnectionCloseRequest(req *http.Request) bool {
    return req.Close || httplex.HeaderValuesContainsToken(req.Header["Connection"], "close")
}

The simple fix is to move cs.bufPipe.closeWithErrorAndCode after the isConnectionCloseRequest call, however, we should audit that cs.req is never used after cs.bufPipe is closed as there may be other instances of this race.

gopherbot commented 7 years ago

Change https://golang.org/cl/75530 mentions this issue: http2: fix transport data race on reused *http.Request objects

gopherbot commented 6 years ago

Change https://golang.org/cl/80078 mentions this issue: net/http: update bundled http2