CFTree: retain children and the context info

ChangeLog

@@ -1,3 +1,26 @@
+2026-06-28  Todd White  <todd.white@thalion.global>
+	* Source/CFTree.c (CFTreeAppendChild, CFTreePrependChild,
+	CFTreeInsertSibling): Retain children with CFRetain when they are
+	added.
+	(CFTreeRemove, CFTreeRemoveAllChildren, CFTreeFinalize): Release the
+	children with CFRelease instead of finalizing them directly.
+	(CFTreeCreate): Retain the context info.
+	(CFTreeFinalize): Release the info (not the node) via the context
+	release callback.
+	(CFTreePrependChild): Set _lastChild to the new child for an empty
+	tree.
+	(CFTreeGetChildAtIndex): Stop at the end of the child list.
+	* Tests/CFTree/basic.m: Regression tests for child ownership and the
+	info retain/release callbacks.
+
+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.
+
 2021-09-30  Frederik Seiffert <frederik@algoriddim.com>
 	* Source/CFDate.c: Fix logic in CFGregorianDateIsValid()
 

Source/CFTree.c

@@ -62,18 +62,24 @@ CFTreeFinalize (CFTypeRef cf)
   CFTreeRef child;
   CFTreeRef tmp;
   CFTreeReleaseCallBack release;
-
+
+  /* Release the children we retained when they were added. */
   child = tree->_firstChild;
   while (child)
     {
       tmp = child->_nextSibling;
-      CFTreeFinalize (child); /* No need to go through CFRelease(). */
+      child->_parent = NULL;
+      child->_nextSibling = NULL;
+      CFRelease (child);
       child = tmp;
     }
-
+  tree->_firstChild = NULL;
+  tree->_lastChild = NULL;
+
+  /* Release the info supplied in the context. */
   release = tree->_context.release;
   if (release)
-    release (tree);
+    release (tree->_context.info);
 }
 
 static CFRuntimeClass CFTreeClass =
@@ -116,20 +122,20 @@ CFTreeCreate (CFAllocatorRef allocator, const CFTreeContext *context)
       if (context == NULL)
         context = &_kCFNullTreeContext;
       memcpy (&new->_context, context, sizeof(CFTreeContext));
+      if (new->_context.retain)
+        new->_context.info =
+          (void *)new->_context.retain (new->_context.info);
     }
-  
+
   return new;
 }
 
 void
 CFTreeAppendChild (CFTreeRef tree, CFTreeRef newChild)
 {
-  CFTreeRetainCallBack retain = newChild->_context.retain;
-
+  CFRetain (newChild);
   newChild->_parent = tree;
-  if (retain)
-    retain (newChild);
-
+
   if (tree->_firstChild == NULL)
     {
       tree->_firstChild = newChild;
@@ -149,12 +155,9 @@ CFTreeInsertSibling (CFTreeRef tree, CFTreeRef newSibling)
 
   if (parent != NULL && newSibling->_parent == NULL)
     {
-      CFTreeRetainCallBack retain = newSibling->_context.retain;
-
+      CFRetain (newSibling);
       newSibling->_parent = parent;
-      if (retain)
-        retain (newSibling);
-
+
       if (parent->_lastChild == tree)
         parent->_lastChild = newSibling;
       else
@@ -174,39 +177,66 @@ CFTreeRemoveAllChildren (CFTreeRef tree)
   while (child)
     {
       tmp = child->_nextSibling;
-      CFTreeFinalize (child);
+      child->_parent = NULL;
+      child->_nextSibling = NULL;
+      CFRelease (child);
       child = tmp;
     }
-  
+
   tree->_firstChild = NULL;
   tree->_lastChild = NULL;
 }
 
 void
 CFTreePrependChild (CFTreeRef tree, CFTreeRef newChild)
 {
+  CFRetain (newChild);
   newChild->_parent = tree;
   newChild->_nextSibling = tree->_firstChild;
   tree->_firstChild = newChild;
   if (tree->_lastChild == NULL)
-    tree->_lastChild = NULL;
+    tree->_lastChild = 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;
+
+  /* Drop the reference the parent took when the node was added. */
+  CFRelease (tree);
 }
 
 void
@@ -240,9 +270,9 @@ CFTreeGetChildAtIndex (CFTreeRef tree, CFIndex idx)
 
   j = 0;
   child = tree->_firstChild;
-  while (j++ < idx)
+  while (child != NULL && j++ < idx)
     child = child->_nextSibling;
-  
+
   return child;
 }
 

Tests/CFTree/basic.m

@@ -1,6 +1,22 @@
 #include "CoreFoundation/CFTree.h"
 #include "../CFTesting.h"
 
+static int infoRetains = 0;
+static int infoReleases = 0;
+
+static const void *
+countingRetain (const void *info)
+{
+  ++infoRetains;
+  return info;
+}
+
+static void
+countingRelease (const void *info)
+{
+  ++infoReleases;
+}
+
 int main (void)
 {
   CFTreeRef tree;
@@ -30,11 +46,72 @@ int main (void)
   PASS_CF(CFTreeGetNextSibling (child1) == child2,
     "Next sibling for child1 is child2.");
   PASS_CF(CFTreeGetChildAtIndex (tree, 2) == child3, "Child3 is at index 2");
-
+  PASS_CF(CFTreeGetChildAtIndex (tree, 5) == NULL,
+    "Out-of-range child index returns NULL.");
+
+  {
+    /* 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);
+  }
+
+  CFRelease (tree);
   CFRelease (child1);
   CFRelease (child2);
   CFRelease (child3);
-  CFRelease (tree);
-
+
+  {
+    /* The context's retain/release apply to the info: it is retained on
+       creation and released when the tree is finalized. */
+    int data = 0;
+    CFTreeContext ic;
+    CFTreeRef it;
+    CFTreeRef ic1;
+
+    ic.version = 0;
+    ic.info = &data;
+    ic.retain = countingRetain;
+    ic.release = countingRelease;
+    ic.copyDescription = NULL;
+
+    it = CFTreeCreate (NULL, &ic);
+    ic1 = CFTreeCreate (NULL, &ic);
+    PASS_CF(infoRetains == 2 && infoReleases == 0,
+      "The context retain callback is invoked for the info on creation.");
+    /* A child retained by its parent survives the caller's release. */
+    CFTreeAppendChild (it, ic1);
+    CFRelease (ic1);
+    CFRelease (it);
+    PASS_CF(infoRetains == 2 && infoReleases == 2,
+      "The context release callback balances the retains after finalize.");
+  }
+
   return 0;
 }