Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions src/parsers/yaml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2354,7 +2354,6 @@ pub struct Parser<'i, Enc: Encoding> {

pub stack_check: StackCheck,

pub merge_props_budget: usize,
pub alias_expansion_budget: usize,
}

Expand Down Expand Up @@ -2390,7 +2389,6 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
tag_handles: StringHashMap::default(),
whitespace_buf: Vec::new(),
stack_check: StackCheck::init(),
merge_props_budget: MappingProps::MAX_MERGED_PROPERTIES,
alias_expansion_budget: Self::MAX_ALIAS_EXPANSION,
}
}
Expand Down Expand Up @@ -2781,7 +2779,7 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
Expr::init(E::Null {}, self.token.start.loc())
};
let mut props = MappingProps::init();
props.append_maybe_merge(key, value, &mut self.merge_props_budget)?;
props.append_maybe_merge(key, value, &mut self.alias_expansion_budget)?;
Expr::init(
E::Object {
properties: props.move_list(),
Expand Down Expand Up @@ -2914,7 +2912,7 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
current_mapping_indent: Some(self.token.indent),
..Default::default()
})?;
props.append_maybe_merge(key, value, &mut self.merge_props_budget)?;
props.append_maybe_merge(key, value, &mut self.alias_expansion_budget)?;
}

// [140] ns-s-flow-map-entries: after an entry, only `,` or `}`.
Expand Down Expand Up @@ -3104,7 +3102,7 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
_ => Expr::init(E::Null {}, mapping_value_start.loc()),
};

props.append_maybe_merge(first_key, value, &mut self.merge_props_budget)?;
props.append_maybe_merge(first_key, value, &mut self.alias_expansion_budget)?;
}

if self.context.get() == Context::FlowIn {
Expand Down Expand Up @@ -3236,7 +3234,7 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}
};

props.append_maybe_merge(key, value, &mut self.merge_props_budget)?;
props.append_maybe_merge(key, value, &mut self.alias_expansion_budget)?;
}

