go/token: Fix race in FileSet.PositionFor.

Methods of FileSet are documented to be safe for concurrent use by
multiple goroutines, so FileSet is protected by a mutex and all its
methods use it to prevent concurrent mutations. All methods of File that
mutate the respective FileSet, including AddLine, do also lock its
mutex, but that does not help when PositionFor is invoked concurrently
and reads without synchronization what AddLine mutates.

The change adds acquiring a RLock around the racy call of File.position
and the respective test.

Fixes #16548

Change-Id: Iecaaa02630b2532cb29ab555376633ee862315dd
Reviewed-on: https://go-review.googlesource.com/25345
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/go/token/position.go b/src/go/token/position.go
index 7306083b0dd8ace37c4005dba49f42fdd7462f00..d4171d80e0c60435a699bb159c2562502bedf5d1 100644 (file)
--- a/src/go/token/position.go
+++ b/src/go/token/position.go
@@ -446,7 +446,9 @@ func (s *FileSet) File(p Pos) (f *File) {
 func (s *FileSet) PositionFor(p Pos, adjusted bool) (pos Position) {
        if p != NoPos {
                if f := s.file(p); f != nil {
+                       s.mutex.RLock()
                        pos = f.position(p, adjusted)
+                       s.mutex.RUnlock()
                }
        }
        return

diff --git a/src/go/token/position_test.go b/src/go/token/position_test.go
index d26939ce277e1c61a1e8f3e31a62f9b7acc9a2d0..63984bc872c824113058bd1526fa6b85f631a467 100644 (file)
--- a/src/go/token/position_test.go
+++ b/src/go/token/position_test.go
@@ -214,7 +214,7 @@ func TestFileSetCacheUnlikely(t *testing.T) {
        }
 }
 
-// issue 4345. Test concurrent use of FileSet.Pos does not trigger a
+// issue 4345. Test that concurrent use of FileSet.Pos does not trigger a
 // race in the FileSet position cache.
 func TestFileSetRace(t *testing.T) {
        fset := NewFileSet()
@@ -237,6 +237,35 @@ func TestFileSetRace(t *testing.T) {
        stop.Wait()
 }
 
+// issue 16548. Test that concurrent use of File.AddLine and FileSet.PositionFor
+// does not trigger a race in the FileSet position cache.
+func TestFileSetRace2(t *testing.T) {
+       const N = 1e3
+       var (
+               fset = NewFileSet()
+               file = fset.AddFile("", -1, N)
+               ch   = make(chan int, 2)
+       )
+
+       go func() {
+               for i := 0; i < N; i++ {
+                       file.AddLine(i)
+               }
+               ch <- 1
+       }()
+
+       go func() {
+               pos := file.Pos(0)
+               for i := 0; i < N; i++ {
+                       fset.PositionFor(pos, false)
+               }
+               ch <- 1
+       }()
+
+       <-ch
+       <-ch
+}
+
 func TestPositionFor(t *testing.T) {
        src := []byte(`
 foo