util/syspolicy/source: fix data race between Reader.Close and reload

Reader.Close set r.store to nil without holding r.mu, while reload read
r.store while holding r.mu. If a policy store is closed while a
concurrent reload is in flight, reload could observe a nil store and
crash tailscaled with a nil interface method call in
readPolicySettingValue.

Nil out r.store only while holding r.mu, and make reload return the
last known policy once the reader is closing instead of reading from
a store that may no longer exist.

Fixes tailscale/corp#45548
Fixes tailscale/triage#394

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Change-Id: I494cfe9ea1df67b563bc061db9e6944f87b42a4e

util/syspolicy/source/policy_reader.go

@@ -125,6 +125,11 @@ func (r *Reader) ReadSettings() (*setting.Snapshot, error) {
 func (r *Reader) reload(force bool) (*setting.Snapshot, error) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
+	if r.closing {
+		// The reader is closing (or already closed) and r.store may be nil.
+		// Return the last known policy instead of reading from the store.
+		return r.lastPolicy, nil
+	}
 	if r.upToDate && !force {
 		return r.lastPolicy, nil
 	}
@@ -267,12 +272,13 @@ func (r *Reader) Close() error {
 			return err
 		}
 	}
-	r.store = nil
-
 	close(r.doneCh)
 
 	r.mu.Lock()
 	defer r.mu.Unlock()
+	// Nil out the store only while holding r.mu; reload reads r.store
+	// under the same lock, so writing it unlocked would be a data race.
+	r.store = nil
 	for _, c := range r.sessions {
 		c.closeInternal()
 	}

util/syspolicy/source/policy_reader_test.go

@@ -5,6 +5,7 @@ package source
 
 import (
 	"cmp"
+	"sync"
 	"testing"
 	"time"
 
@@ -244,6 +245,39 @@ func TestReaderLifecycle(t *testing.T) {
 	}
 }
 
+// TestReaderCloseReloadRace is a regression test for tailscale/corp#45548,
+// where [Reader.Close] set r.store to nil without holding r.mu while a
+// concurrent [Reader.reload] read r.store under r.mu, causing a data race
+// and a potential nil interface method call panic.
+func TestReaderCloseReloadRace(t *testing.T) {
+	setting.SetDefinitionsForTest(t, setting.NewDefinition("StringValue", setting.DeviceSetting, setting.StringValue))
+	origin := setting.NewNamedOrigin("Test", setting.DeviceScope)
+	for range 100 {
+		store := NewTestStoreOf(t, TestSettingOf("StringValue", "S1"))
+		reader, err := newReader(store, origin)
+		if err != nil {
+			t.Fatalf("newReader failed: %v", err)
+		}
+
+		var wg sync.WaitGroup
+		start := make(chan struct{})
+		for range 4 {
+			wg.Go(func() {
+				<-start
+				for range 10 {
+					reader.ReadSettings()
+				}
+			})
+		}
+		wg.Go(func() {
+			<-start
+			reader.Close()
+		})
+		close(start)
+		wg.Wait()
+	}
+}
+
 func TestReadingSession(t *testing.T) {
 	setting.SetDefinitionsForTest(t, setting.NewDefinition("StringValue", setting.DeviceSetting, setting.StringValue))
 	store := NewTestStore(t)