Skip to content

Commit a0b3d53

Browse files
committed
fix(dom): bound children() and ancestors() walks against cyclic chains
descendants() already caps its walk at nodes.len() as defense-in-depth against a corrupted/cyclic node graph, but children() (sibling chain) and ancestors() (parent chain) looped over next_sibling / parent pointers with no bound. A single corrupted pointer would hang these walkers — and every caller — forever, while descendants() would recover. The append_child / insert_before guards prevent such cycles through the public API, so this is hardening, not a reachable bug. Mirror the descendants() bound in both loops: stop once the collected count exceeds nodes.len(). Adds tests that forge a sibling cycle and a parent cycle by writing the node arena directly and assert each walk stays bounded instead of hanging. Closes #582
1 parent 97124ed commit a0b3d53

1 file changed

Lines changed: 84 additions & 0 deletions

File tree

crates/obscura-dom/src/tree.rs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -976,6 +976,13 @@ impl DomTree {
976976
.and_then(|n| n.first_child);
977977
while let Some(child_id) = current {
978978
result.push(child_id);
979+
// Defense in depth: a valid sibling chain is at most nodes.len()
980+
// long. Exceeding that means next_sibling forms a cycle (which the
981+
// append_child / insert_before guards prevent); stop rather than
982+
// loop forever. On a valid tree this bound is never reached.
983+
if result.len() > inner.nodes.len() {
984+
break;
985+
}
979986
current = inner.nodes.get(child_id.index())
980987
.and_then(|n| n.as_ref())
981988
.and_then(|n| n.next_sibling);
@@ -1287,6 +1294,12 @@ impl DomTree {
12871294
.and_then(|n| n.parent);
12881295
while let Some(parent_id) = current {
12891296
result.push(parent_id);
1297+
// Defense in depth: a valid parent chain is at most nodes.len()
1298+
// long. Exceeding that means parent forms a cycle (which the
1299+
// reparenting guards prevent); stop rather than loop forever.
1300+
if result.len() > inner.nodes.len() {
1301+
break;
1302+
}
12901303
current = inner.nodes.get(parent_id.index())
12911304
.and_then(|n| n.as_ref())
12921305
.and_then(|n| n.parent);
@@ -2243,4 +2256,75 @@ mod tests {
22432256

22442257
assert!(dest.len() >= 100_000);
22452258
}
2259+
2260+
/// SEC-008 / #582 — children() must terminate on a corrupted cyclic sibling
2261+
/// chain, the same way descendants() already does. The public mutation API
2262+
/// cannot create such a cycle, so we forge one by writing the node arena
2263+
/// directly, then assert the walk stays bounded instead of hanging forever.
2264+
#[test]
2265+
fn children_walk_is_bounded_on_corrupted_sibling_cycle() {
2266+
let tree = DomTree::new();
2267+
let doc = tree.document();
2268+
let mk = |n: &str| {
2269+
tree.new_node(NodeData::Element {
2270+
name: QualName::new(None, ns!(html), LocalName::from(n)),
2271+
attrs: vec![],
2272+
template_contents: None,
2273+
mathml_annotation_xml_integration_point: false,
2274+
})
2275+
};
2276+
let root = mk("root");
2277+
let a = mk("a");
2278+
let b = mk("b");
2279+
tree.append_child(doc, root);
2280+
tree.append_child(root, a);
2281+
tree.append_child(root, b);
2282+
2283+
// Forge a sibling cycle a -> a that append_child never produces.
2284+
{
2285+
let mut inner = tree.inner.borrow_mut();
2286+
inner.nodes[a.index()].as_mut().unwrap().next_sibling = Some(a);
2287+
}
2288+
2289+
let node_count = tree.inner.borrow().nodes.len();
2290+
let kids = tree.children(root);
2291+
assert!(
2292+
kids.len() <= node_count + 1,
2293+
"children() must stay bounded on a cyclic sibling chain, got {}",
2294+
kids.len()
2295+
);
2296+
}
2297+
2298+
/// SEC-008 / #582 — ancestors() companion to the children() cycle test.
2299+
#[test]
2300+
fn ancestors_walk_is_bounded_on_corrupted_parent_cycle() {
2301+
let tree = DomTree::new();
2302+
let doc = tree.document();
2303+
let mk = |n: &str| {
2304+
tree.new_node(NodeData::Element {
2305+
name: QualName::new(None, ns!(html), LocalName::from(n)),
2306+
attrs: vec![],
2307+
template_contents: None,
2308+
mathml_annotation_xml_integration_point: false,
2309+
})
2310+
};
2311+
let root = mk("root");
2312+
let child = mk("child");
2313+
tree.append_child(doc, root);
2314+
tree.append_child(root, child);
2315+
2316+
// Forge a parent cycle child -> child.
2317+
{
2318+
let mut inner = tree.inner.borrow_mut();
2319+
inner.nodes[child.index()].as_mut().unwrap().parent = Some(child);
2320+
}
2321+
2322+
let node_count = tree.inner.borrow().nodes.len();
2323+
let ancestors = tree.ancestors(child);
2324+
assert!(
2325+
ancestors.len() <= node_count + 1,
2326+
"ancestors() must stay bounded on a cyclic parent chain, got {}",
2327+
ancestors.len()
2328+
);
2329+
}
22462330
}

0 commit comments

Comments
 (0)