net/http: fix unwanted HTTP/2 conn Transport crash after IdleConnTimeout

Go 1.7 crashed after Transport.IdleConnTimeout if an HTTP/2 connection
was established but but its caller no longer wanted it. (Assuming the
connection cache was enabled, which it is by default)

Fixes #16208

Change-Id: I9628757f7669e344f416927c77f00ed3864839e3
Reviewed-on: https://go-review.googlesource.com/27450
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

2 files changed, 59 insertions, 0 deletions

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 4604b90ec0..c66623db88 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -585,6 +585,7 @@ var (
 	errReadLoopExiting    = errors.New("http: persistConn.readLoop exiting")
 	errServerClosedIdle   = errors.New("http: server closed idle connection")
 	errIdleConnTimeout    = errors.New("http: idle connection timeout")
+	errNotCachingH2Conn   = errors.New("http: not caching alternate protocol's connections")
 )
 
 // transportReadFromServerError is used by Transport.readLoop when the
@@ -628,6 +629,9 @@ func (t *Transport) tryPutIdleConn(pconn *persistConn) error {
 	if pconn.isBroken() {
 		return errConnBroken
 	}
+	if pconn.alt != nil {
+		return errNotCachingH2Conn
+	}
 	pconn.markReused()
 	key := pconn.cacheKey
 
diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 749d4530b8..298682d04d 100644
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -3511,6 +3511,61 @@ func TestTransportIdleConnTimeout(t *testing.T) {
 	}
 }
 
+// Issue 16208: Go 1.7 crashed after Transport.IdleConnTimeout if an
+// HTTP/2 connection was established but but its caller no longer
+// wanted it. (Assuming the connection cache was enabled, which it is
+// by default)
+//
+// This test reproduced the crash by setting the IdleConnTimeout low
+// (to make the test reasonable) and then making a request which is
+// canceled by the DialTLS hook, which then also waits to return the
+// real connection until after the RoundTrip saw the error.  Then we
+// know the successful tls.Dial from DialTLS will need to go into the
+// idle pool. Then we give it a of time to explode.
+func TestIdleConnH2Crash(t *testing.T) {
+	cst := newClientServerTest(t, h2Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		// nothing
+	}))
+	defer cst.close()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	gotErr := make(chan bool, 1)
+
+	cst.tr.IdleConnTimeout = 5 * time.Millisecond
+	cst.tr.DialTLS = func(network, addr string) (net.Conn, error) {
+		cancel()
+		<-gotErr
+		c, err := tls.Dial(network, addr, &tls.Config{
+			InsecureSkipVerify: true,
+			NextProtos:         []string{"h2"},
+		})
+		if err != nil {
+			t.Error(err)
+			return nil, err
+		}
+		if cs := c.ConnectionState(); cs.NegotiatedProtocol != "h2" {
+			t.Errorf("protocol = %q; want %q", cs.NegotiatedProtocol, "h2")
+			c.Close()
+			return nil, errors.New("bogus")
+		}
+		return c, nil
+	}
+
+	req, _ := NewRequest("GET", cst.ts.URL, nil)
+	req = req.WithContext(ctx)
+	res, err := cst.c.Do(req)
+	if err == nil {
+		res.Body.Close()
+		t.Fatal("unexpected success")
+	}
+	gotErr <- true
+
+	// Wait for the explosion.
+	time.Sleep(cst.tr.IdleConnTimeout * 10)
+}
+
 type funcConn struct {
 	net.Conn
 	read  func([]byte) (int, error)