DISCUSS:SIMPLE FIX talloc: improve the new_parent detection for abandoned children

We need to avoid that children get into a parent <-> child loop
were they're no children of the null_context anymore.

metze

WAS: e8403d0fc1a8cf3050d8132d82a75069b44fbb5a
WAS: a0872111e3ff27c8f6f5d2fcb5be8ec1961feb04

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index 4168b74dcee881341fa43699bc63179bc727987c..2d87069750adbed4b2ccc49abffdf5f6922b4762 100644 (file)
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1079,6 +1079,7 @@ static inline void _tc_free_poolmem(struct talloc_chunk *tc,
 
 static inline void _tc_free_children_internal(struct talloc_chunk *tc,
                                                  void *ptr,
+                                                 bool only_children,
                                                  const char *location);
 
 static inline int _talloc_free_internal(void *ptr, const char *location);
@@ -1159,7 +1160,7 @@ static inline int _tc_free_internal(struct talloc_chunk *tc,
 
        tc->flags |= TALLOC_FLAG_LOOP;
 
-       _tc_free_children_internal(tc, ptr, location);
+       _tc_free_children_internal(tc, ptr, false, location);
 
        _talloc_chunk_set_free(tc, location);
 
@@ -1627,34 +1628,84 @@ _PUBLIC_ void *talloc_init(const char *fmt, ...)
 
 static inline void _tc_free_children_internal(struct talloc_chunk *tc,
                                                  void *ptr,
+                                                 bool only_children,
                                                  const char *location)
 {
        while (tc->child) {
-               /* we need to work out who will own an abandoned child
-                  if it cannot be freed. In priority order, the first
-                  choice is owner of any remaining reference to this
-                  pointer, the second choice is our parent, and the
-                  final choice is the null context. */
+               /*
+                * we need to work out who will own an abandoned child
+                * if it cannot be freed. In priority order, the first
+                * choice is owner of any remaining reference to this
+                * pointer, the second choice is our parent followed
+                * by grandparents finally terminated by the null context.
+                *
+                * finding the parent here is potentially quite
+                * expensive, but the alternative, which is to change
+                * talloc to always have a valid tc->parent pointer,
+                * makes realloc more expensive where there are a
+                * large number of children.
+                *
+                * The reason we need the parent pointer here is that
+                * if _talloc_free_internal() fails due to references
+                * or a failing destructor we need to re-parent, but
+                * the free call can invalidate the prev pointer.
+                */
                void *child = TC_PTR_FROM_CHUNK(tc->child);
-               const void *new_parent = null_context;
+               const void *new_parent = NULL;
+               bool need_new_parent = false;
+
                if (unlikely(tc->child->refs)) {
-                       struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs);
-                       if (p) new_parent = TC_PTR_FROM_CHUNK(p);
+                       struct talloc_reference_handle *next = tc->child->refs;
+                       const void *invalid_parent;
+
+                       if (only_children) {
+                               invalid_parent = child;
+                       } else {
+                               invalid_parent = ptr;
+                       };
+
+                       /*
+                        * if the child has references
+                        * we should find a new parent for it
+                        */
+                       need_new_parent = true;
+
+                       while (next) {
+                               struct talloc_chunk *p;
+                               int is_child;
+                               struct talloc_reference_handle *ref = next;
+                               next = ref->next;
+
+                               is_child = talloc_is_parent(ref, invalid_parent);
+                               if (is_child) {
+                                       continue;
+                               }
+
+                               p = talloc_parent_chunk(ref);
+                               if (p) {
+                                       new_parent = TC_PTR_FROM_CHUNK(p);
+                                       break;
+                               }
+                       }
+               } else if (unlikely(tc->child->destructor)) {
+                       /*
+                        * if there is a destructor is attached
+                        * to the child there is a change that
+                        * _talloc_free_internal() returns -1
+                        * so we need to find a parent for the
+                        * child.
+                        */
+                       need_new_parent = true;
                }
-               /* finding the parent here is potentially quite
-                  expensive, but the alternative, which is to change
-                  talloc to always have a valid tc->parent pointer,
-                  makes realloc more expensive where there are a
-                  large number of children.
-
-                  The reason we need the parent pointer here is that
-                  if _talloc_free_internal() fails due to references
-                  or a failing destructor we need to re-parent, but
-                  the free call can invalidate the prev pointer.
-               */
-               if (new_parent == null_context && (tc->child->refs || tc->child->destructor)) {
-                       old_parent = talloc_parent_chunk(ptr);
+
+               if (unlikely(need_new_parent && new_parent == NULL)) {
+                       if (only_children) {
+                               new_parent = ptr;
+                       } else {
+                               new_parent = talloc_parent(ptr);
+                       }
                }
+
                if (unlikely(_tc_free_internal(tc->child, location) == -1)) {
                        if (talloc_parent_chunk(child) != tc) {
                                /*
@@ -1663,10 +1714,6 @@ static inline void _tc_free_children_internal(struct talloc_chunk *tc,
                                 */
                                continue;
                        }
-                       if (new_parent == null_context) {
-                               struct talloc_chunk *p = old_parent;
-                               if (p) new_parent = TC_PTR_FROM_CHUNK(p);
-                       }
                        _talloc_steal_internal(new_parent, child);
                }
        }
@@ -1701,7 +1748,7 @@ _PUBLIC_ void talloc_free_children(void *ptr)
                }
        }
 
-       _tc_free_children_internal(tc, ptr, __location__);
+       _tc_free_children_internal(tc, ptr, true, __location__);
 
        /* .. so we put it back after all other children have been freed */
        if (tc_name) {