diff --git a/taskfile/ast/matrix.go b/taskfile/ast/matrix.go index aab0687d..8421b77e 100644 --- a/taskfile/ast/matrix.go +++ b/taskfile/ast/matrix.go @@ -93,6 +93,21 @@ func (matrix *Matrix) DeepCopy() *Matrix { } } +// DeepCopy returns a copy of the MatrixRow. Without this, deepcopy.OrderedMap +// falls back to copying the *MatrixRow pointer as-is, so every "copy" of a +// Matrix would still share the same underlying rows - see #2890, where +// concurrent invocations of a task with a `ref:` matrix row raced on +// resolveMatrixRefs mutating that shared row. +func (row *MatrixRow) DeepCopy() *MatrixRow { + if row == nil { + return nil + } + return &MatrixRow{ + Ref: row.Ref, + Value: deepcopy.Slice(row.Value), + } +} + func (matrix *Matrix) UnmarshalYAML(node *yaml.Node) error { switch node.Kind { case yaml.MappingNode: diff --git a/variables.go b/variables.go index c7c6cc84..733cfaf4 100644 --- a/variables.go +++ b/variables.go @@ -347,13 +347,14 @@ func itemsFromFor( var values []any // The list of values to loop over // Get the list from a matrix if f.Matrix.Len() != 0 { - if err := resolveMatrixRefs(f.Matrix, cache); err != nil { + resolvedMatrix, err := resolveMatrixRefs(f.Matrix, cache) + if err != nil { return nil, nil, errors.TaskfileInvalidError{ URI: location.Taskfile, Err: err, } } - return asAnySlice(product(f.Matrix)), nil, nil + return asAnySlice(product(resolvedMatrix)), nil, nil } // Get the list from the explicit for list if len(f.List) > 0 { @@ -425,22 +426,43 @@ func itemsFromFor( return values, keys, nil } -func resolveMatrixRefs(matrix *ast.Matrix, cache *templater.Cache) error { +// resolveMatrixRefs resolves any `ref:` rows in matrix and returns a new +// Matrix with those rows populated. It must not mutate the matrix passed in: +// that matrix is part of the shared, cached Task AST, and concurrent +// invocations of the same task (e.g. via parallel deps) call this with the +// same *ast.Matrix and would otherwise race on the row.Value assignment +// below, intermittently leaking a value resolved for one caller into another +// caller's expansion. See #2890. +func resolveMatrixRefs(matrix *ast.Matrix, cache *templater.Cache) (*ast.Matrix, error) { if matrix.Len() == 0 { - return nil + return matrix, nil } + hasRef := false for _, row := range matrix.All() { + if row.Ref != "" { + hasRef = true + break + } + } + if !hasRef { + return matrix, nil + } + resolved := matrix.DeepCopy() + for _, row := range resolved.All() { if row.Ref != "" { v := templater.ResolveRef(row.Ref, cache) + if cache.Err() != nil { + return nil, cache.Err() + } switch value := v.(type) { case []any: row.Value = value default: - return fmt.Errorf("matrix reference %q must resolve to a list", row.Ref) + return nil, fmt.Errorf("matrix reference %q must resolve to a list", row.Ref) } } } - return nil + return resolved, nil } func resolveEnumRefs(requires *ast.Requires, cache *templater.Cache) error { diff --git a/variables_test.go b/variables_test.go new file mode 100644 index 00000000..60a923da --- /dev/null +++ b/variables_test.go @@ -0,0 +1,45 @@ +package task + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/go-task/task/v3/internal/templater" + "github.com/go-task/task/v3/taskfile/ast" +) + +// TestResolveMatrixRefsDoesNotMutateInput is a regression test for #2890. The +// *ast.Matrix passed to resolveMatrixRefs is part of the shared, cached Task +// AST: the same *ast.Matrix is reused on every invocation of a task. If +// resolveMatrixRefs resolved `ref:` rows in place, concurrent invocations of +// the same task (e.g. via parallel deps) would race on that mutation and leak +// a value resolved for one caller into another caller's expansion. +// +// The invariant that prevents this is that resolveMatrixRefs must resolve into +// a copy and leave its input untouched, which this test asserts deterministically. +func TestResolveMatrixRefsDoesNotMutateInput(t *testing.T) { + t.Parallel() + + matrix := ast.NewMatrix( + &ast.MatrixElement{Key: "ARCH", Value: &ast.MatrixRow{Ref: ".ARCH_VAR"}}, + ) + + vars := ast.NewVars() + vars.Set("ARCH_VAR", ast.Var{Value: []any{"amd64"}}) + cache := &templater.Cache{Vars: vars} + + resolved, err := resolveMatrixRefs(matrix, cache) + require.NoError(t, err) + + // The returned matrix has the ref resolved... + row, ok := resolved.Get("ARCH") + require.True(t, ok, "ARCH row missing from resolved matrix") + require.Equal(t, []any{"amd64"}, row.Value) + + // ...but the shared input matrix must be left untouched. + orig, ok := matrix.Get("ARCH") + require.True(t, ok, "ARCH row missing from input matrix") + require.Nil(t, orig.Value, "input matrix was mutated: Ref rows must be resolved into a copy") + require.Equal(t, ".ARCH_VAR", orig.Ref, "input matrix Ref was altered") +}