prevent reference promotion to owner

The new rules are:

1. if you talloc() you must make preparation to free.
Usually the allocating reference being freed is enough, otherwise
explicitly talloc_free.

2. if you talloc_steal you must make preparation to free.
Usually the stealing reference being freed is enough, otherwise
explicitly talloc_free.

3. if you talloc_reference something you must make preparation to
remove the reference. Usually the reference itself being freed
is enough

4. you only call talloc_free once, on behalf of the allocator
or the stealer. If you want to remove a reference, then use
talloc_unreference

5. A function that calls talloc_free on an object has by definition
a passing-of-ownership in it's operation, and callers of the
function ought to be aware of this.

6. the destructor is called when the object is freed, not when
you call talloc_free. References might keep it alive for quite a while.

Signed-off-by: Sam Liddicott <sam@liddicott.com>

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index d5b0c3bfe53252d06ed0215515d2fcb3bf1f44fd..140da5304767f7cc238b34ecc13153999761a2dd 100644 (file)
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -103,6 +103,9 @@
 */
 static void *null_context;
 static void *autofree_context;
+static void *no_owner_context;
+void *talloc_no_owner_context(void);
+static inline int _talloc_free(void *ptr);
 
 struct talloc_reference_handle {
        struct talloc_reference_handle *next, *prev;
@@ -426,6 +429,11 @@ static int talloc_reference_destructor(struct talloc_reference_handle *handle)
 {
        struct talloc_chunk *ptr_tc = talloc_chunk_from_ptr(handle->ptr);
        _TLIST_REMOVE(ptr_tc->refs, handle);
+       /* If ptr_tc has no refs left and is owner by no_owner_context then we free it */
+       if (ptr_tc->refs == NULL &&
+               no_owner_context && talloc_parent(handle->ptr) == no_owner_context) {
+                       _talloc_free(handle->ptr);
+       }
        return 0;
 }
 
@@ -501,6 +509,10 @@ static inline int _talloc_free(void *ptr)
        tc = talloc_chunk_from_ptr(ptr);
 
        if (unlikely(tc->refs)) {
+       /*TODO: Fix this loop detection. The code here breaks top reference if
+               it is also one of our children. It is insufficient and can lead
+               to dangling references */
+
                int is_child;
                /* check this is a reference from a child or grantchild
                 * back to it's parent or grantparent
@@ -514,9 +526,8 @@ static inline int _talloc_free(void *ptr)
                        _talloc_free(tc->refs);
                        return _talloc_free(ptr);
                } else {
-                       /* the first reference becomes the owner */
-                       _talloc_steal(talloc_parent(tc->refs), ptr);
-                       _talloc_free(tc->refs);
+                       /* we can't free if we have refs, so no_owner_context becomes the owner */
+                       _talloc_steal(talloc_no_owner_context(), ptr);
                }
                return -1;
        }
@@ -552,22 +563,11 @@ static inline int _talloc_free(void *ptr)
        tc->flags |= TALLOC_FLAG_LOOP;
 
        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. */
                void *child = TC_PTR_FROM_CHUNK(tc->child);
-               const void *new_parent = null_context;
-               if (unlikely(tc->child->refs)) {
-                       struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs);
-                       if (p) new_parent = TC_PTR_FROM_CHUNK(p);
-               }
+               const void *new_parent = talloc_no_owner_context();
+               /* if talloc_free fails because of refs and not due to the destructor,
+                  it will already have a new owner no_owner_context */
                if (unlikely(_talloc_free(child) == -1)) {
-                       if (new_parent == null_context) {
-                               struct talloc_chunk *p = talloc_parent_chunk(ptr);
-                               if (p) new_parent = TC_PTR_FROM_CHUNK(p);
-                       }
                        talloc_steal(new_parent, child);
                }
        }
@@ -724,19 +724,7 @@ int talloc_unlink(const void *context, void *ptr)
                return _talloc_free(ptr);
        }
 
-       new_p = talloc_parent_chunk(tc_p->refs);
-       if (new_p) {
-               new_parent = TC_PTR_FROM_CHUNK(new_p);
-       } else {
-               new_parent = NULL;
-       }
-
-       if (talloc_unreference(new_parent, ptr) != 0) {
-               return -1;
-       }
-
-       talloc_steal(new_parent, ptr);
-
+       talloc_steal(talloc_no_owner_context(), ptr);
        return 0;
 }
 
@@ -915,10 +903,10 @@ void talloc_free_children(void *ptr)
                   pointer, the second choice is our parent, and the
                   final choice is the null context. */
                void *child = TC_PTR_FROM_CHUNK(tc->child);
-               const void *new_parent = null_context;
+               const void *new_parent = talloc_no_owner_context();
+
                if (unlikely(tc->child->refs)) {
-                       struct talloc_chunk *p = talloc_parent_chunk(tc->child->refs);
-                       if (p) new_parent = TC_PTR_FROM_CHUNK(p);
+                       new_parent = talloc_no_owner_context();
                }
                if (unlikely(_talloc_free(child) == -1)) {
                        if (new_parent == null_context) {
@@ -975,6 +963,18 @@ void *talloc_named_const(const void *context, size_t size, const char *name)
 */
 int talloc_free(void *ptr)
 {
+       struct talloc_chunk *tc;
+
+       if (unlikely(ptr == NULL)) {
+               return -1;
+       }
+
+       tc = talloc_chunk_from_ptr(ptr);
+
+       if (no_owner_context && tc->parent == no_owner_context) {
+           talloc_abort_double_free();
+       }
+
        return _talloc_free(ptr);
 }
 
@@ -1224,7 +1224,11 @@ static void talloc_report_depth_FILE_helper(const void *ptr, int depth, int max_
        FILE *f = (FILE *)_f;
 
        if (is_ref) {
-               fprintf(f, "%*sreference to: %s\n", depth*4, "", name);
+               if (talloc_parent(ptr) == no_owner_context && no_owner_context) {
+                       fprintf(f, "%*sreference to: %s [no owner]\n", depth*4, "", name);
+               } else {
+                       fprintf(f, "%*sreference to: %s\n", depth*4, "", name);
+               }
                return;
        }
 
@@ -1720,6 +1724,29 @@ void *talloc_autofree_context(void)
        return autofree_context;
 }
 
+/* this is only used by the test suite so that failures from on test don't
+   confuse the results of the next test */
+void talloc_erase_no_owner_context(void)
+{
+       /* force free all children */
+       if (no_owner_context) {
+               struct talloc_chunk *tc;
+               tc = talloc_chunk_from_ptr(no_owner_context);
+               tc->child=NULL;
+               tc->refs=NULL;
+               _talloc_free(no_owner_context);
+               no_owner_context=NULL;
+       }
+}
+
+void *talloc_no_owner_context(void)
+{
+       if (no_owner_context == NULL) {
+               no_owner_context = _talloc_named_const(NULL, 0, "no_owner_context");
+       }
+       return no_owner_context;
+}
+
 size_t talloc_get_size(const void *context)
 {
        struct talloc_chunk *tc;