diff --git a/util/syspolicy/source/policy_reader.go b/util/syspolicy/source/policy_reader.go index f97d200a1..ece08e3cb 100644 --- a/util/syspolicy/source/policy_reader.go +++ b/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() } diff --git a/util/syspolicy/source/policy_reader_test.go b/util/syspolicy/source/policy_reader_test.go index e5a893f56..0a477b0c6 100644 --- a/util/syspolicy/source/policy_reader_test.go +++ b/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)