From: Steven Hartland <steven.hartland@multiplay.co.uk>
Date: Thu, 7 May 2020 10:08:08 +0000 (+0000)
Subject: net/http/httputil: fix deadlock in DumpRequestOut
X-Git-Tag: go1.16rc1~122
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=d2131704a6fda781bd3b823dbe8f57741663f466;p=gostls13.git

net/http/httputil: fix deadlock in DumpRequestOut

Fix a deadlock in DumpRequestOut which can occur if the request is
cancelled between response being sent and it being processed.

Also:
* Ensure we don't get a reader leak when an error is reported by the
  transport before the body is consumed.
* Add leaked goroutine retries to avoid false test failures.

Fixes #38352

Change-Id: I83710791b2985b997f61fe5b49eadee0bb51bdee
Reviewed-on: https://go-review.googlesource.com/c/go/+/232798
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Damien Neil <dneil@google.com>
---

diff --git a/src/net/http/httputil/dump.go b/src/net/http/httputil/dump.go
index 4c9d28bed8..2948f27e5d 100644
--- a/src/net/http/httputil/dump.go
+++ b/src/net/http/httputil/dump.go
@@ -138,6 +138,8 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
 		select {
 		case dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n"):
 		case <-quitReadCh:
+			// Ensure delegateReader.Read doesn't block forever if we get an error.
+			close(dr.c)
 		}
 	}()
 
@@ -146,7 +148,8 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
 	req.Body = save
 	if err != nil {
 		pw.Close()
-		quitReadCh <- struct{}{}
+		dr.err = err
+		close(quitReadCh)
 		return nil, err
 	}
 	dump := buf.Bytes()
@@ -167,13 +170,17 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
 // delegateReader is a reader that delegates to another reader,
 // once it arrives on a channel.
 type delegateReader struct {
-	c chan io.Reader
-	r io.Reader // nil until received from c
+	c   chan io.Reader
+	err error     // only used if r is nil and c is closed.
+	r   io.Reader // nil until received from c
 }
 
 func (r *delegateReader) Read(p []byte) (int, error) {
 	if r.r == nil {
-		r.r = <-r.c
+		var ok bool
+		if r.r, ok = <-r.c; !ok {
+			return 0, r.err
+		}
 	}
 	return r.r.Read(p)
 }
diff --git a/src/net/http/httputil/dump_test.go b/src/net/http/httputil/dump_test.go
index 7571eb0820..8168b2ebc0 100644
--- a/src/net/http/httputil/dump_test.go
+++ b/src/net/http/httputil/dump_test.go
@@ -7,13 +7,17 @@ package httputil
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"fmt"
 	"io"
+	"math/rand"
 	"net/http"
 	"net/url"
 	"runtime"
+	"runtime/pprof"
 	"strings"
 	"testing"
+	"time"
 )
 
 type eofReader struct{}
@@ -311,11 +315,39 @@ func TestDumpRequest(t *testing.T) {
 			}
 		}
 	}
-	if dg := runtime.NumGoroutine() - numg0; dg > 4 {
-		buf := make([]byte, 4096)
-		buf = buf[:runtime.Stack(buf, true)]
-		t.Errorf("Unexpectedly large number of new goroutines: %d new: %s", dg, buf)
+
+	// Validate we haven't leaked any goroutines.
+	var dg int
+	dl := deadline(t, 5*time.Second, time.Second)
+	for time.Now().Before(dl) {
+		if dg = runtime.NumGoroutine() - numg0; dg <= 4 {
+			// No unexpected goroutines.
+			return
+		}
+
+		// Allow goroutines to schedule and die off.
+		runtime.Gosched()
+	}
+
+	buf := make([]byte, 4096)
+	buf = buf[:runtime.Stack(buf, true)]
+	t.Errorf("Unexpectedly large number of new goroutines: %d new: %s", dg, buf)
+}
+
+// deadline returns the time which is needed before t.Deadline()
+// if one is configured and it is s greater than needed in the future,
+// otherwise defaultDelay from the current time.
+func deadline(t *testing.T, defaultDelay, needed time.Duration) time.Time {
+	if dl, ok := t.Deadline(); ok {
+		if dl = dl.Add(-needed); dl.After(time.Now()) {
+			// Allow an arbitrarily long delay.
+			return dl
+		}
 	}
+
+	// No deadline configured or its closer than needed from now
+	// so just use the default.
+	return time.Now().Add(defaultDelay)
 }
 
 func chunk(s string) string {
@@ -445,3 +477,43 @@ func TestDumpResponse(t *testing.T) {
 		}
 	}
 }
+
+// Issue 38352: Check for deadlock on cancelled requests.
+func TestDumpRequestOutIssue38352(t *testing.T) {
+	if testing.Short() {
+		return
+	}
+	t.Parallel()
+
+	timeout := 10 * time.Second
+	if deadline, ok := t.Deadline(); ok {
+		timeout = time.Until(deadline)
+		timeout -= time.Second * 2 // Leave 2 seconds to report failures.
+	}
+	for i := 0; i < 1000; i++ {
+		delay := time.Duration(rand.Intn(5)) * time.Millisecond
+		ctx, cancel := context.WithTimeout(context.Background(), delay)
+		defer cancel()
+
+		r := bytes.NewBuffer(make([]byte, 10000))
+		req, err := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", r)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		out := make(chan error)
+		go func() {
+			_, err = DumpRequestOut(req, true)
+			out <- err
+		}()
+
+		select {
+		case <-out:
+		case <-time.After(timeout):
+			b := &bytes.Buffer{}
+			fmt.Fprintf(b, "deadlock detected on iteration %d after %s with delay: %v\n", i, timeout, delay)
+			pprof.Lookup("goroutine").WriteTo(b, 1)
+			t.Fatal(b.String())
+		}
+	}
+}