From 886f02bdee34ecabc6c405837421c776582e5ccb Mon Sep 17 00:00:00 2001 From: Daisuke Maki Date: Thu, 4 Jun 2026 10:14:55 +0900 Subject: [PATCH] fix data race in Source.linesInRange under in-place compaction In-place compaction in Append overwrites and then clears positions in the backing array. linesInRange returned a slice aliased into that array and released the read lock before the caller iterated it, so a concurrent compaction could nil out the caller's view (panic on DisplayString()) and trip the race detector. Return a copy. Add a -race regression test that pins the contract. --- source.go | 11 +++++++--- source_capacity_test.go | 47 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/source.go b/source.go index b8fbd26..3b12127 100644 --- a/source.go +++ b/source.go @@ -324,12 +324,17 @@ func (s *Source) SetupDone() <-chan struct{} { return s.setupDone } -// linesInRange returns a slice of lines between from and to (indices into the -// live window) from the buffer. +// linesInRange returns the lines between from and to (indices into the live +// window) from the buffer. The returned slice is a copy: Append may compact +// the backing array in place after the lock is released, which would +// otherwise overwrite or clear the slice the caller is still iterating. func (s *Source) linesInRange(from, to int) []line.Line { s.mutex.RLock() defer s.mutex.RUnlock() - return s.lines[s.start+from : s.start+to] + src := s.lines[s.start+from : s.start+to] + out := make([]line.Line, len(src)) + copy(out, src) + return out } // LineAt returns the line at the given index (within the live window) from the buffer. diff --git a/source_capacity_test.go b/source_capacity_test.go index 080399b..436fd01 100644 --- a/source_capacity_test.go +++ b/source_capacity_test.go @@ -3,6 +3,7 @@ package peco import ( "fmt" "strings" + "sync" "testing" "github.com/peco/peco/line" @@ -74,3 +75,49 @@ func TestSourceUnlimitedRetainsAll(t *testing.T) { require.NoError(t, err) require.Equal(t, fmt.Sprintf("line%d", total-1), last.DisplayString()) } + +// TestSourceLinesInRangeConcurrentAppend verifies that the slice returned by +// linesInRange remains stable for the caller even when Append concurrently +// compacts the backing array. Run under -race, this catches any future +// regression where linesInRange returns a slice aliased into the live storage. +func TestSourceLinesInRangeConcurrentAppend(t *testing.T) { + const capacity = 32 + ig := newIDGen() + go ig.Run(t.Context()) + + s := NewSource("-", strings.NewReader(""), false, ig, capacity, false, false) + for i := range capacity { + s.Append(line.NewRaw(uint64(i), fmt.Sprintf("seed%d", i), false, false)) + } + + stop := make(chan struct{}) + var wg sync.WaitGroup + wg.Go(func() { + i := uint64(capacity) + for { + select { + case <-stop: + return + default: + } + s.Append(line.NewRaw(i, fmt.Sprintf("hot%d", i), false, false)) + i++ + } + }) + + // Repeatedly grab a window and read every element. If linesInRange + // returned a slice aliased into s.lines, a concurrent compaction would + // either nil out entries (post-clear) or rewrite them, producing a race + // detector hit and/or a nil DisplayString() deref. + for range 2000 { + rng := s.linesInRange(0, capacity) + require.Len(t, rng, capacity) + for _, l := range rng { + require.NotNil(t, l) + _ = l.DisplayString() + } + } + + close(stop) + wg.Wait() +}