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: data race in Transport on req.Header #21316

Closed
tombergan opened this issue Aug 4, 2017 · 2 comments
Closed

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

tombergan opened this issue Aug 4, 2017 · 2 comments
Assignees
Milestone

Comments

@tombergan
Copy link
Contributor

@tombergan tombergan commented Aug 4, 2017

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.

@tombergan tombergan added this to the Go1.10 milestone Aug 4, 2017
@tombergan tombergan self-assigned this Aug 4, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 2, 2017

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

gopherbot pushed a commit to golang/net that referenced this issue Nov 2, 2017
Based on golang/go#19653, it should be possible to reuse an http.Request
object after the outstanding request has completed. This CL fixes a race
in the http/2 library that occurs when a caller tries to reuse an
http.Request just after the request completed.

The new test failed with -race before this CL and passes after this CL.
Verified with -count 10000.

Updates golang/go#21316

Change-Id: I014cf9cefd0dd21f6f41763ba554d23ddc7fca40
Reviewed-on: https://go-review.googlesource.com/75530
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 28, 2017

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

@gopherbot gopherbot closed this in 5b649ff Nov 28, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Based on golang/go#19653, it should be possible to reuse an http.Request
object after the outstanding request has completed. This CL fixes a race
in the http/2 library that occurs when a caller tries to reuse an
http.Request just after the request completed.

The new test failed with -race before this CL and passes after this CL.
Verified with -count 10000.

Updates golang/go#21316

Change-Id: I014cf9cefd0dd21f6f41763ba554d23ddc7fca40
Reviewed-on: https://go-review.googlesource.com/75530
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
jasonwbarnett pushed a commit to jasonwbarnett/fileserver that referenced this issue Jul 11, 2018
Update http2 to x/net git rev db473f6b23.

(And un-skip TestWriteHeader0_h2 added in CL 80077, now fixed.)

Includes:

   http2: remove afterReqBodyWriteError wrapper
   https://golang.org/cl/75252

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

   http2: require either ECDSA or RSA ciphersuite
   https://golang.org/cl/30721

   http2: don't log about timeouts reading client preface on new connections
   https://golang.org/cl/79498

   http2: don't crash in Transport on server's DATA following bogus HEADERS
   https://golang.org/cl/80056

   http2: panic on invalid WriteHeader status code
   https://golang.org/cl/80076

   http2: fix race on ClientConn.maxFrameSize
   https://golang.org/cl/79238

   http2: don't autodetect Content-Type when the response has an empty body
   https://golang.org/cl/80135

Fixes golang/go#18776
Updates golang/go#20784
Fixes golang/go#21316
Fixes golang/go#22721
Fixes golang/go#22880

Change-Id: Ie86e24e0ee2582a5a82afe5de3c7c801528be069
Reviewed-on: https://go-review.googlesource.com/80078
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@golang golang locked and limited conversation to collaborators Nov 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.