diff options
author | Emmanuel T Odeke <emmanuel@orijtech.com> | 2019-10-13 15:07:06 -0700 |
---|---|---|
committer | Brad Fitzpatrick <bradfitz@golang.org> | 2019-10-14 18:28:36 +0000 |
commit | 2018d431c7e865d6db909cc0aa0d1f6d01e54583 (patch) | |
tree | f1011e33038e6cb23c1d53df52b00bd325c879cd | |
parent | 49073c579e02ad97a1c3e0b645c4cac1a3622fb6 (diff) | |
download | go-2018d431c7e865d6db909cc0aa0d1f6d01e54583.tar.gz go-2018d431c7e865d6db909cc0aa0d1f6d01e54583.zip |
[release-branch.go1.13] net/http: fix Transport panic with nil Request.Header
For Go 1.13 we introduced Header.Clone and it returns
nil if a nil Header is cloned. Unfortunately, though,
this exported Header.Clone nil behavior differed from
the old Go 1.12 and earlier internal header clone
behavior which always returned non-nil Headers.
This CL fixes the places where that distinction mattered.
Fixes #34882
Change-Id: Id19dea2272948c8dd10883b18ea7f7b8b33ea8eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/200977
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 9969c720800302c63147720da5507633133bd4a6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/201040
-rw-r--r-- | src/net/http/client.go | 4 | ||||
-rw-r--r-- | src/net/http/clone.go | 10 | ||||
-rw-r--r-- | src/net/http/header_test.go | 32 | ||||
-rw-r--r-- | src/net/http/request_test.go | 28 |
4 files changed, 72 insertions, 2 deletions
diff --git a/src/net/http/client.go b/src/net/http/client.go index 65a9d51cc6..20496d2f0e 100644 --- a/src/net/http/client.go +++ b/src/net/http/client.go @@ -238,7 +238,7 @@ func send(ireq *Request, rt RoundTripper, deadline time.Time) (resp *Response, d username := u.Username() password, _ := u.Password() forkReq() - req.Header = ireq.Header.Clone() + req.Header = cloneOrMakeHeader(ireq.Header) req.Header.Set("Authorization", "Basic "+basicAuth(username, password)) } @@ -668,7 +668,7 @@ func (c *Client) makeHeadersCopier(ireq *Request) func(*Request) { // The headers to copy are from the very initial request. // We use a closured callback to keep a reference to these original headers. var ( - ireqhdr = ireq.Header.Clone() + ireqhdr = cloneOrMakeHeader(ireq.Header) icookies map[string][]*Cookie ) if c.Jar != nil && ireq.Header.Get("Cookie") != "" { diff --git a/src/net/http/clone.go b/src/net/http/clone.go index 5f2784d280..3a3375bff7 100644 --- a/src/net/http/clone.go +++ b/src/net/http/clone.go @@ -62,3 +62,13 @@ func cloneMultipartFileHeader(fh *multipart.FileHeader) *multipart.FileHeader { fh2.Header = textproto.MIMEHeader(Header(fh.Header).Clone()) return fh2 } + +// cloneOrMakeHeader invokes Header.Clone but if the +// result is nil, it'll instead make and return a non-nil Header. +func cloneOrMakeHeader(hdr Header) Header { + clone := hdr.Clone() + if clone == nil { + clone = make(Header) + } + return clone +} diff --git a/src/net/http/header_test.go b/src/net/http/header_test.go index 51fcab103b..4789362919 100644 --- a/src/net/http/header_test.go +++ b/src/net/http/header_test.go @@ -7,6 +7,7 @@ package http import ( "bytes" "internal/race" + "reflect" "runtime" "testing" "time" @@ -219,3 +220,34 @@ func TestHeaderWriteSubsetAllocs(t *testing.T) { t.Errorf("allocs = %g; want 0", n) } } + +// Issue 34878: test that every call to +// cloneOrMakeHeader never returns a nil Header. +func TestCloneOrMakeHeader(t *testing.T) { + tests := []struct { + name string + in, want Header + }{ + {"nil", nil, Header{}}, + {"empty", Header{}, Header{}}, + { + name: "non-empty", + in: Header{"foo": {"bar"}}, + want: Header{"foo": {"bar"}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := cloneOrMakeHeader(tt.in) + if got == nil { + t.Fatal("unexpected nil Header") + } + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("Got: %#v\nWant: %#v", got, tt.want) + } + got.Add("A", "B") + got.Get("A") + }) + } +} diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go index b072f95802..bb06d922f0 100644 --- a/src/net/http/request_test.go +++ b/src/net/http/request_test.go @@ -826,6 +826,34 @@ func TestWithContextDeepCopiesURL(t *testing.T) { } } +func TestNoPanicOnRoundTripWithBasicAuth_h1(t *testing.T) { + testNoPanicWithBasicAuth(t, h1Mode) +} + +func TestNoPanicOnRoundTripWithBasicAuth_h2(t *testing.T) { + testNoPanicWithBasicAuth(t, h2Mode) +} + +// Issue 34878: verify we don't panic when including basic auth (Go 1.13 regression) +func testNoPanicWithBasicAuth(t *testing.T, h2 bool) { + defer afterTest(t) + cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {})) + defer cst.close() + + u, err := url.Parse(cst.ts.URL) + if err != nil { + t.Fatal(err) + } + u.User = url.UserPassword("foo", "bar") + req := &Request{ + URL: u, + Method: "GET", + } + if _, err := cst.c.Do(req); err != nil { + t.Fatalf("Unexpected error: %v", err) + } +} + // verify that NewRequest sets Request.GetBody and that it works func TestNewRequestGetBody(t *testing.T) { tests := []struct { |