Merge pull request #512 from mnaza/fix/remove-double-free-500

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

crates/obscura-dom/src/tree.rs

@@ -469,12 +469,20 @@ impl DomTree {
             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 {
-            inner.nodes[desc_id.index()] = None;
-            inner.free_list.push(desc_id.0);
+            if matches!(inner.nodes.get(desc_id.index()), Some(Some(_))) {
+                inner.nodes[desc_id.index()] = None;
+                inner.free_list.push(desc_id.0);
+            }
+        }
+        if matches!(inner.nodes.get(node_id.index()), Some(Some(_))) {
+            inner.nodes[node_id.index()] = None;
+            inner.free_list.push(node_id.0);
         }
-        inner.nodes[node_id.index()] = None;
-        inner.free_list.push(node_id.0);
     }
 
     pub fn children(&self, node_id: NodeId) -> Vec<NodeId> {
@@ -932,6 +940,30 @@ mod tests {
         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]
     fn test_append_child() {
         let tree = DomTree::new();