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++ }