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 #34878

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>

diff --git a/src/net/http/client.go b/src/net/http/client.go
index 38612f22ef76643bb2739d2023a34e8337f5fc67..9566b8940e67defb56bbb4a060141e53c25c54a5 100644 (file)
--- a/src/net/http/client.go
+++ b/src/net/http/client.go
@@ -240,7 +240,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))
        }
 
@@ -719,7 +719,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 5f2784d28089d2758cbf06aa0484e8a988ad625d..3a3375bff7164163d39753f4bbf1edb9e87f87cd 100644 (file)
--- 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 51fcab103b42472fd4280d438d63d93b507c1d6e..47893629194b6a6dedde343b06c73ac0db976f34 100644 (file)
--- 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 b072f958024b903f79f052a8fc471f092d415cc8..bb06d922f0fb9e6cfe75c3a6f3319d2e65ae6d2f 100644 (file)
--- 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 {