net/http: fix races cloning TLS config

Found in a Google program running under the race detector.
No test, but verified that this fixes the race with go run -race of:

	package main

	import (
	        "crypto/tls"
	        "fmt"
	        "net"
	        "net/http"
	        "net/http/httptest"
	)

	func main() {
	        for {
	                ts := httptest.NewTLSServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}))
	                conf := &tls.Config{} // non-nil
	                a, b := net.Pipe()
	                go func() {
	                        sconn := tls.Server(a, conf)
	                        sconn.Handshake()
	                }()
	                tr := &http.Transport{
	                        TLSClientConfig: conf,
	                }
	                req, _ := http.NewRequest("GET", ts.URL, nil)
	                _, err := tr.RoundTrip(req)
	                println(fmt.Sprint(err))
	                a.Close()
	                b.Close()
	                ts.Close()
	        }
	}

Also modified cmd/vet to report the copy-of-mutex bug statically
in CL 13646, and fixed two other instances in the code found by vet.
But vet could not have told us about cloneTLSConfig vs cloneTLSClientConfig.

Confirmed that original report is also fixed by this.

Fixes #12099.

Change-Id: Iba0171549e01852a5ec3438c25a1951c98524dec
Reviewed-on: https://go-review.googlesource.com/13453
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 1b292ea2de39e248becfafa8cf0c41d4aa3c6985..a3e43555bb37b4f120deeec2e5bd16277976b29d 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -473,7 +473,7 @@ func (srv *Server) newConn(rwc net.Conn) (c *conn, err error) {
        if debugServerConnections {
                c.rwc = newLoggingConn("server", c.rwc)
        }
-       c.sr = liveSwitchReader{r: c.rwc}
+       c.sr.r = c.rwc
        c.lr = io.LimitReader(&c.sr, noLimit).(*io.LimitedReader)
        br := newBufioReader(c.lr)
        bw := newBufioWriterSize(checkConnErrorWriter{c}, 4<<10)
@@ -2015,10 +2015,7 @@ func (srv *Server) ListenAndServeTLS(certFile, keyFile string) error {
        if addr == "" {
                addr = ":https"
        }
-       config := &tls.Config{}
-       if srv.TLSConfig != nil {
-               *config = *srv.TLSConfig
-       }
+       config := cloneTLSConfig(srv.TLSConfig)
        if config.NextProtos == nil {
                config.NextProtos = []string{"http/1.1"}
        }

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 09434f1234406ecd519f14a74ae09258bf7b8c1b..70d1864605993f5a46ab908251da112581d070ed 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -645,16 +645,9 @@ func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) {
 
        if cm.targetScheme == "https" && !tlsDial {
                // Initiate TLS and check remote host name against certificate.
-               cfg := t.TLSClientConfig
-               if cfg == nil || cfg.ServerName == "" {
-                       host := cm.tlsHost()
-                       if cfg == nil {
-                               cfg = &tls.Config{ServerName: host}
-                       } else {
-                               clone := *cfg // shallow clone
-                               clone.ServerName = host
-                               cfg = &clone
-                       }
+               cfg := cloneTLSClientConfig(t.TLSClientConfig)
+               if cfg.ServerName == "" {
+                       cfg.ServerName = cm.tlsHost()
                }
                plainConn := pconn.conn
                tlsConn := tls.Client(plainConn, cfg)
@@ -1399,3 +1392,70 @@ func isNetWriteError(err error) bool {
                return false
        }
 }
+
+// cloneTLSConfig returns a shallow clone of the exported
+// fields of cfg, ignoring the unexported sync.Once, which
+// contains a mutex and must not be copied.
+//
+// The cfg must not be in active use by tls.Server, or else
+// there can still be a race with tls.Server updating SessionTicketKey
+// and our copying it, and also a race with the server setting
+// SessionTicketsDisabled=false on failure to set the random
+// ticket key.
+//
+// If cfg is nil, a new zero tls.Config is returned.
+func cloneTLSConfig(cfg *tls.Config) *tls.Config {
+       if cfg == nil {
+               return &tls.Config{}
+       }
+       return &tls.Config{
+               Rand:                     cfg.Rand,
+               Time:                     cfg.Time,
+               Certificates:             cfg.Certificates,
+               NameToCertificate:        cfg.NameToCertificate,
+               GetCertificate:           cfg.GetCertificate,
+               RootCAs:                  cfg.RootCAs,
+               NextProtos:               cfg.NextProtos,
+               ServerName:               cfg.ServerName,
+               ClientAuth:               cfg.ClientAuth,
+               ClientCAs:                cfg.ClientCAs,
+               InsecureSkipVerify:       cfg.InsecureSkipVerify,
+               CipherSuites:             cfg.CipherSuites,
+               PreferServerCipherSuites: cfg.PreferServerCipherSuites,
+               SessionTicketsDisabled:   cfg.SessionTicketsDisabled,
+               SessionTicketKey:         cfg.SessionTicketKey,
+               ClientSessionCache:       cfg.ClientSessionCache,
+               MinVersion:               cfg.MinVersion,
+               MaxVersion:               cfg.MaxVersion,
+               CurvePreferences:         cfg.CurvePreferences,
+       }
+}
+
+// cloneTLSClientConfig is like cloneTLSConfig but omits
+// the fields SessionTicketsDisabled and SessionTicketKey.
+// This makes it safe to call cloneTLSClientConfig on a config
+// in active use by a server.
+func cloneTLSClientConfig(cfg *tls.Config) *tls.Config {
+       if cfg == nil {
+               return &tls.Config{}
+       }
+       return &tls.Config{
+               Rand:                     cfg.Rand,
+               Time:                     cfg.Time,
+               Certificates:             cfg.Certificates,
+               NameToCertificate:        cfg.NameToCertificate,
+               GetCertificate:           cfg.GetCertificate,
+               RootCAs:                  cfg.RootCAs,
+               NextProtos:               cfg.NextProtos,
+               ServerName:               cfg.ServerName,
+               ClientAuth:               cfg.ClientAuth,
+               ClientCAs:                cfg.ClientCAs,
+               InsecureSkipVerify:       cfg.InsecureSkipVerify,
+               CipherSuites:             cfg.CipherSuites,
+               PreferServerCipherSuites: cfg.PreferServerCipherSuites,
+               ClientSessionCache:       cfg.ClientSessionCache,
+               MinVersion:               cfg.MinVersion,
+               MaxVersion:               cfg.MaxVersion,
+               CurvePreferences:         cfg.CurvePreferences,
+       }
+}