From dc3612e0d1a569e2ad1a3deea9ef6eab72616dc4 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 19 Aug 2016 23:13:29 +0000 Subject: [release-branch.go1.7] 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 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-on: https://go-review.googlesource.com/28637 Reviewed-by: Brad Fitzpatrick --- src/net/http/transport.go | 4 +++ src/net/http/transport_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 66721190de..1f0763471b 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -590,6 +590,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 @@ -633,6 +634,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) -- cgit v1.2.3-54-g00ecf