fix(dom): guard DomTree::remove() against OOB and double-free

remove() freed a node and its descendants by directly indexing inner.nodes and
unconditionally pushing each slot onto the free list. An out-of-range id panicked
out-of-bounds, and removing an already-freed node pushed its slot a second time,
so two later allocations could be handed the same NodeId (aliasing/corruption).
Free a slot only when it is currently live, via checked access.

Fixes #500
This commit is contained in:
Andrey Mnatsakanov committed 2026-07-26 13:29:12 +02:00
1 parent 8078cc3070
commit 52e110b67c
1 file changed
+32
+32
View File
@@ -469,13 +469,21 @@ impl DomTree {
inner.id_index.remove(&id_str); inner.id_index.remove(&id_str);
} }
// Only free slots that are currently live. Freeing an out-of-range id
// would panic on direct indexing, and freeing an already-freed slot
// would push it onto the free list a second time — later handing the
// same NodeId to two live nodes (aliasing).
for desc_id in descendants { for desc_id in descendants {
if matches!(inner.nodes.get(desc_id.index()), Some(Some(_))) {
inner.nodes[desc_id.index()] = None; inner.nodes[desc_id.index()] = None;
inner.free_list.push(desc_id.0); inner.free_list.push(desc_id.0);
} }
}
if matches!(inner.nodes.get(node_id.index()), Some(Some(_))) {
inner.nodes[node_id.index()] = None; inner.nodes[node_id.index()] = None;
inner.free_list.push(node_id.0); inner.free_list.push(node_id.0);
} }
}
pub fn children(&self, node_id: NodeId) -> Vec<NodeId> { pub fn children(&self, node_id: NodeId) -> Vec<NodeId> {
let inner = self.inner.borrow(); let inner = self.inner.borrow();
@@ -932,6 +940,30 @@ mod tests {
assert!(node.is_document()); assert!(node.is_document());
} }
#[test]
fn remove_out_of_range_id_is_a_noop() {
let tree = DomTree::new();
// Direct indexing into `nodes` panicked out-of-bounds for an id past the
// end of the slot vector; it must be a no-op instead.
tree.remove(NodeId::new(9999));
assert_eq!(tree.len(), 1);
}
#[test]
fn remove_twice_does_not_alias_slots() {
let tree = DomTree::new();
let doc = tree.document();
let a = tree.new_node(NodeData::Text { contents: "a".into() });
tree.append_child(doc, a);
tree.remove(a);
// Removing the already-freed node again must not push its slot onto the
// free list a second time, or two later allocations alias one slot.
tree.remove(a);
let x = tree.new_node(NodeData::Text { contents: "x".into() });
let y = tree.new_node(NodeData::Text { contents: "y".into() });
assert_ne!(x, y, "double-free aliased two live nodes onto the same slot");
}
#[test] #[test]
fn test_append_child() { fn test_append_child() {
let tree = DomTree::new(); let tree = DomTree::new();