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

net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently #51197

Closed
moshegood opened this issue Feb 14, 2022 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@moshegood
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.17.6 darwin/amd64

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/moshe/Library/Caches/go-build"
GOENV="/Users/moshe/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/moshe/code/go/pkg/mod"
GONOPROXY="git.sqcorp.co,git.corp.squareup.com,github.com/squareup"
GONOSUMDB="git.sqcorp.co,git.corp.squareup.com,github.com/squareup"
GOOS="darwin"
GOPATH="/Users/moshe/code/go"
GOPRIVATE="git.sqcorp.co,git.corp.squareup.com,github.com/squareup"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/usr/local/Cellar/go/1.17.6/libexec/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/k0/pqfxzl896xg7g6vq_wby46380000gn/T/go-build4218274825=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Used net/http2 with net/httputil/ReverseProxy.
Then ran a test with the -race flag.

What did you expect to see?

No race conditions.

What did you see instead?

Race conditions.

Not quite sure how hard this will be to replicate cleanly to reproduce.
Basically, the http2 portion of net/http will close a request body when it’s done.
Also, the ProxyServer portion of net/http/httputil will close a request body when it’s done.
So the request body gets closed twice.
But the http2requestBody.Close() method, while obviously coded to support being called multiple times, will run into a race condition if Close() is called twice concurrently.

Very simple fix as well.
Current code:

// requestBody is the Handler's Request.Body type.
// Read and Close may be called concurrently.
type http2requestBody struct {
	_             http2incomparable
	stream        *http2stream
	conn          *http2serverConn
	closed        bool       // for use by Close only
	sawEOF        bool       // for use by Read only
	pipe          *http2pipe // non-nil if we have a HTTP entity message body
	needsContinue bool       // need to send a 100-continue
}

func (b *http2requestBody) Close() error {
	if b.pipe != nil && !b.closed {
		b.pipe.BreakWithError(http2errClosedBody)
	}
	b.closed = true
	return nil
}

Fixed code:

// requestBody is the Handler's Request.Body type.
// Read and Close may be called concurrently.
type http2requestBody struct {
	_             http2incomparable
	stream        *http2stream
	conn          *http2serverConn
	closeOnce     sync.Once       // for use by Close only
	sawEOF        bool       // for use by Read only
	pipe          *http2pipe // non-nil if we have a HTTP entity message body
	needsContinue bool       // need to send a 100-continue
}

func (b *http2requestBody) Close() error {
	b.closeOnce.Do(func(){
		if b.pipe != nil {
			b.pipe.BreakWithError(http2errClosedBody)
		}
	})
	return nil
}
@dmitshur dmitshur changed the title affected/package: net/http - support http2requestBody.Close() being called multiple times concurrently net/http: support http2requestBody.Close() being called multiple times concurrently Feb 14, 2022
@dmitshur dmitshur changed the title net/http: support http2requestBody.Close() being called multiple times concurrently net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently Feb 14, 2022
@moshegood
Copy link
Author

Example code to reproduce:

package main

import (
	"log"
	"net/http"
	"net/http/httputil"
	"net/url"
)

func main() {
	endpoint := "https://httpstat.us/"
	url, err := url.Parse(endpoint)
	if err != nil {
		panic(err)
	}

	// Create a server on port 8000
	// Exactly how you would run an HTTP/1.1 server
	srv := &http.Server{Addr: ":8000", Handler: httputil.NewSingleHostReverseProxy(url)}

	// Start the server with TLS, since we are running HTTP/2 it must be
	// run with TLS.
	// Exactly how you would run an HTTP/1.1 server with TLS connection.
	log.Printf("Serving stuff on https://0.0.0.0:8000")
	log.Fatal(srv.ListenAndServeTLS("server.crt", "server.key"))
}

Then I can run it as:

$ GORACE="log_path=stderr" go run -race server.go

And trigger the race with:

for i in $(seq 20); do curl -k -X POST -d@random.stl --max-time 0.$i  --http2 https://localhost:8000/; done

@gopherbot
Copy link

Change https://go.dev/cl/385874 mentions this issue: net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently

@dmitshur
Copy link
Contributor

CC @neild.

@dmitshur dmitshur added this to the Backlog milestone Feb 15, 2022
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 15, 2022
@neild
Copy link
Contributor

neild commented Feb 17, 2022

The race is a side effect of the fix for #46866, which causes ReverseProxy to always close the body of outgoing requests before returning. When the ReverseProxy handler returns early, this can result in racing closes: One by ReverseProxy.ServeHTTP and one by RoundTrip.

While the http package doesn't document that it's okay to Close a request body multiple times or concurrently, making this safe seems reasonable and simpler than finding an alternate workaround for #46866.

@gopherbot
Copy link

Change https://go.dev/cl/386495 mentions this issue: net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently

@moshegood
Copy link
Author

How do we get this change submitted to upstream?
PR has been reviewed, but needs a googler.

@scathers
Copy link

Now that the change has been approved, any idea on when it will be merged in?

dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
While no guarantees are made about the safety of repeated or
concurrent closes of a request body, HTTP/1 request bodies are
concurrency-safe and HTTP/2 ones should be as well.

Fixes golang/go#51197

Change-Id: Id6527dc2932579cabc9cbe921c6e0b3b4a3d472c
Reviewed-on: https://go-review.googlesource.com/c/net/+/386495
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
While no guarantees are made about the safety of repeated or
concurrent closes of a request body, HTTP/1 request bodies are
concurrency-safe and HTTP/2 ones should be as well.

Fixes golang/go#51197

Change-Id: Id6527dc2932579cabc9cbe921c6e0b3b4a3d472c
Reviewed-on: https://go-review.googlesource.com/c/net/+/386495
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants