From 5e12a0baefa195e6f7fda07503ca41cd8a229397 Mon Sep 17 00:00:00 2001
From: Ted Robertson <10043369+tredondo@users.noreply.github.com>
Date: Tue, 19 May 2026 02:03:26 -0700
Subject: [PATCH] daemon: fix data race in getClientConfig iterating rules
 during load

Loader.GetAll() returned the loader internal map by reference under an

RLock that was released as the call returned. getClientConfig() then

called GetAll() twice -- once for len() to size a slice, once to range

over the entries -- without holding any lock. A concurrent goroutine

(e.g. the loader still synchronously processing rules during

reloadConfiguration) writing to the map between the two calls makes the

iteration yield more entries than the pre-sized slice, panicking with

"index out of range [N] with length N". On bad luck the Go runtime

trips "concurrent map iteration and map write" instead.

The race is reachable on real systems whenever the UI poller, started

by reloadConfiguration via c.Connect(), reconnects before

c.rules.Reload(path) finishes loading rules from disk. Consistently

reproducible with ~2100 rules on disk.

Make GetAll() return a shallow snapshot of the rule map under the read

lock so length and iteration are guaranteed consistent for the caller.

Update the one caller (getClientConfig) to take the snapshot once.

Add a regression test running concurrent writers and readers against

the loader; -race catches the original bug, and the new GetAll

contract makes it pass.
---
 daemon/rule/loader.go      | 16 ++++++-
 daemon/rule/loader_test.go | 85 ++++++++++++++++++++++++++++++++++++++
 daemon/ui/notifications.go | 10 +++--
 3 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/daemon/rule/loader.go b/daemon/rule/loader.go
index 390453a6a5..5fb4600cde 100644
--- a/daemon/rule/loader.go
+++ b/daemon/rule/loader.go
@@ -65,11 +65,23 @@ func (l *Loader) NumRules() int {
 	return len(l.rules)
 }
 
-// GetAll returns the loaded rules.
+// GetAll returns a shallow snapshot of the loaded rules.
+//
+// The returned map is freshly allocated under the loader's read lock, so
+// the caller may safely call len() on it and range over it without holding
+// any lock and without racing against concurrent rule loading or replacement.
+// The *Rule values are still shared with the loader, but rules are replaced
+// wholesale (replaceUserRule writes a new pointer into the map) rather than
+// mutated in place, so the snapshot remains internally consistent for the
+// read-only use cases this method serves (e.g. the UI client-config dump).
 func (l *Loader) GetAll() map[string]*Rule {
 	l.RLock()
 	defer l.RUnlock()
-	return l.rules
+	snapshot := make(map[string]*Rule, len(l.rules))
+	for k, v := range l.rules {
+		snapshot[k] = v
+	}
+	return snapshot
 }
 
 // EnableChecksums enables checksums field for rules globally.
diff --git a/daemon/rule/loader_test.go b/daemon/rule/loader_test.go
index 262f231565..59c21ba07d 100644
--- a/daemon/rule/loader_test.go
+++ b/daemon/rule/loader_test.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"math/rand"
 	"os"
+	"sync"
 	"testing"
 	"time"
 )
@@ -147,6 +148,90 @@ func TestRuleLoaderList(t *testing.T) {
 	}
 }
 
+// TestRuleLoaderGetAllSnapshot exercises the slice/map race that used to
+// crash daemon startup with a large rule corpus: getClientConfig() called
+// GetAll() twice without holding any lock, so the map could grow between
+// len() and the for-range and the iterator would walk off the end of the
+// pre-sized slice (or panic with "concurrent map iteration and map write"
+// on bad luck).
+//
+// Verify GetAll() now returns a consistent snapshot: iterating it produces
+// exactly len(snapshot) entries, even while other goroutines are mutating
+// the underlying loader. Run with -race to also catch the underlying data
+// race on l.rules that this fix removes.
+func TestRuleLoaderGetAllSnapshot(t *testing.T) {
+	t.Parallel()
+
+	l, err := NewLoader(false)
+	if err != nil {
+		t.Fatal("NewLoader: ", err)
+	}
+
+	var emptyOps []Operator
+	op, _ := NewOperator(Simple, false, OpTrue, "", emptyOps)
+	if err := op.Compile(); err != nil {
+		t.Fatal("Compile: ", err)
+	}
+
+	// Pre-seed with some rules so iterations have something to do.
+	for i := 0; i < 100; i++ {
+		r := Create(fmt.Sprintf("seed-%04d", i), "", true, false, false, Allow, Restart, op)
+		if err := l.replaceUserRule(r); err != nil {
+			t.Fatal("seed replaceUserRule: ", err)
+		}
+	}
+
+	stop := make(chan struct{})
+	var wg sync.WaitGroup
+
+	// Writers churn the map: add and remove rules continuously, exercising
+	// the same Lock()/Unlock() path as the daemon's loader during rule load.
+	for w := 0; w < 4; w++ {
+		wg.Add(1)
+		go func(w int) {
+			defer wg.Done()
+			for i := 0; ; i++ {
+				select {
+				case <-stop:
+					return
+				default:
+				}
+				name := fmt.Sprintf("churn-%d-%d", w, i)
+				r := Create(name, "", true, false, false, Allow, Restart, op)
+				_ = l.replaceUserRule(r)
+				if i&1 == 0 {
+					_ = l.Delete(name)
+				}
+			}
+		}(w)
+	}
+
+	// Readers do exactly what getClientConfig used to do incorrectly:
+	// observe len(), then iterate, and expect the two to agree.
+	for r := 0; r < 8; r++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for i := 0; i < 200; i++ {
+				rules := l.GetAll()
+				n := len(rules)
+				count := 0
+				for range rules {
+					count++
+				}
+				if count != n {
+					t.Errorf("GetAll snapshot inconsistent: len=%d iterated=%d", n, count)
+					return
+				}
+			}
+		}()
+	}
+
+	time.Sleep(200 * time.Millisecond)
+	close(stop)
+	wg.Wait()
+}
+
 func TestLiveReload(t *testing.T) {
 	t.Parallel()
 	t.Log("Test rules loader with live reload")
diff --git a/daemon/ui/notifications.go b/daemon/ui/notifications.go
index 16003bbace..424bf3a59e 100644
--- a/daemon/ui/notifications.go
+++ b/daemon/ui/notifications.go
@@ -37,10 +37,14 @@ func (c *Client) getClientConfig() *protocol.ClientConfig {
 	nodeName := core.GetHostname()
 	nodeVersion := core.GetKernelVersion()
 	var ts time.Time
-	rulesTotal := len(c.rules.GetAll())
-	ruleList := make([]*protocol.Rule, rulesTotal)
+	// GetAll() returns a snapshot, so len() and the range below see a
+	// consistent view even if the loader is concurrently adding rules
+	// (this used to panic with "index out of range" on big rule sets, when
+	// the UI subscribed back during rules.Reload()).
+	rules := c.rules.GetAll()
+	ruleList := make([]*protocol.Rule, len(rules))
 	idx := 0
-	for _, r := range c.rules.GetAll() {
+	for _, r := range rules {
 		ruleList[idx] = r.Serialize()
 		idx++
 	}