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: request.Body.Close can be called more than once #40382

Closed
benoit-a opened this issue Jul 24, 2020 · 6 comments
Closed

net/http: request.Body.Close can be called more than once #40382

benoit-a opened this issue Jul 24, 2020 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@benoit-a
Copy link

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

❯ go version
go version go1.14.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
❯ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/me/Library/Caches/go-build"
GOENV="/Users/me/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/me/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.5/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/yp/8jdpdcb13mn5k6m6jdcqlbjr0000gn/T/go-build087461839=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Context: doing a large PUT on a remote server ( > 200 kB) with no specified Content-Length (using T-E: chunked)

I am passing a io.ReadCloser to request.Body and I want to avoid a double call on close (these are supposed to be undefined behaviour).
But sometimes, when the remote server is not consuming the entire payload and returns an error, Close() is called twice.

The following code reproduce the issue on my laptop. This may require multiple runs.

package main

import (
	"fmt"
	"io"
	"net/http"
	"net/http/httptest"
	"runtime/debug"
	"strings"
)

type smallReader struct {
	r      io.Reader
	closed bool
	prevBt []byte
}

func (r *smallReader) Read(p []byte) (int, error) {
	return r.r.Read(p)
}

func (r *smallReader) Close() error {
	if r.closed == true {
		fmt.Println((string)(r.prevBt))
		panic("already closed")
	}
	r.prevBt = debug.Stack()
	r.closed = true
	return nil
}

func newSmallReader(size int) io.ReadCloser {
	r := &smallReader{
		r:      strings.NewReader(strings.Repeat("X", size)),
		closed: false,
	}
	return r
}

func TestCrash906() {
	h := func(w http.ResponseWriter, r *http.Request) {
		if r.Body != nil {
			buf := make([]byte, 1)
			r.Body.Read(buf)
			defer r.Body.Close()
		}
		w.Header().Add("Connection", "close")
		w.WriteHeader(411)
	}

	mux := http.NewServeMux()
	mux.HandleFunc("/", h)

	srv := httptest.NewServer(mux)
	defer srv.Close()

	for i := 300000; i < 600000; i += 10000 {
		req, err := http.NewRequest(http.MethodPut, srv.URL, nil)

		req.Body = newSmallReader(i)

		if err != nil || req == nil {
			panic("not expected")
		}

		resp, _ := http.DefaultClient.Do(req)
		if resp != nil {
			resp.Body.Close()
		}
	}
}

func main() {
	TestCrash906()
}

What did you expect to see?

Nothing, Close() should be called only once.

What did you see instead?

❯ go run -race main.go
goroutine 51 [running]:
runtime/debug.Stack(0xc0000a01f0, 0x0, 0x3)
	/usr/local/Cellar/go/1.14.5/libexec/src/runtime/debug/stack.go:24 +0xab
main.(*smallReader).Close(0xc0000a01e0, 0x0, 0x0)
	/Users/benoitartuso/Sources/T906/main.go:27 +0x65
net/http.(*transferWriter).writeBody(0xc0002b6000, 0x1528200, 0xc0002a0000, 0x2, 0x2)
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/transfer.go:372 +0x5aa
net/http.(*Request).write(0xc0000ae100, 0x1528200, 0xc0002a0000, 0x0, 0xc00029a060, 0x0, 0x0, 0x0)
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/request.go:685 +0x85a
net/http.(*persistConn).writeLoop(0xc0000ca000)
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/transport.go:2280 +0x332
created by net/http.(*Transport).dialConn
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/transport.go:1648 +0xc34

panic: already closed

goroutine 51 [running]:
main.(*smallReader).Close(0xc0000a01e0, 0x1528400, 0xc0002c4000)
	/Users/benoitartuso/Sources/T906/main.go:25 +0x1e4
net/http.(*Request).closeBody(...)
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/request.go:1391
net/http.(*persistConn).writeLoop(0xc0000ca000)
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/transport.go:2296 +0x5a4
created by net/http.(*Transport).dialConn
	/usr/local/Cellar/go/1.14.5/libexec/src/net/http/transport.go:1648 +0xc34
exit status 2

With a first glance at the code, I can see a reason for that.

In T-E: chunked, the net/http.(*transferWriter).writeBody function will close the body and then, will write the EOF chunk. This write and all the following "Flush" calls on the writer may return an error.
If there is an error, writeLoop() interpret it as a signal that the body is not closed. So any error happening after the actual close of the body in writeBody() will lead to a double close.

@seankhliao
Copy link
Member

io.Closer says

The behavior of Close after the first call is undefined. Specific implementations may document their own behavior.

So I agree that net/http should not call Close twice

@cagedmantis cagedmantis added this to the Backlog milestone Jul 28, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 28, 2020
@cagedmantis
Copy link
Contributor

/cc @bradfitz @bcmills

@rsc
Copy link
Contributor

rsc commented Aug 4, 2020

Since this bug dates back to Go 1.14 I think we can leave it for the next cycle and not hold up Go 1.15.

@rsc
Copy link
Contributor

rsc commented Aug 4, 2020

/cc @neild

@gopherbot
Copy link

Change https://golang.org/cl/257819 mentions this issue: net/http: ensure Request.Body.Close is called once and only once

@networkimprov
Copy link

cc @fraenkel @odeke-em as possible reviewers.

@golang golang locked and limited conversation to collaborators Oct 16, 2021
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

6 participants