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: Transport should close body on all errors and match Client.Do docs #35015

Closed
odeke-em opened this issue Oct 20, 2019 · 4 comments
Closed
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@odeke-em
Copy link
Member

I just saw CL https://go-review.googlesource.com/c/go/+/202237 by @bored-engineer (thank you Luke!) and was going to review and recommend/create a test for the CL creator.

The docs for http.Client.Do https://golang.org/pkg/net/http/#Client.Do promise that we'll always close the body even on errors and so far we missed a few spots on the trivial cases with minor oversight

if isHTTP {
for k, vv := range req.Header {
if !httpguts.ValidHeaderFieldName(k) {
return nil, fmt.Errorf("net/http: invalid header field name %q", k)
}
for _, v := range vv {
if !httpguts.ValidHeaderFieldValue(v) {
return nil, fmt.Errorf("net/http: invalid header field value %q for key %v", v, k)
}
}
}
}
if t.useRegisteredProtocol(req) {
altProto, _ := t.altProto.Load().(map[string]RoundTripper)
if altRT := altProto[scheme]; altRT != nil {
if resp, err := altRT.RoundTrip(req); err != ErrSkipAltProtocol {
return resp, err
}
}
}
if !isHTTP {
req.closeBody()
return nil, &badStringError{"unsupported protocol scheme", scheme}
}
if req.Method != "" && !validMethod(req.Method) {
return nil, fmt.Errorf("net/http: invalid method %q", req.Method)

that is:
a) if isHTTP {
b) if t.useRegisteredProtocol(req)
c) if req.Method != "" && !validMethod(req.Method) {

and I realize this is because that code has been modified piecemeal over the past decade.

This test is what we can use to bootstrap checks

package main

import (
	"io"
	"net/http"
	"net/http/httptest"
	"net/url"
	"testing"
)

type bodyCloser bool

func (bc *bodyCloser) Close() error {
	*bc = true
        return nil
}

func (bc *bodyCloser) Read(b []byte) (n int, err error) {
	return 0, io.EOF
}

func TestInvalidMethodClosesBody(t *testing.T) {
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
	defer cst.Close()
	var bc bodyCloser

	u, _ := url.Parse(cst.URL)
	req := &http.Request{
		Method: " ",
		URL:    u,
		Body:   &bc,
	}

	_, err := http.DefaultClient.Do(req)
	if err == nil {
		t.Fatal("Expected an error")
	}
	if !bc {
		t.Fatal("Expected body to have been closed")
	}
}

which currently gives

$ go test
--- FAIL: TestInvalidMethodClosesBody (0.00s)
    it_test.go:39: Expected body to have been closed

This issue will be to send CLs to enforce those closes as well as tests so that we don't regress.

@gopherbot
Copy link

Change https://golang.org/cl/202237 mentions this issue: net/http: ensure request body is closed when method is invalid

gopherbot pushed a commit that referenced this issue Oct 20, 2019
Updates #35015

Change-Id: Ibfe8f72ed3887ca88ce9c1d8a29dacda72f3fe17
GitHub-Last-Rev: 4bfc56e
GitHub-Pull-Request: #35014
Reviewed-on: https://go-review.googlesource.com/c/go/+/202237
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/202239 mentions this issue: net/http: make Transport.RoundTrip close body on any invalid request

@bored-engineer
Copy link
Contributor

@odeke-em what do you think about the remaining useRegisteredProtocol case where an error is returned by the underlying protocol handler. In my opinion Transport shouldn't handle that, it should be the responsibility of the protocol handler as the handler could have already closed the request body before an error occurred. In that case I think updating the docs so that expectation is clear would be a good addition.

@odeke-em
Copy link
Member Author

odeke-em commented Oct 21, 2019 via email

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 21, 2019
@golang golang locked and limited conversation to collaborators Oct 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted 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

4 participants