From b0ba9589d754dd9a7827a87df57007eec0fdb483 Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Mon, 6 Apr 2026 01:30:44 -0700 Subject: [PATCH] Fix findInArray misuse on MappingNodes in equality and contains (#2645) recurseNodeObjectEqual and containsObject both used findInArray to locate keys in a MappingNode's Content array. findInArray steps by 1, so it matches against both keys (even indices) and values (odd indices). In recurseNodeObjectEqual, when a null key in the LHS matched a null value in the RHS at the last position, rhs.Content[indexInRHS+1] accessed an out-of-bounds index, causing a panic. In containsObject, a %2 guard prevented the panic but introduced false negatives: when a null value appeared before the actual null key, findInArray returned the value's odd index, the guard rejected it, and the function reported the key as missing. Both functions now use findKeyInMap, which steps by 2 and compares only key positions. The %2 guard in containsObject is removed. Reproducer for the panic (recurseNodeObjectEqual): echo '? [{~: ~}] : v1 ? [{2: ~}] : v2' | yq '. += .' Reproducer for the false negative (containsObject): printf '? 1\n: ~\n? ~\n: x\n' | yq 'contains({~: "x"})' Found by OSS-Fuzz via the lima project's FuzzEvaluateExpression target. https://issues.oss-fuzz.com/issues/383860504 Signed-off-by: Jan Dubois Co-authored-by: Claude Opus 4.6 (1M context) --- pkg/yqlib/lib.go | 2 +- pkg/yqlib/lib_test.go | 18 ++++++++++++++++++ pkg/yqlib/operator_add_test.go | 12 ++++++++++++ pkg/yqlib/operator_contains.go | 4 ++-- pkg/yqlib/operator_contains_test.go | 10 ++++++++++ 5 files changed, 43 insertions(+), 3 deletions(-) diff --git a/pkg/yqlib/lib.go b/pkg/yqlib/lib.go index b051983c..876353ac 100644 --- a/pkg/yqlib/lib.go +++ b/pkg/yqlib/lib.go @@ -80,7 +80,7 @@ func recurseNodeObjectEqual(lhs *CandidateNode, rhs *CandidateNode) bool { key := lhs.Content[index] value := lhs.Content[index+1] - indexInRHS := findInArray(rhs, key) + indexInRHS := findKeyInMap(rhs, key) if indexInRHS == -1 || !recursiveNodeEqual(value, rhs.Content[indexInRHS+1]) { return false diff --git a/pkg/yqlib/lib_test.go b/pkg/yqlib/lib_test.go index a6ad5786..23ed2853 100644 --- a/pkg/yqlib/lib_test.go +++ b/pkg/yqlib/lib_test.go @@ -300,6 +300,24 @@ func TestRecurseNodeObjectEqual(t *testing.T) { test.AssertResult(t, true, recurseNodeObjectEqual(obj1, obj2)) test.AssertResult(t, false, recurseNodeObjectEqual(obj1, obj3)) test.AssertResult(t, false, recurseNodeObjectEqual(obj1, obj4)) + + // A null key must not match a null value in the other map. + // Regression test for https://issues.oss-fuzz.com/issues/383860504 + nullKey := &CandidateNode{Kind: ScalarNode, Tag: "!!null"} + nullVal := &CandidateNode{Kind: ScalarNode, Tag: "!!null"} + intKey := createScalarNode(2, "2") + intKey.Tag = "!!int" + intVal := &CandidateNode{Kind: ScalarNode, Tag: "!!null"} + + mapWithNullKey := &CandidateNode{ + Kind: MappingNode, + Content: []*CandidateNode{nullKey, nullVal}, + } + mapWithIntKey := &CandidateNode{ + Kind: MappingNode, + Content: []*CandidateNode{intKey, intVal}, + } + test.AssertResult(t, false, recurseNodeObjectEqual(mapWithNullKey, mapWithIntKey)) } func TestParseInt(t *testing.T) { diff --git a/pkg/yqlib/operator_add_test.go b/pkg/yqlib/operator_add_test.go index 9d4d6e7b..d5cff38d 100644 --- a/pkg/yqlib/operator_add_test.go +++ b/pkg/yqlib/operator_add_test.go @@ -527,6 +527,18 @@ var addOperatorScenarios = []expressionScenario{ expression: `.a += [2]`, expectedError: "!!seq () cannot be added to a !!str (a)", }, + { + // Regression test for https://issues.oss-fuzz.com/issues/383860504 + // Adding a map to itself must not panic when sequence keys contain + // single-entry mappings with a null key in one and a non-null key + // in the other. + skipDoc: true, + document: "? [{~: ~}]\n: v1\n? [{2: ~}]\n: v2", + expression: `. += .`, + expected: []string{ + "D0, P[], (!!map)::? [{~: ~}]\n: v1\n? [{2: ~}]\n: v2\n", + }, + }, } func TestAddOperatorScenarios(t *testing.T) { diff --git a/pkg/yqlib/operator_contains.go b/pkg/yqlib/operator_contains.go index fa07d33f..dc7be8c8 100644 --- a/pkg/yqlib/operator_contains.go +++ b/pkg/yqlib/operator_contains.go @@ -46,9 +46,9 @@ func containsObject(lhs *CandidateNode, rhs *CandidateNode) (bool, error) { rhsKey := rhs.Content[index] rhsValue := rhs.Content[index+1] log.Debugf("Looking for %v in the lhs", rhsKey.Value) - lhsKeyIndex := findInArray(lhs, rhsKey) + lhsKeyIndex := findKeyInMap(lhs, rhsKey) log.Debugf("index is %v", lhsKeyIndex) - if lhsKeyIndex < 0 || lhsKeyIndex%2 != 0 { + if lhsKeyIndex < 0 { return false, nil } lhsValue := lhs.Content[lhsKeyIndex+1] diff --git a/pkg/yqlib/operator_contains_test.go b/pkg/yqlib/operator_contains_test.go index f714ace2..1fe1f964 100644 --- a/pkg/yqlib/operator_contains_test.go +++ b/pkg/yqlib/operator_contains_test.go @@ -65,6 +65,16 @@ var containsOperatorScenarios = []expressionScenario{ "D0, P[], (!!bool)::false\n", }, }, + { + // Regression: findInArray could match a null key against a null + // value at an earlier odd index, producing a false negative. + skipDoc: true, + document: "? 1\n: ~\n? ~\n: x", + expression: `contains({~: "x"})`, + expected: []string{ + "D0, P[], (!!bool)::true\n", + }, + }, { description: "String contains substring", document: `"foobar"`,