Ok(Expr::init(
Expand Down Expand Up @@ -3270,8 +3268,6 @@ pub struct MappingProps {
}

impl MappingProps {
pub const MAX_MERGED_PROPERTIES: usize = 1024 * 1024;

pub fn init() -> Self {
Self {
list: bun_alloc::AstAlloc::vec(),
Expand All @@ -3289,8 +3285,18 @@ impl MappingProps {
&mut self,
merge_props: &[G::Property],
budget: &mut usize,
) -> Result<(), AllocError> {
self.list.reserve(merge_props.len().min(*budget));
) -> Result<(), ParseError> {
// The `merge_props` slice may have been produced by an inner `<<:`
// that already expanded an alias, so a chain of inline wrappers
// (`{<<: {<<: ... {<<: *big}}}`) re-materializes the same properties
// at every level without any further alias resolution. Charge each
// copy against the alias-expansion budget so nested merges are
// bounded the same way direct `*alias` references are.
*budget = budget
.checked_sub(merge_props.len())
.ok_or(ParseError::ExcessiveAliasing)?;

self.list.reserve(merge_props.len());

while self.merge_indexed < self.list.len() {
let idx = self.merge_indexed;
Expand All @@ -3314,7 +3320,6 @@ impl MappingProps {
}
}
}
*budget = budget.checked_sub(1).ok_or(AllocError)?;
// `G::Property` is not `Clone`; reconstruct from its `Copy` fields.
self.list.push(G::Property {
key: merge_prop.key,
Expand All @@ -3338,7 +3343,7 @@ impl MappingProps {
key: Expr,
value: Expr,
budget: &mut usize,
) -> Result<(), AllocError> {
) -> Result<(), ParseError> {
let is_merge_key = match &key.data {
ast::ExprData::EString(key_str) => key_str.eql_comptime(b"<<"),
_ => false,
Expand Down
74 changes: 62 additions & 12 deletions test/js/bun/yaml/yaml.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4473,33 +4473,83 @@
expect(elapsed).toBeLessThan(isDebug || isASAN ? 15_000 : 4_000);
}, 30_000);

test("limits how many properties merge keys can materialize from a small document", () => {
test("merge keys across many mappings are bounded only by the alias-expansion budget", () => {
// A normal merge-key document still resolves.
const small = YAML.parse("base: &base\n x: 1\n y: 2\nchild:\n <<: *base\n z: 3\n") as {
base: Record<string, number>;
child: Record<string, number>;
};
expect(small.child).toEqual({ x: 1, y: 2, z: 3 });

// One anchor with `keyCount` properties merged into `mergeCount` separate
// mappings would materialize keyCount * mergeCount (~1.2 million) property
// entries from a ~30 KB document. The parser caps the total number of
// properties materialized through merge keys and reports an error instead
// of allocating memory proportional to the product.
const keyCount = 2048;
const mergeCount = 600;
// One 64-key anchor merged into 16,500 separate mappings materializes just
// over a million properties from a ~380 KB document. Every `*base` reference
// is already charged against the alias-expansion budget (16M nodes), so the
// parser must accept this document rather than imposing a separate
// per-stream cap on merged properties.
const keyCount = 64;
const mergeCount = 16_500;

const lines: string[] = ["a: &a"];
const lines: string[] = ["base: &base"];
for (let i = 0; i < keyCount; i++) {
lines.push(` k${i}: ${i}`);
}
lines.push("out:");
for (let i = 0; i < mergeCount; i++) {
lines.push(`m${i}:`);
lines.push(" <<: *a");
lines.push(` m${i}:`);
lines.push(" <<: *base");
}
const input = lines.join("\n");

expect(() => YAML.parse(input)).toThrow();
const parsed = YAML.parse(input) as {
base: Record<string, number>;
out: Record<string, Record<string, number>>;
};

expect(Object.keys(parsed.base)).toHaveLength(keyCount);
expect(Object.keys(parsed.out)).toHaveLength(mergeCount);
expect(parsed.out.m0).toEqual(parsed.base);
expect(parsed.out[`m${mergeCount - 1}`]).toEqual(parsed.base);
expect(parsed.out.m0[`k${keyCount - 1}`]).toBe(keyCount - 1);
}, 120_000);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

test("bounds merge-key materialization through nested inline wrappers", () => {
// `{<<: {<<: ... {<<: *big}}}` resolves `*big` once but re-materializes
// every property at each nesting level, so depth * keyCount properties
// are allocated from an input that is linear in depth + keyCount. Each
// copy must be charged against the alias-expansion budget.
function wrap(keyCount: number, depth: number) {
const keys: string[] = [];
for (let i = 0; i < keyCount; i++) keys.push(`k${i}: ${i}`);
let out = "out: ";
for (let i = 0; i < depth; i++) out += "{<<: ";
out += "*big";
for (let i = 0; i < depth; i++) out += "}";
return [`big: &big {${keys.join(", ")}}`, out];
}

// A reasonable nesting depth still parses and produces the merged result.
const ok = YAML.parse(wrap(100, 50).join("\n") + "\n") as {
big: Record<string, number>;
out: Record<string, number>;
};
expect(Object.keys(ok.out)).toHaveLength(100);
expect(ok.out).toEqual(ok.big);

// Pre-consume the alias-expansion budget down to a small remainder, then
// show that nested-merge copies are charged against the same budget: 200
// inline wrappers around a 100-key anchor push it over the limit and are
// rejected rather than silently materializing depth * keyCount properties.
const width = 30;
const fan = (ref: string) => `[${new Array(width).fill(ref).join(", ")}]`;
const bomb = [
`a: &a ${fan("0")}`,
`b: &b ${fan("*a")}`,
`c: &c ${fan("*b")}`,
`d: &d ${fan("*c")}`,
`pad: [${new Array(18).fill("*d").concat(new Array(29).fill("*c")).join(", ")}]`,
];
const payload = bomb.concat(wrap(100, 200)).join("\n") + "\n";
expect(() => YAML.parse(payload)).toThrow(/[Ee]xcessive aliasing/);

Check warning on line 4552 in test/js/bun/yaml/yaml.test.ts

View check run for this annotation

Claude / Claude Code Review

Nested-merge regression test can pass for the wrong reason

nit: the `.toThrow(/[Ee]xcessive aliasing/)` assertion can pass for the wrong reason — the bomb pre-consumes ~16,759,547 of the 16,777,216 alias-expansion budget via hand-tuned magic numbers (18, 29), leaving only a ~0.1% margin, so any future tweak to `MAX_ALIAS_EXPANSION` or `charge_alias_expansion` node-counting will make `pad:` itself throw before the parser ever reaches the nested `{<<: ...}` section. Add `expect(() => YAML.parse(bomb.join("\n") + "\n")).not.toThrow()` before building `payl
Comment thread
robobun marked this conversation as resolved.
}, 30_000);

test("bounds alias expansion for parsed and imported YAML documents", async () => {
Expand Down
Loading