From c42356e809e58812aeb9ca4181b8cca943266517 Mon Sep 17 00:00:00 2001 From: He-Pin Date: Wed, 5 Aug 2026 23:50:45 +0800 Subject: [PATCH] fix: std.parseYaml fails cleanly on cyclic aliases instead of crashing the JVM Motivation: A YAML document with a cyclic alias (e.g. `a: &x [*x]` or `a: &x {b: *x}`) made the JVM die with a raw java.lang.StackOverflowError ("Exception in thread main") instead of a Jsonnet runtime error, because SnakeYAML resolves an alias to the anchor's Node instance and the recursive yamlNodeToJson conversion re-enters the same collection forever. go-jsonnet and jrsonnet both report clean runtime errors for the same inputs. The JS/Native backends never crashed (scala-yaml rejects cyclic aliases at compose time), but their error message embeds a non-deterministic identity hash ("There is no anchor for ...Anchor@78 alias", upstream VirtusLab/scala-yaml#365), diverging from the JVM and untestable in goldens. Modification: - src-jvm Platform.scala: thread a per-document IdentityHashMap through the recursion and guard the two collection cases (MappingNode, SequenceNode). Re-entering an in-progress collection is a cycle and fails with "Recursive YAML alias reference"; a node revisited after completion is a shared (DAG) alias and stays legal (removed in finally). Scalar nodes are deliberately not tracked (they cannot contain references), so scalar-heavy documents pay zero extra work; collections pay one O(1) identity-hash put/remove each and the try/finally is exception-table based (no happy-path cost). - src-js / src-native Platform.scala: normalize scala-yaml's "There is no anchor for ..." compose error (only reachable for invalid YAML: cyclic or forward aliases) to the same "Recursive YAML alias reference" message, making the error identical across all backends. - Tests: three cyclic-error cases (sequence cycle, mapping cycle, cycle after successful conversions incl. a shared alias) plus two DAG tests (shared alias, diamond reuse). With the normalized message the same goldens match on every platform, so nothing is skipped. Result: Cyclic aliases produce a regular, deterministic runtime error at the std.parseYaml call site on all platforms instead of crashing the JVM; shared aliases are unchanged (regression-tested). Verified locally: ./mill 'sjsonnet.jvm[_].test' (all Scala versions), 'sjsonnet.native[2.13.18].test' (526/526, cyclic tests included on the scala-yaml backend), js/wasm/native compile, __.checkFormat. References: Found by four-way differential testing (sjsonnet vs go-jsonnet vs jrsonnet vs spec). Upstream message defect: VirtusLab/scala-yaml#365. --- sjsonnet/src-js/sjsonnet/Platform.scala | 9 +- sjsonnet/src-jvm/sjsonnet/Platform.scala | 85 ++++++++++++------- sjsonnet/src-native/sjsonnet/Platform.scala | 9 +- .../error.parseYaml_cyclic_alias.jsonnet | 3 + ...rror.parseYaml_cyclic_alias.jsonnet.golden | 3 + ...or.parseYaml_cyclic_alias_after_ok.jsonnet | 4 + ...eYaml_cyclic_alias_after_ok.jsonnet.golden | 3 + ...ror.parseYaml_cyclic_alias_mapping.jsonnet | 2 + ...seYaml_cyclic_alias_mapping.jsonnet.golden | 3 + .../parseYaml_shared_alias_dag.jsonnet | 6 ++ .../parseYaml_shared_alias_dag.jsonnet.golden | 1 + .../parseYaml_shared_alias_diamond.jsonnet | 11 +++ ...seYaml_shared_alias_diamond.jsonnet.golden | 1 + 13 files changed, 105 insertions(+), 35 deletions(-) create mode 100644 sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet.golden create mode 100644 sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet.golden create mode 100644 sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet.golden create mode 100644 sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet.golden create mode 100644 sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet.golden diff --git a/sjsonnet/src-js/sjsonnet/Platform.scala b/sjsonnet/src-js/sjsonnet/Platform.scala index dc0ecf6d1..da8a92e8b 100644 --- a/sjsonnet/src-js/sjsonnet/Platform.scala +++ b/sjsonnet/src-js/sjsonnet/Platform.scala @@ -464,7 +464,14 @@ object Platform { } ujson.Arr(buf) } - case Left(e) => Error.fail("Error converting YAML to JSON: " + e.getMessage) + case Left(e) => + // scala-yaml rejects cyclic (and forward) aliases at compose time with + // "There is no anchor for alias"; the hash is + // non-deterministic, so normalize to the same stable message the JVM + // backend produces. + if (e.getMessage != null && e.getMessage.startsWith("There is no anchor for")) + Error.fail("Recursive YAML alias reference") + else Error.fail("Error converting YAML to JSON: " + e.getMessage) } } diff --git a/sjsonnet/src-jvm/sjsonnet/Platform.scala b/sjsonnet/src-jvm/sjsonnet/Platform.scala index ab4d619ae..039b46f44 100644 --- a/sjsonnet/src-jvm/sjsonnet/Platform.scala +++ b/sjsonnet/src-jvm/sjsonnet/Platform.scala @@ -167,7 +167,18 @@ object Platform { private def parseYamlDecimalLong(value: String): ujson.Num = ujson.Num(java.lang.Long.parseLong(value).toDouble) - private def yamlNodeToJson(node: Node, input: String): ujson.Value = node match { + private def yamlNodeToJson(node: Node, input: String): ujson.Value = + yamlNodeToJson(node, input, new java.util.IdentityHashMap[Node, java.lang.Boolean]()) + + // SnakeYAML resolves an alias to the anchor's Node instance, so a cyclic alias + // (e.g. `a: &x [*x]`) re-enters a collection node that is still being converted + // and would overflow the stack. Only collection nodes are tracked (scalars + // cannot contain references); revisiting a completed node is a shared (DAG) + // alias and stays legal. + private def yamlNodeToJson( + node: Node, + input: String, + inProgress: java.util.IdentityHashMap[Node, java.lang.Boolean]): ujson.Value = node match { case sn: ScalarNode => val rawValue = sn.getValue // SnakeYAML 2.x strips the trailing newline from clip-chomped block scalars @@ -249,43 +260,51 @@ object Platform { } case mn: MappingNode => - val buf = upickle.core.LinkedHashMap[String, ujson.Value]() - buf.sizeHint(mn.getValue.size) - for (tuple <- mn.getValue.asScala) { - val keyNode = tuple.getKeyNode - if (keyNode.getTag == Tag.MERGE) { - // YAML merge key (<<): merge referenced mapping(s) with lower priority. - // Convert to JSON first so nested merge keys are resolved recursively. - val mergeObjs: Seq[ujson.Obj] = tuple.getValueNode match { - case mapNode: MappingNode => - Seq(yamlNodeToJson(mapNode, input).asInstanceOf[ujson.Obj]) - case seqNode: SequenceNode => - seqNode.getValue.asScala.map { node => - yamlNodeToJson(node, input).asInstanceOf[ujson.Obj] - }.toSeq - case other => Error.fail("Invalid YAML merge value: " + other.getTag) - } - for (obj <- mergeObjs; (k, v) <- obj.value) { - if (!buf.contains(k)) { - buf(k) = v + if (inProgress.put(mn, java.lang.Boolean.TRUE) != null) + Error.fail("Recursive YAML alias reference") + try { + val buf = upickle.core.LinkedHashMap[String, ujson.Value]() + buf.sizeHint(mn.getValue.size) + for (tuple <- mn.getValue.asScala) { + val keyNode = tuple.getKeyNode + if (keyNode.getTag == Tag.MERGE) { + // YAML merge key (<<): merge referenced mapping(s) with lower priority. + // Convert to JSON first so nested merge keys are resolved recursively. + val mergeObjs: Seq[ujson.Obj] = tuple.getValueNode match { + case mapNode: MappingNode => + Seq(yamlNodeToJson(mapNode, input, inProgress).asInstanceOf[ujson.Obj]) + case seqNode: SequenceNode => + seqNode.getValue.asScala.map { node => + yamlNodeToJson(node, input, inProgress).asInstanceOf[ujson.Obj] + }.toSeq + case other => Error.fail("Invalid YAML merge value: " + other.getTag) } + for (obj <- mergeObjs; (k, v) <- obj.value) { + if (!buf.contains(k)) { + buf(k) = v + } + } + } else { + val key = keyNode match { + case sn: ScalarNode => yamlScalarKey(sn, input) + case other => Error.fail("Invalid YAML mapping key type: " + other.getTag) + } + buf(key) = yamlNodeToJson(tuple.getValueNode, input, inProgress) } - } else { - val key = keyNode match { - case sn: ScalarNode => yamlScalarKey(sn, input) - case other => Error.fail("Invalid YAML mapping key type: " + other.getTag) - } - buf(key) = yamlNodeToJson(tuple.getValueNode, input) } - } - ujson.Obj(buf) + ujson.Obj(buf) + } finally inProgress.remove(mn) case sn: SequenceNode => - val buf = new mutable.ArrayBuffer[ujson.Value](sn.getValue.size) - for (n <- sn.getValue.asScala) { - buf += yamlNodeToJson(n, input) - } - ujson.Arr(buf) + if (inProgress.put(sn, java.lang.Boolean.TRUE) != null) + Error.fail("Recursive YAML alias reference") + try { + val buf = new mutable.ArrayBuffer[ujson.Value](sn.getValue.size) + for (n <- sn.getValue.asScala) { + buf += yamlNodeToJson(n, input, inProgress) + } + ujson.Arr(buf) + } finally inProgress.remove(sn) case _ => Error.fail("Unsupported YAML node type: " + node.getClass.getSimpleName) diff --git a/sjsonnet/src-native/sjsonnet/Platform.scala b/sjsonnet/src-native/sjsonnet/Platform.scala index 9b7a1a926..5f5f9909e 100644 --- a/sjsonnet/src-native/sjsonnet/Platform.scala +++ b/sjsonnet/src-native/sjsonnet/Platform.scala @@ -435,7 +435,14 @@ object Platform { } ujson.Arr(buf) } - case Left(e) => Error.fail("Error converting YAML to JSON: " + e.getMessage) + case Left(e) => + // scala-yaml rejects cyclic (and forward) aliases at compose time with + // "There is no anchor for alias"; the hash is + // non-deterministic, so normalize to the same stable message the JVM + // backend produces. + if (e.getMessage != null && e.getMessage.startsWith("There is no anchor for")) + Error.fail("Recursive YAML alias reference") + else Error.fail("Error converting YAML to JSON: " + e.getMessage) } } diff --git a/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet new file mode 100644 index 000000000..fe00ee71b --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet @@ -0,0 +1,3 @@ +// Cyclic YAML aliases must fail with a clean error instead of overflowing +// the stack (previously: java.lang.StackOverflowError crashed the process). +std.parseYaml("a: &x [*x]") diff --git a/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet.golden new file mode 100644 index 000000000..53f1ae4a5 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias.jsonnet.golden @@ -0,0 +1,3 @@ +sjsonnet.Error: [std.parseYaml] Recursive YAML alias reference + at [].(error.parseYaml_cyclic_alias.jsonnet:3:14) + diff --git a/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet new file mode 100644 index 000000000..e675dda5a --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet @@ -0,0 +1,4 @@ +// A cycle appearing only after several successful conversions (including a +// shared alias) must still be reported as a cycle: the in-progress set must +// neither false-positive on the earlier nodes nor miss the later cycle. +std.parseYaml("ok1: {v: 1}\nok2: [1, 2, 3]\nshared: &s {a: 1}\nreuse: *s\nbad: &x [*x]") diff --git a/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet.golden new file mode 100644 index 000000000..c4507879d --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_after_ok.jsonnet.golden @@ -0,0 +1,3 @@ +sjsonnet.Error: [std.parseYaml] Recursive YAML alias reference + at [].(error.parseYaml_cyclic_alias_after_ok.jsonnet:4:14) + diff --git a/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet new file mode 100644 index 000000000..63b83dab4 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet @@ -0,0 +1,2 @@ +// Cyclic alias through a mapping: the anchor value contains itself. +std.parseYaml("a: &x {b: *x}") diff --git a/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet.golden new file mode 100644 index 000000000..16c043cdb --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.parseYaml_cyclic_alias_mapping.jsonnet.golden @@ -0,0 +1,3 @@ +sjsonnet.Error: [std.parseYaml] Recursive YAML alias reference + at [].(error.parseYaml_cyclic_alias_mapping.jsonnet:2:14) + diff --git a/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet new file mode 100644 index 000000000..db27abcd5 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet @@ -0,0 +1,6 @@ +// Shared (non-cyclic) aliases must keep working after the cycle guard: +// the same anchor referenced twice is a DAG, not a cycle. +std.assertEqual( + std.parseYaml("a: &x {v: 1}\nb: *x\nc: *x"), + {a: {v: 1}, b: {v: 1}, c: {v: 1}} +) diff --git a/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet.golden new file mode 100644 index 000000000..27ba77dda --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_dag.jsonnet.golden @@ -0,0 +1 @@ +true diff --git a/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet new file mode 100644 index 000000000..b3baafe10 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet @@ -0,0 +1,11 @@ +// Diamond sharing plus nested alias reuse: the same anchor referenced multiple +// times at different positions (including twice within one sequence) is a DAG, +// not a cycle. The cycle guard must not reject it, and each reference converts +// to an equal value. +local dag = std.parseYaml("a: &x {v: 1}\nb: *x\nc: *x\nd: [*x, *x]\ninner: &in {w: 2}\ne: {p: *in, q: *in}"); +std.assertEqual(dag, { + a: {v: 1}, b: {v: 1}, c: {v: 1}, + d: [{v: 1}, {v: 1}], + inner: {w: 2}, + e: {p: {w: 2}, q: {w: 2}}, +}) diff --git a/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet.golden new file mode 100644 index 000000000..27ba77dda --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/parseYaml_shared_alias_diamond.jsonnet.golden @@ -0,0 +1 @@ +true