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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: odeke-em

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") != "" {

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
+}

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")
+		})
+	}
+}

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 {