admin: Sync server variables (fix #4260) (#4274)

* Synchronize server assignment/references to avoid data race

* only hold lock during var reassignment

2 files changed, 49 insertions, 20 deletions

diff --git a/admin.go b/admin.go
index ff5bc71..fb45168 100644
--- a/admin.go
+++ b/admin.go
@@ -335,6 +335,7 @@ func replaceLocalAdminServer(cfg *Config) error {
 		return err
 	}
 
+	serverMu.Lock()
 	localAdminServer = &http.Server{
 		Addr:              addr.String(), // for logging purposes only
 		Handler:           handler,
@@ -343,10 +344,14 @@ func replaceLocalAdminServer(cfg *Config) error {
 		IdleTimeout:       60 * time.Second,
 		MaxHeaderBytes:    1024 * 64,
 	}
+	serverMu.Unlock()
 
 	adminLogger := Log().Named("admin")
 	go func() {
-		if err := localAdminServer.Serve(ln); !errors.Is(err, http.ErrServerClosed) {
+		serverMu.Lock()
+		server := localAdminServer
+		serverMu.Unlock()
+		if err := server.Serve(ln); !errors.Is(err, http.ErrServerClosed) {
 			adminLogger.Error("admin server shutdown for unknown reason", zap.Error(err))
 		}
 	}()
@@ -474,6 +479,7 @@ func replaceRemoteAdminServer(ctx Context, cfg *Config) error {
 		return err
 	}
 
+	serverMu.Lock()
 	// create secure HTTP server
 	remoteAdminServer = &http.Server{
 		Addr:              addr.String(), // for logging purposes only
@@ -485,6 +491,7 @@ func replaceRemoteAdminServer(ctx Context, cfg *Config) error {
 		MaxHeaderBytes:    1024 * 64,
 		ErrorLog:          serverLogger,
 	}
+	serverMu.Unlock()
 
 	// start listener
 	ln, err := Listen(addr.Network, addr.JoinHostPort(0))
@@ -494,7 +501,10 @@ func replaceRemoteAdminServer(ctx Context, cfg *Config) error {
 	ln = tls.NewListener(ln, tlsConfig)
 
 	go func() {
-		if err := remoteAdminServer.Serve(ln); !errors.Is(err, http.ErrServerClosed) {
+		serverMu.Lock()
+		server := remoteAdminServer
+		serverMu.Unlock()
+		if err := server.Serve(ln); !errors.Is(err, http.ErrServerClosed) {
 			remoteLogger.Error("admin remote server shutdown for unknown reason", zap.Error(err))
 		}
 	}()
@@ -1229,6 +1239,7 @@ var bufPool = sync.Pool{
 
 // keep a reference to admin endpoint singletons while they're active
 var (
+	serverMu                            sync.Mutex
 	localAdminServer, remoteAdminServer *http.Server
 	identityCertCache                   *certmagic.Cache
 )
diff --git a/admin_test.go b/admin_test.go
index cfb4ab7..608a32c 100644
--- a/admin_test.go
+++ b/admin_test.go
@@ -17,9 +17,28 @@ package caddy
 import (
 	"encoding/json"
 	"reflect"
+	"sync"
 	"testing"
 )
 
+var testCfg = []byte(`{
+			"apps": {
+				"http": {
+					"servers": {
+						"myserver": {
+							"listen": ["tcp/localhost:8080-8084"],
+							"read_timeout": "30s"
+						},
+						"yourserver": {
+							"listen": ["127.0.0.1:5000"],
+							"read_header_timeout": "15s"
+						}
+					}
+				}
+			}
+		}
+		`)
+
 func TestUnsyncedConfigAccess(t *testing.T) {
 	// each test is performed in sequence, so
 	// each change builds on the previous ones;
@@ -108,25 +127,24 @@ func TestUnsyncedConfigAccess(t *testing.T) {
 	}
 }
 
+// TestLoadConcurrent exercises Load under concurrent conditions
+// and is most useful under test with `-race` enabled.
+func TestLoadConcurrent(t *testing.T) {
+	var wg sync.WaitGroup
+
+	for i := 0; i < 100; i++ {
+		wg.Add(1)
+		go func() {
+			_ = Load(testCfg, true)
+			wg.Done()
+		}()
+	}
+
+	wg.Wait()
+}
+
 func BenchmarkLoad(b *testing.B) {
 	for i := 0; i < b.N; i++ {
-		cfg := []byte(`{
-			"apps": {
-				"http": {
-					"servers": {
-						"myserver": {
-							"listen": ["tcp/localhost:8080-8084"],
-							"read_timeout": "30s"
-						},
-						"yourserver": {
-							"listen": ["127.0.0.1:5000"],
-							"read_header_timeout": "15s"
-						}
-					}
-				}
-			}
-		}
-		`)
-		Load(cfg, true)
+		Load(testCfg, true)
 	}
 }