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>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 4604b90ec0db58bb4e3b634eb90e9463df0852ca..c66623db881ebb482ef73e666d1d86ec16f3ce25 100644 (file)
--- 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 749d4530b84063c41169b8bb7d2d26fcdc9cc1de..298682d04de93a4f727063d5dc20ad509dd586fd 100644 (file)
--- 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)