From 429e03cddc0123f8ab880fcb6cf8d4dc875834bf Mon Sep 17 00:00:00 2001
From: Julien Tant <785518+JulienTant@users.noreply.github.com>
Date: Thu, 25 Jun 2026 12:12:21 -0700
Subject: [PATCH] [MM-69464] Fix data race in pluginapi.ConfigureLogrus
 (#37183)

* [MM-69464] Fix data race in pluginapi.ConfigureLogrus

ConfigureLogrus registered the hook via logger.Hooks.Add, which mutates
the hook map without holding the logger mutex. logrus copies that map
under the mutex when firing hooks, so configuring the logger while another
goroutine logs through it is a data race that can panic with "concurrent
map read and map write". This is reachable when a plugin re-runs its
activation while goroutines from a prior activation still log through
logrus.StandardLogger() (originally reported as MM-52096).

Use logger.AddHook, which acquires the logger mutex. Also configure the
passed-in logger instead of the global standard logger: SetReportCaller
and SetLevel previously called the package-level logrus functions, silently
mutating logrus.StandardLogger() regardless of the logger argument.

Adds a -race regression test that configures the logger concurrently with
logging.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Use WaitGroup.Go in the regression test (golangci modernize)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Remove obvious comment on SetReportCaller

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 server/public/pluginapi/logrus.go      | 10 +++++---
 server/public/pluginapi/logrus_test.go | 33 ++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/server/public/pluginapi/logrus.go b/server/public/pluginapi/logrus.go
index e5e65b9ab22..56a602d5f94 100644
--- a/server/public/pluginapi/logrus.go
+++ b/server/public/pluginapi/logrus.go
@@ -60,10 +60,14 @@ func (lh *LogrusHook) Fire(entry *logrus.Entry) error {
 // discarding the default output to avoid duplicating the events across the standard STDOUT proxy.
 func ConfigureLogrus(logger *logrus.Logger, client *Client) {
 	hook := NewLogrusHook(client.Log)
-	logger.Hooks.Add(hook)
+
+	// AddHook (not Hooks.Add) takes the logger mutex, so registering the hook is
+	// safe against goroutines logging through the same logger concurrently.
+	logger.AddHook(hook)
 	logger.SetOutput(io.Discard)
-	logrus.SetReportCaller(true)
+
+	logger.SetReportCaller(true)
 
 	// By default, log everything to the server, and let it decide what gets through.
-	logrus.SetLevel(logrus.TraceLevel)
+	logger.SetLevel(logrus.TraceLevel)
 }
diff --git a/server/public/pluginapi/logrus_test.go b/server/public/pluginapi/logrus_test.go
index 04c5481a580..a5de2c5cb2a 100644
--- a/server/public/pluginapi/logrus_test.go
+++ b/server/public/pluginapi/logrus_test.go
@@ -1,6 +1,7 @@
 package pluginapi_test
 
 import (
+	"sync"
 	"testing"
 
 	"github.com/mattermost/mattermost/server/public/plugin/plugintest"
@@ -83,3 +84,35 @@ func TestLogrus(t *testing.T) {
 		})
 	}
 }
+
+// TestConfigureLogrusConcurrentWithLogging guards against a data race between
+// registering the hook and logging through the same logger. Run with -race.
+func TestConfigureLogrusConcurrentWithLogging(t *testing.T) {
+	api := &plugintest.API{}
+	api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe()
+	client := pluginapi.NewClient(api, &plugintest.Driver{})
+
+	logger := logrus.New()
+
+	var wg sync.WaitGroup
+	start := make(chan struct{})
+
+	for range 4 {
+		wg.Go(func() {
+			<-start
+			for range 500 {
+				logger.WithField("k", "v").Debug("message")
+			}
+		})
+	}
+
+	wg.Go(func() {
+		<-start
+		for range 50 {
+			pluginapi.ConfigureLogrus(logger, client)
+		}
+	})
+
+	close(start)
+	wg.Wait()
+}