Browse Source

fix(tgbot): read the admin list and running flag under their mutex (#6491)

Start and Stop replace adminIds under tgBotMutex, but the report cron, the
backup job and every incoming callback (checkAdmin) read it unlocked. A
concurrent read of a slice header being replaced is not a benign race: it can
observe a torn header and iterate past the backing array. The readers now take
a snapshot under the same lock, and SendMsgToTgbot uses the existing IsRunning
accessor instead of reading the flag directly.
BlindMaster24 13 hours ago
parent
commit
615876b2eb

+ 8 - 0
internal/web/service/tgbot/tgbot.go

@@ -440,6 +440,14 @@ func (t *Tgbot) IsRunning() bool {
 	return isRunning
 }
 
+// adminSnapshot returns the admin chat list under the mutex Start and Stop
+// replace it under: a torn slice header is not a harmless race.
+func adminSnapshot() []int64 {
+	tgBotMutex.Lock()
+	defer tgBotMutex.Unlock()
+	return slices.Clone(adminIds)
+}
+
 // SetHostname sets the hostname for the bot.
 func (t *Tgbot) SetHostname() {
 	host, err := os.Hostname()

+ 73 - 0
internal/web/service/tgbot/tgbot_admin_list_race_test.go

@@ -0,0 +1,73 @@
+package tgbot
+
+import (
+	"sync"
+	"testing"
+)
+
+// Regression test: writers replace adminIds and isRunning under tgBotMutex, so
+// a bare read of either is reported by -race (CI's `race` job).
+func TestAdminListReadersShareTheWriterLock(t *testing.T) {
+	mock, _ := staleButtonServer(t, map[string]any{
+		"sendMessage": map[string]any{"ok": true, "result": map[string]any{
+			"message_id": 1,
+			"date":       0,
+			"chat":       map[string]any{"id": 1, "type": "private"},
+		}},
+	})
+	swapTestBot(t, mock.URL)
+	defer mock.Close()
+
+	tgBotMutex.Lock()
+	origAdmins := adminIds
+	origRunning := isRunning
+	tgBotMutex.Unlock()
+	t.Cleanup(func() {
+		tgBotMutex.Lock()
+		adminIds = origAdmins
+		isRunning = origRunning
+		tgBotMutex.Unlock()
+	})
+
+	stop := make(chan struct{})
+	var writer sync.WaitGroup
+	writer.Add(1)
+	go func() {
+		defer writer.Done()
+		for i := 0; ; i++ {
+			select {
+			case <-stop:
+				return
+			default:
+			}
+			// The same lock order Start and Stop write with.
+			tgBotMutex.Lock()
+			if i%2 == 0 {
+				adminIds = []int64{111, 222, 333}
+			} else {
+				adminIds = nil
+			}
+			isRunning = i%2 == 0
+			tgBotMutex.Unlock()
+		}
+	}()
+
+	tb := &Tgbot{}
+	var readers sync.WaitGroup
+	for range 4 {
+		readers.Add(1)
+		go func() {
+			defer readers.Done()
+			for range 300 {
+				checkAdmin(111)
+				_ = tb.IsRunning()
+				// SendMsgToTgbot reads isRunning itself; the mock bot keeps
+				// the live path cheap enough to run under -race.
+				tb.SendMsgToTgbot(1, "tick")
+			}
+		}()
+	}
+	readers.Wait()
+	close(stop)
+	writer.Wait()
+}

+ 4 - 3
internal/web/service/tgbot/tgbot_report.go

@@ -53,10 +53,11 @@ func (t *Tgbot) SendBackupToAdmins() {
 		logger.Error("Error in getting db backup: ", err)
 	}
 	dbFilename := t.serverService.BackupFilename("")
-	for i, adminId := range adminIds {
+	admins := adminSnapshot()
+	for i, adminId := range admins {
 		t.sendBackupData(adminId, dbData, dbFilename)
 		// Add delay between sends to avoid Telegram rate limits
-		if i < len(adminIds)-1 {
+		if i < len(admins)-1 {
 			time.Sleep(1 * time.Second)
 		}
 	}
@@ -67,7 +68,7 @@ func (t *Tgbot) sendExhaustedToAdmins() {
 	if !t.IsRunning() {
 		return
 	}
-	for _, adminId := range adminIds {
+	for _, adminId := range adminSnapshot() {
 		t.getExhausted(adminId)
 	}
 }

+ 1 - 1
internal/web/service/tgbot/tgbot_router.go

@@ -1332,7 +1332,7 @@ func (t *Tgbot) answerCallback(callbackQuery *telego.CallbackQuery, isAdmin bool
 
 // checkAdmin checks if the given Telegram ID is an admin.
 func checkAdmin(tgId int64) bool {
-	return slices.Contains(adminIds, tgId)
+	return slices.Contains(adminSnapshot(), tgId)
 }
 
 // isClientSelfCallback reports whether a callback is per-user rather than

+ 4 - 3
internal/web/service/tgbot/tgbot_send.go

@@ -121,7 +121,7 @@ func splitMessageLines(block string, limit int) []string {
 
 // SendMsgToTgbot sends a message to the Telegram bot with optional reply markup.
 func (t *Tgbot) SendMsgToTgbot(chatId int64, msg string, replyMarkup ...telego.ReplyMarkup) {
-	if !isRunning {
+	if !t.IsRunning() {
 		return
 	}
 
@@ -180,12 +180,13 @@ func (t *Tgbot) SendMsgToTgbot(chatId int64, msg string, replyMarkup ...telego.R
 
 // SendMsgToTgbotAdmins sends a message to all admin Telegram chats.
 func (t *Tgbot) SendMsgToTgbotAdmins(msg string, replyMarkup ...telego.ReplyMarkup) {
+	admins := adminSnapshot()
 	if len(replyMarkup) > 0 {
-		for _, adminId := range adminIds {
+		for _, adminId := range admins {
 			t.SendMsgToTgbot(adminId, msg, replyMarkup[0])
 		}
 	} else {
-		for _, adminId := range adminIds {
+		for _, adminId := range admins {
 			t.SendMsgToTgbot(adminId, msg)
 		}
 	}