Guard ExpressionParser initialization with sync.Once (#2789)

* Guard ExpressionParser initialization with sync.Once

The evaluators call InitExpressionParser on every Evaluate, so two goroutines
could construct a parser at the same time. newParticipleLexer also fills in the
ParticipleTokenType field on each shared participleYqRules entry as it builds
the lexer, which another goroutine could be reading through getYqDefinition.

These are pointer and struct field writes rather than map writes, so the
runtime does not stop the process; it corrupts quietly instead.

The nil check stays inside the Once so a caller that assigns ExpressionParser
itself is still respected.

Fixes #2788

* docs: use UK spelling for initialised in new comments

Matches the existing prose convention in pkg/yqlib (operator_reduce.go,
decoder_ini.go, operator_alternative_test.go). Review feedback on #2789.
This commit is contained in:
Michael Pursifull
2026-08-03 12:51:40 +10:00
committed by GitHub
parent e2e7bfe397
commit a0c6389b58
2 changed files with 50 additions and 3 deletions
+13 -3
View File
@@ -8,14 +8,24 @@ import (
"math"
"strconv"
"strings"
"sync"
)
var ExpressionParser ExpressionParserInterface
var expressionParserOnce sync.Once
// InitExpressionParser initialises the package level ExpressionParser, and is
// safe to call concurrently. The evaluators call it on every Evaluate, so
// without the guard two goroutines could construct a parser at the same time,
// and newParticipleLexer populates the shared participleYqRules entries as it
// goes, which a third goroutine could be reading through getYqDefinition.
func InitExpressionParser() {
if ExpressionParser == nil {
ExpressionParser = newExpressionParser()
}
expressionParserOnce.Do(func() {
if ExpressionParser == nil {
ExpressionParser = newExpressionParser()
}
})
}
var log = newLogger()
+37
View File
@@ -3,6 +3,7 @@ package yqlib
import (
"fmt"
"strings"
"sync"
"testing"
"github.com/mikefarah/yq/v4/test"
@@ -555,3 +556,39 @@ func TestProcessEscapeCharacters(t *testing.T) {
test.AssertResultComplexWithContext(t, tt.expected, actual, fmt.Sprintf("Input: %q", tt.input))
}
}
// TestInitExpressionParserConcurrent covers the data race described in #2788.
// The evaluators call InitExpressionParser on every Evaluate, so before it was
// guarded two goroutines could build a parser at the same time while a third
// read the shared participleYqRules entries that newParticipleLexer fills in.
//
// To reproduce the original race the process must not have initialised the
// parser yet, so run this test on its own with the detector enabled:
//
// go test -race -run TestInitExpressionParserConcurrent ./pkg/yqlib/
func TestInitExpressionParserConcurrent(t *testing.T) {
const goroutines = 32
var wg sync.WaitGroup
errs := make(chan error, goroutines)
for range goroutines {
wg.Add(1)
go func() {
defer wg.Done()
InitExpressionParser()
if _, err := ExpressionParser.ParseExpression(".a.b"); err != nil {
errs <- err
}
}()
}
wg.Wait()
close(errs)
for err := range errs {
t.Fatalf("concurrent ParseExpression failed: %v", err)
}
if ExpressionParser == nil {
t.Fatal("ExpressionParser should be initialised after concurrent InitExpressionParser calls")
}
}