Merge pull request #50 from DTW-Thalion/fix/cftree-remove-uaf

CFTree: fix CFTreeRemove so it actually unlinks the node

Author: fredkiefer

ChangeLog

@@ -13,6 +13,12 @@
 	multi-batch, and repeated-enumeration cases.
 
 2026-06-28  Todd White  <todd.white@thalion.global>
+	* Source/CFTree.c (CFTreeRemove): Unlink the node from its parent's
+	list of children (it walked the wrong list and never unlinked,
+	leaving a dangling pointer in the parent), update the parent's
+	_lastChild, and clear the node's _parent and _nextSibling.
+	* Tests/CFTree/basic.m: Regression tests for CFTreeRemove; release
+	the tree before its (unretained) children.
 	* Source/CFBinaryHeap.c (CFBinaryHeapCreateCopy): Enlarge the copy's
 	capacity to at least the source count before copying the values.
 	(CFBinaryHeapRemoveMinimumValue): Do nothing on an empty heap

Source/CFTree.c

@@ -195,18 +195,39 @@ CFTreePrependChild (CFTreeRef tree, CFTreeRef newChild)
 void
 CFTreeRemove (CFTreeRef tree)
 {
-  CFTreeRef child;
-  CFTreeRef previousSibling;
-  
-  previousSibling = NULL;
-  child = tree->_firstChild;
-  while (child != previousSibling)
-    child = child->_nextSibling;
-  
-  if (previousSibling)
-    previousSibling->_nextSibling = tree->_nextSibling;
-  
-  CFTreeFinalize (tree);
+  CFTreeRef parent = tree->_parent;
+
+  if (parent == NULL)
+    return;
+
+  /* Unlink the tree from its parent's list of children. */
+  if (parent->_firstChild == tree)
+    {
+      parent->_firstChild = tree->_nextSibling;
+    }
+  else
+    {
+      CFTreeRef previousSibling = parent->_firstChild;
+
+      while (previousSibling != NULL
+             && previousSibling->_nextSibling != tree)
+        previousSibling = previousSibling->_nextSibling;
+      if (previousSibling != NULL)
+        previousSibling->_nextSibling = tree->_nextSibling;
+    }
+
+  /* Fix up the parent's last-child pointer if we removed the last child. */
+  if (parent->_lastChild == tree)
+    {
+      CFTreeRef last = parent->_firstChild;
+
+      while (last != NULL && last->_nextSibling != NULL)
+        last = last->_nextSibling;
+      parent->_lastChild = last;
+    }
+
+  tree->_parent = NULL;
+  tree->_nextSibling = NULL;
 }
 
 void

Tests/CFTree/basic.m

@@ -52,10 +52,47 @@ int main (void)
     CFRelease (t2);
   }
 
+  {
+    /* CFTreeRemove must unlink the node from its parent.  It used to walk
+       the wrong list, never unlink, and prematurely finalize the node,
+       leaving a dangling pointer in the parent. */
+    CFTreeRef r  = CFTreeCreate (NULL, &ctxt);
+    CFTreeRef r1 = CFTreeCreate (NULL, &ctxt);
+    CFTreeRef r2 = CFTreeCreate (NULL, &ctxt);
+    CFTreeRef r3 = CFTreeCreate (NULL, &ctxt);
+
+    CFTreeAppendChild (r, r1);
+    CFTreeAppendChild (r, r2);
+    CFTreeAppendChild (r, r3);
+
+    CFTreeRemove (r2);
+    PASS_CF(CFTreeGetChildCount (r) == 2
+      && CFTreeGetNextSibling (r1) == r3
+      && CFTreeGetParent (r2) == NULL,
+      "CFTreeRemove unlinks a child from its parent.");
+
+    /* Removing the last child updates _lastChild, so a following append
+       links onto the right node. */
+    CFTreeRemove (r3);
+    CFTreeAppendChild (r, r2);
+    PASS_CF(CFTreeGetChildCount (r) == 2
+      && CFTreeGetChildAtIndex (r, 1) == r2,
+      "Append after removing the last child works.");
+
+    CFRelease (r);
+    CFRelease (r1);
+    CFRelease (r2);
+    CFRelease (r3);
+  }
+
+  /* Release the parent before its children: CFTreeAppendChild() does not
+     retain the child, so the children must outlive the tree that links
+     them. */
+  CFRelease (tree);
+
   CFRelease (child1);
   CFRelease (child2);
   CFRelease (child3);
-  CFRelease (tree);
-  
+
   return 0;
 }