From 70ee525261f7d8adbcf58c2cf54efb7e7efb1c82 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 13 Jan 2016 16:30:00 +0000 Subject: [PATCH] net/http: fix Transport crash when abandoning dial which upgrades protos When the Transport was creating an bound HTTP connection (protocol unknown initially) and then ends up deciding it doesn't need it, a goroutine sits around to clean up whatever the result was. That goroutine made the false assumption that the result was always an HTTP/1 connection or an error. It may also be an alternate protocol in which case the *persistConn.conn net.Conn field is nil, and the alt field is non-nil. Fixes #13839 Change-Id: Ia4972e5eb1ad53fa00410b3466d4129c753e0871 Reviewed-on: https://go-review.googlesource.com/18573 Reviewed-by: Russ Cox Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- src/net/http/export_test.go | 1 + src/net/http/transport.go | 30 ++++++++--- src/net/http/transport_test.go | 93 ++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 6 deletions(-) diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go index 514d02b2a3..52bccbdce3 100644 --- a/src/net/http/export_test.go +++ b/src/net/http/export_test.go @@ -21,6 +21,7 @@ var ( ExportServerNewConn = (*Server).newConn ExportCloseWriteAndWait = (*conn).closeWriteAndWait ExportErrRequestCanceled = errRequestCanceled + ExportErrRequestCanceledConn = errRequestCanceledConn ExportServeFile = serveFile ExportHttp2ConfigureTransport = http2ConfigureTransport ExportHttp2ConfigureServer = http2ConfigureServer diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 6c08391766..9378b8385e 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -618,9 +618,13 @@ func (t *Transport) replaceReqCanceler(r *Request, fn func()) bool { return true } -func (t *Transport) dial(network, addr string) (c net.Conn, err error) { +func (t *Transport) dial(network, addr string) (net.Conn, error) { if t.Dial != nil { - return t.Dial(network, addr) + c, err := t.Dial(network, addr) + if c == nil && err == nil { + err = errors.New("net/http: Transport.Dial hook returned (nil, nil)") + } + return c, err } return net.Dial(network, addr) } @@ -682,10 +686,10 @@ func (t *Transport) getConn(req *Request, cm connectMethod) (*persistConn, error return pc, nil case <-req.Cancel: handlePendingDial() - return nil, errors.New("net/http: request canceled while waiting for connection") + return nil, errRequestCanceledConn case <-cancelc: handlePendingDial() - return nil, errors.New("net/http: request canceled while waiting for connection") + return nil, errRequestCanceledConn } } @@ -705,6 +709,9 @@ func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) { if err != nil { return nil, err } + if pconn.conn == nil { + return nil, errors.New("net/http: Transport.DialTLS returned (nil, nil)") + } if tc, ok := pconn.conn.(*tls.Conn); ok { cs := tc.ConnectionState() pconn.tlsState = &cs @@ -1326,6 +1333,7 @@ func (e *httpError) Temporary() bool { return true } var errTimeout error = &httpError{err: "net/http: timeout awaiting response headers", timeout: true} var errClosed error = &httpError{err: "net/http: server closed connection before response was received"} var errRequestCanceled = errors.New("net/http: request canceled") +var errRequestCanceledConn = errors.New("net/http: request canceled while waiting for connection") // TODO: unify? func nop() {} @@ -1502,9 +1510,19 @@ func (pc *persistConn) closeLocked(err error) { } pc.broken = true if pc.closed == nil { - pc.conn.Close() pc.closed = err - close(pc.closech) + if pc.alt != nil { + // Do nothing; can only get here via getConn's + // handlePendingDial's putOrCloseIdleConn when + // it turns out the abandoned connection in + // flight ended up negotiating an alternate + // protocol. We don't use the connection + // freelist for http2. That's done by the + // alternate protocol's RoundTripper. + } else { + pc.conn.Close() + close(pc.closech) + } } pc.mutateHeaderFunc = nil } diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index 46e330315b..3b2a5f978e 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -24,6 +24,7 @@ import ( . "net/http" "net/http/httptest" "net/http/httputil" + "net/http/internal" "net/url" "os" "reflect" @@ -2939,6 +2940,98 @@ func TestTransportReuseConnEmptyResponseBody(t *testing.T) { } } +// Issue 13839 +func TestNoCrashReturningTransportAltConn(t *testing.T) { + cert, err := tls.X509KeyPair(internal.LocalhostCert, internal.LocalhostKey) + if err != nil { + t.Fatal(err) + } + ln := newLocalListener(t) + defer ln.Close() + + handledPendingDial := make(chan bool, 1) + SetPendingDialHooks(nil, func() { handledPendingDial <- true }) + defer SetPendingDialHooks(nil, nil) + + testDone := make(chan struct{}) + defer close(testDone) + go func() { + tln := tls.NewListener(ln, &tls.Config{ + NextProtos: []string{"foo"}, + Certificates: []tls.Certificate{cert}, + }) + sc, err := tln.Accept() + if err != nil { + t.Error(err) + return + } + if err := sc.(*tls.Conn).Handshake(); err != nil { + t.Error(err) + return + } + <-testDone + sc.Close() + }() + + addr := ln.Addr().String() + + req, _ := NewRequest("GET", "https://fake.tld/", nil) + cancel := make(chan struct{}) + req.Cancel = cancel + + doReturned := make(chan bool, 1) + madeRoundTripper := make(chan bool, 1) + + tr := &Transport{ + DisableKeepAlives: true, + TLSNextProto: map[string]func(string, *tls.Conn) RoundTripper{ + "foo": func(authority string, c *tls.Conn) RoundTripper { + madeRoundTripper <- true + return funcRoundTripper(func() { + t.Error("foo RoundTripper should not be called") + }) + }, + }, + Dial: func(_, _ string) (net.Conn, error) { + panic("shouldn't be called") + }, + DialTLS: func(_, _ string) (net.Conn, error) { + tc, err := tls.Dial("tcp", addr, &tls.Config{ + InsecureSkipVerify: true, + NextProtos: []string{"foo"}, + }) + if err != nil { + return nil, err + } + if err := tc.Handshake(); err != nil { + return nil, err + } + close(cancel) + <-doReturned + return tc, nil + }, + } + c := &Client{Transport: tr} + + _, err = c.Do(req) + if ue, ok := err.(*url.Error); !ok || ue.Err != ExportErrRequestCanceledConn { + t.Fatalf("Do error = %v; want url.Error with errRequestCanceledConn", err) + } + + doReturned <- true + <-madeRoundTripper + <-handledPendingDial +} + +var errFakeRoundTrip = errors.New("fake roundtrip") + +type funcRoundTripper func() + +func (fn funcRoundTripper) RoundTrip(*Request) (*Response, error) { + fn() + return nil, errFakeRoundTrip +} + func wantBody(res *Response, err error, want string) error { if err != nil { return err -- 2.50.0