diff --git a/pkg/yqlib/operator_traverse_path.go b/pkg/yqlib/operator_traverse_path.go index da98125223..5d152fc320 100644 --- a/pkg/yqlib/operator_traverse_path.go +++ b/pkg/yqlib/operator_traverse_path.go @@ -36,9 +36,33 @@ func traversePathOperator(_ *dataTreeNavigator, context Context, expressionNode return context.ChildContext(matches), nil } +// resolveAliasChain follows an alias chain iteratively, returning the +// first non-alias node. Returns an error if a cycle is detected. +func resolveAliasChain(node *CandidateNode) (*CandidateNode, error) { + if node.Kind != AliasNode { + return node, nil + } + visited := map[*CandidateNode]bool{} + for node.Kind == AliasNode { + if visited[node] { + return nil, fmt.Errorf("alias cycle detected") + } + visited[node] = true + log.Debug("its an alias!") + node = node.Alias + } + return node, nil +} + func traverse(context Context, matchingNode *CandidateNode, operation *Operation) (*list.List, error) { log.Debugf("Traversing %v", NodeToString(matchingNode)) + var err error + matchingNode, err = resolveAliasChain(matchingNode) + if err != nil { + return nil, err + } + if matchingNode.Tag == "!!null" && operation.Value != "[]" && !context.DontAutoCreate { log.Debugf("Guessing kind") // we must have added this automatically, lets guess what it should be now @@ -62,10 +86,6 @@ func traverse(context Context, matchingNode *CandidateNode, operation *Operation log.Debugf("its a sequence of %v things!", len(matchingNode.Content)) return traverseArray(matchingNode, operation, operation.Preferences.(traversePreferences)) - case AliasNode: - log.Debug("its an alias!") - matchingNode = matchingNode.Alias - return traverse(context, matchingNode, operation) default: return list.New(), nil } @@ -125,7 +145,13 @@ func traverseNodesWithArrayIndices(context Context, indicesToTraverse []*Candida return context.ChildContext(matchingNodeMap), nil } -func traverseArrayIndices(context Context, matchingNode *CandidateNode, indicesToTraverse []*CandidateNode, prefs traversePreferences) (*list.List, error) { // call this if doc / alias like the other traverse +func traverseArrayIndices(context Context, matchingNode *CandidateNode, indicesToTraverse []*CandidateNode, prefs traversePreferences) (*list.List, error) { + var err error + matchingNode, err = resolveAliasChain(matchingNode) + if err != nil { + return nil, err + } + if matchingNode.Tag == "!!null" { log.Debugf("OperatorArrayTraverse got a null - turning it into an empty array") // auto vivification @@ -138,9 +164,6 @@ func traverseArrayIndices(context Context, matchingNode *CandidateNode, indicesT } switch matchingNode.Kind { - case AliasNode: - matchingNode = matchingNode.Alias - return traverseArrayIndices(context, matchingNode, indicesToTraverse, prefs) case SequenceNode: return traverseArrayWithIndices(matchingNode, indicesToTraverse, prefs) case MappingNode: diff --git a/pkg/yqlib/operator_traverse_path_test.go b/pkg/yqlib/operator_traverse_path_test.go index 2527f0e222..6b2d63e3e8 100644 --- a/pkg/yqlib/operator_traverse_path_test.go +++ b/pkg/yqlib/operator_traverse_path_test.go @@ -665,6 +665,16 @@ var traversePathOperatorScenarios = []expressionScenario{ "D0, P[a], (!!null)::null\n", }, }, + { + // Regression test for https://issues.oss-fuzz.com/issues/390467412 + // go-yaml accepts cross-document alias references (invalid per + // YAML spec). A nested assignment on such an alias can create a + // circular alias node, which must not cause a stack overflow. + skipDoc: true, + document: "&-- a\n---\n*--", + expression: ". = (.x = 1)", + expectedError: "alias cycle detected", + }, } func TestTraversePathOperatorScenarios(t *testing.T) { @@ -682,3 +692,58 @@ func TestTraversePathOperatorAlignedToSpecScenarios(t *testing.T) { appendOperatorDocumentScenario(t, "traverse-read", fixedTraversePathOperatorScenarios) ConfiguredYamlPreferences.FixMergeAnchorToSpec = false } + +// Regression test for https://issues.oss-fuzz.com/issues/390467412 +// A circular alias (alias pointing back to itself) must not cause a +// stack overflow. resolveAliasChain should detect the cycle and return +// an error; both traverse() and traverseArrayIndices() use it. +func TestTraverseAliasCycle(t *testing.T) { + aliasNode := &CandidateNode{ + Kind: AliasNode, + } + aliasNode.Alias = aliasNode // A -> A + + op := &Operation{ + OperationType: traversePathOpType, + Value: "key", + StringValue: "key", + Preferences: traversePreferences{}, + } + _, err := traverse(Context{}, aliasNode, op) + if err == nil { + t.Fatal("expected error for alias cycle, got nil") + } + if err.Error() != "alias cycle detected" { + t.Fatalf("expected 'alias cycle detected', got %q", err.Error()) + } + + // Same cycle must be caught through the array traversal path. + _, err = traverseArrayIndices(Context{}, aliasNode, nil, traversePreferences{}) + if err == nil { + t.Fatal("expected error for alias cycle via traverseArrayIndices, got nil") + } + if err.Error() != "alias cycle detected" { + t.Fatalf("expected 'alias cycle detected', got %q", err.Error()) + } +} + +func TestTraverseAliasCycleChain(t *testing.T) { + nodeA := &CandidateNode{Kind: AliasNode} + nodeB := &CandidateNode{Kind: AliasNode} + nodeA.Alias = nodeB + nodeB.Alias = nodeA // A -> B -> A + + op := &Operation{ + OperationType: traversePathOpType, + Value: "key", + StringValue: "key", + Preferences: traversePreferences{}, + } + _, err := traverse(Context{}, nodeA, op) + if err == nil { + t.Fatal("expected error for alias cycle chain, got nil") + } + if err.Error() != "alias cycle detected" { + t.Fatalf("expected 'alias cycle detected', got %q", err.Error()) + } +}