Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
32 changes: 9 additions & 23 deletions src/parsers/yaml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2354,7 +2354,6 @@

pub stack_check: StackCheck,

pub merge_props_budget: usize,
pub alias_expansion_budget: usize,
}

Expand Down Expand Up @@ -2390,7 +2389,6 @@
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 @@
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)?;
Expr::init(
E::Object {
properties: props.move_list(),
Expand Down Expand Up @@ -2914,7 +2912,7 @@
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)?;
}

// [140] ns-s-flow-map-entries: after an entry, only `,` or `}`.
Expand Down Expand Up @@ -3104,7 +3102,7 @@
_ => 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)?;
}

if self.context.get() == Context::FlowIn {
Expand Down Expand Up @@ -3236,7 +3234,7 @@
}
};

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

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

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

pub fn init() -> Self {
Self {
list: bun_alloc::AstAlloc::vec(),
Expand All @@ -3285,12 +3281,8 @@
Ok(())
}

pub fn merge(
&mut self,
merge_props: &[G::Property],
budget: &mut usize,
) -> Result<(), AllocError> {
self.list.reserve(merge_props.len().min(*budget));
pub fn merge(&mut self, merge_props: &[G::Property]) -> Result<(), AllocError> {
self.list.reserve(merge_props.len());

Check failure on line 3285 in src/parsers/yaml.rs

View check run for this annotation

Claude / Claude Code Review

Nested inline merge keys can amplify memory beyond alias_expansion_budget

Removing `merge_props_budget` opens a memory-amplification gap: nested inline merge wrappers around a single alias — `{<<: {<<: ... {<<: *big}}}` — re-materialize the anchor's K properties at every nesting level D, allocating D×K `G::Property` structs while `alias_expansion_budget` is charged only once (~2K). With K=100,000 and D in the hundreds (bounded only by `StackCheck`), a ~1.5 MB document produces tens of millions of arena allocations that the old 1M cap rejected. Consider decrementing `a
Comment thread
robobun marked this conversation as resolved.
Outdated

while self.merge_indexed < self.list.len() {
let idx = self.merge_indexed;
Expand All @@ -3314,7 +3306,6 @@
}
}
}
*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 @@ -3333,12 +3324,7 @@
Ok(())
}

pub fn append_maybe_merge(
&mut self,
key: Expr,
value: Expr,
budget: &mut usize,
) -> Result<(), AllocError> {
pub fn append_maybe_merge(&mut self, key: Expr, value: Expr) -> Result<(), AllocError> {
let is_merge_key = match &key.data {
ast::ExprData::EString(key_str) => key_str.eql_comptime(b"<<"),
_ => false,
Expand All @@ -3354,14 +3340,14 @@
}

match &value.data {
ast::ExprData::EObject(value_obj) => self.merge(value_obj.properties.slice(), budget),
ast::ExprData::EObject(value_obj) => self.merge(value_obj.properties.slice()),
ast::ExprData::EArray(value_arr) => {
for item in value_arr.items.slice() {
let item_obj = match &item.data {
ast::ExprData::EObject(obj) => obj,
_ => continue,
};
self.merge(item_obj.properties.slice(), budget)?;
self.merge(item_obj.properties.slice())?;
}
Ok(())
}
Expand Down
36 changes: 23 additions & 13 deletions test/js/bun/yaml/yaml.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4473,34 +4473,44 @@
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();
}, 30_000);
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);

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

View check run for this annotation

Claude / Claude Code Review

120s test timeout — workload could be reshaped to fit 30s convention

nit: the 120s timeout is 4× the file's 30s convention, and CLAUDE.md says "Don't raise per-test timeouts to make a slow test pass; shrink the workload." The test only needs to materialize >1,048,576 merged properties to guard against regression — e.g. `keyCount = 1024` × `mergeCount = 1030` crosses the same threshold with ~16× fewer mappings/JS objects and a ~10× smaller document, and should fit comfortably under 30s on debug+ASAN.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

test("bounds alias expansion for parsed and imported YAML documents", async () => {
// A document with a few levels of anchors, where each level is a sequence of
Expand Down
Loading