net/http: make Request.Clone create fresh copies for matches and otherValues

This change fixes Request.Clone to correctly work with SetPathValue
by creating fresh copies for matches and otherValues so that
SetPathValue for cloned requests doesn't pollute the original request.

While here, also added a doc for Request.SetPathValue.

Fixes #64911

Change-Id: I2831b38e135935dfaea2b939bb9db554c75b65ef
GitHub-Last-Rev: 1981db16475a49fe8d4b874a6bceec64d28a1332
GitHub-Pull-Request: golang/go#64913
Reviewed-on: https://go-review.googlesource.com/c/go/+/553375
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Jes Cok <xigua67damn@gmail.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

2 files changed, 43 insertions, 0 deletions

diff --git a/src/net/http/request.go b/src/net/http/request.go
index ed2cdac136..fce2d16f95 100644
--- a/src/net/http/request.go
+++ b/src/net/http/request.go
@@ -397,6 +397,20 @@ func (r *Request) Clone(ctx context.Context) *Request {
 	r2.Form = cloneURLValues(r.Form)
 	r2.PostForm = cloneURLValues(r.PostForm)
 	r2.MultipartForm = cloneMultipartForm(r.MultipartForm)
+
+	// Copy matches and otherValues. See issue 61410.
+	if s := r.matches; s != nil {
+		s2 := make([]string, len(s))
+		copy(s2, s)
+		r2.matches = s2
+	}
+	if s := r.otherValues; s != nil {
+		s2 := make(map[string]string, len(s))
+		for k, v := range s {
+			s2[k] = v
+		}
+		r2.otherValues = s2
+	}
 	return r2
 }
 
@@ -1427,6 +1441,8 @@ func (r *Request) PathValue(name string) string {
 	return r.otherValues[name]
 }
 
+// SetPathValue sets name to value, so that subsequent calls to r.PathValue(name)
+// return value.
 func (r *Request) SetPathValue(name, value string) {
 	if i := r.patIndex(name); i >= 0 {
 		r.matches[i] = value
diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go
index 1531da3d8c..6ce32332e7 100644
--- a/src/net/http/request_test.go
+++ b/src/net/http/request_test.go
@@ -1053,6 +1053,33 @@ func TestRequestCloneTransferEncoding(t *testing.T) {
 	}
 }
 
+// Ensure that Request.Clone works correctly with PathValue.
+// See issue 64911.
+func TestRequestClonePathValue(t *testing.T) {
+	req, _ := http.NewRequest("GET", "https://example.org/", nil)
+	req.SetPathValue("p1", "orig")
+
+	clonedReq := req.Clone(context.Background())
+	clonedReq.SetPathValue("p2", "copy")
+
+	// Ensure that any modifications to the cloned
+	// request do not pollute the original request.
+	if g, w := req.PathValue("p2"), ""; g != w {
+		t.Fatalf("p2 mismatch got %q, want %q", g, w)
+	}
+	if g, w := req.PathValue("p1"), "orig"; g != w {
+		t.Fatalf("p1 mismatch got %q, want %q", g, w)
+	}
+
+	// Assert on the changes to the cloned request.
+	if g, w := clonedReq.PathValue("p1"), "orig"; g != w {
+		t.Fatalf("p1 mismatch got %q, want %q", g, w)
+	}
+	if g, w := clonedReq.PathValue("p2"), "copy"; g != w {
+		t.Fatalf("p2 mismatch got %q, want %q", g, w)
+	}
+}
+
 // Issue 34878: verify we don't panic when including basic auth (Go 1.13 regression)
 func TestNoPanicOnRoundTripWithBasicAuth(t *testing.T) { run(t, testNoPanicWithBasicAuth) }
 func testNoPanicWithBasicAuth(t *testing.T, mode testMode) {