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>

2 files changed, 72 insertions, 15 deletions

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 1b292ea2de..a3e43555bb 100644
--- 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 09434f1234..70d1864605 100644
--- 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,
+	}
+}