net/http: conditionally configure HTTP/2 in Server.Serve(Listener)

Don't configure HTTP/2 in http.Server.Serve(net.Listener) if the
Server's TLSConfig is set and doesn't include the "h2" NextProto
value. This avoids mutating a *tls.Config already in use if
previously passed to tls.NewListener.

Also document this. (it's come up a few times now)

Fixes #15908

Change-Id: I283eed82fdb29a791f80d801aadd9f75db244de0
Reviewed-on: https://go-review.googlesource.com/24508Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

src/net/http/serve_test.go

@@ -1107,11 +1107,44 @@ func TestTLSServer(t *testing.T) {
 	})
 }
 
-func TestAutomaticHTTP2_Serve(t *testing.T) {
+// Issue 15908
+func TestAutomaticHTTP2_Serve_NoTLSConfig(t *testing.T) {
+	testAutomaticHTTP2_Serve(t, nil, true)
+}
+
+func TestAutomaticHTTP2_Serve_NonH2TLSConfig(t *testing.T) {
+	testAutomaticHTTP2_Serve(t, &tls.Config{}, false)
+}
+
+func TestAutomaticHTTP2_Serve_H2TLSConfig(t *testing.T) {
+	testAutomaticHTTP2_Serve(t, &tls.Config{NextProtos: []string{"h2"}}, true)
+}
+
+func testAutomaticHTTP2_Serve(t *testing.T, tlsConf *tls.Config, wantH2 bool) {
 	defer afterTest(t)
 	ln := newLocalListener(t)
 	ln.Close() // immediately (not a defer!)
 	var s Server
+	s.TLSConfig = tlsConf
+	if err := s.Serve(ln); err == nil {
+		t.Fatal("expected an error")
+	}
+	gotH2 := s.TLSNextProto["h2"] != nil
+	if gotH2 != wantH2 {
+		t.Errorf("http2 configured = %v; want %v", gotH2, wantH2)
+	}
+}
+
+func TestAutomaticHTTP2_Serve_WithTLSConfig(t *testing.T) {
+	defer afterTest(t)
+	ln := newLocalListener(t)
+	ln.Close() // immediately (not a defer!)
+	var s Server
+	// Set the TLSConfig. In reality, this would be the
+	// *tls.Config given to tls.NewListener.
+	s.TLSConfig = &tls.Config{
+		NextProtos: []string{"h2"},
+	}
 	if err := s.Serve(ln); err == nil {
 		t.Fatal("expected an error")
 	}

src/net/http/server.go

@@ -2222,9 +2222,37 @@ func (srv *Server) ListenAndServe() error {
 
 var testHookServerServe func(*Server, net.Listener) // used if non-nil
 
+// shouldDoServeHTTP2 reports whether Server.Serve should configure
+// automatic HTTP/2. (which sets up the srv.TLSNextProto map)
+func (srv *Server) shouldConfigureHTTP2ForServe() bool {
+	if srv.TLSConfig == nil {
+		// Compatibility with Go 1.6:
+		// If there's no TLSConfig, it's possible that the user just
+		// didn't set it on the http.Server, but did pass it to
+		// tls.NewListener and passed that listener to Serve.
+		// So we should configure HTTP/2 (to set up srv.TLSNextProto)
+		// in case the listener returns an "h2" *tls.Conn.
+		return true
+	}
+	// The user specified a TLSConfig on their http.Server.
+	// In this, case, only configure HTTP/2 if their tls.Config
+	// explicitly mentions "h2". Otherwise http2.ConfigureServer
+	// would modify the tls.Config to add it, but they probably already
+	// passed this tls.Config to tls.NewListener. And if they did,
+	// it's too late anyway to fix it. It would only be potentially racy.
+	// See Issue 15908.
+	return strSliceContains(srv.TLSConfig.NextProtos, http2NextProtoTLS)
+}
+
 // Serve accepts incoming connections on the Listener l, creating a
 // new service goroutine for each. The service goroutines read requests and
 // then call srv.Handler to reply to them.
+//
+// For HTTP/2 support, srv.TLSConfig should be initialized to the
+// provided listener's TLS Config before calling Serve. If
+// srv.TLSConfig is non-nil and doesn't include the string "h2" in
+// Config.NextProtos, HTTP/2 support is not enabled.
+//
 // Serve always returns a non-nil error.
 func (srv *Server) Serve(l net.Listener) error {
 	defer l.Close()
@@ -2232,9 +2260,13 @@ func (srv *Server) Serve(l net.Listener) error {
 		fn(srv, l)
 	}
 	var tempDelay time.Duration // how long to sleep on accept failure
-	if err := srv.setupHTTP2(); err != nil {
-		return err
+
+	if srv.shouldConfigureHTTP2ForServe() {
+		if err := srv.setupHTTP2(); err != nil {
+			return err
+		}
 	}
+
 	// TODO: allow changing base context? can't imagine concrete
 	// use cases yet.
 	baseCtx := context.Background()