talloc: 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.

Started-by: Sam Liddicott <sam@liddicott.com>
Finished-by: Stefan Metzmacher <metze@samba.org>
metze

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index dedf98a1a69d588083946a5bc2dc7c7673a25ec6..e28d91fc68c533223352e41ca4a0724245e6ebc8 100644 (file)
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -63,6 +63,8 @@
 #define TALLOC_FLAG_POOL 0x04          /* This is a talloc pool */
 #define TALLOC_FLAG_POOLMEM 0x08       /* This is allocated in a pool */
 #define TALLOC_MAGIC_REFERENCE ((const char *)1)
+#define TALLOC_MAGIC_NO_OWNER ((void *)1)
+#define TALLOC_MAGIC_NO_OWNER_TC ((struct talloc_chunk *)1)
 
 /* by default we abort when given a bad pointer (such as when talloc_free() is called 
    on a pointer that came from malloc() */
@@ -103,6 +105,9 @@
 */
 static void *null_context;
 static void *autofree_context;
+static void *no_owner_context = TALLOC_MAGIC_NO_OWNER;
+static const char *strict_owner_mode;
+static inline int _talloc_free(void *ptr);
 
 struct talloc_reference_handle {
        struct talloc_reference_handle *next, *prev;
@@ -138,6 +143,11 @@ struct talloc_chunk {
 #define TC_HDR_SIZE ((sizeof(struct talloc_chunk)+15)&~15)
 #define TC_PTR_FROM_CHUNK(tc) ((void *)(TC_HDR_SIZE + (char*)tc))
 
+void _talloc_set_strict_owner_mode(const char *location)
+{
+       strict_owner_mode = location;
+}
+
 static void (*talloc_abort_fn)(const char *reason);
 
 void talloc_set_abort_fn(void (*abort_fn)(const char *reason))
@@ -159,6 +169,11 @@ static void talloc_abort_double_free(void)
        talloc_abort("Bad talloc magic value - double free");
 }
 
+static void talloc_abort_no_owner_free(void)
+{
+       talloc_abort("Bad talloc parent - no_owner free");
+}
+
 static void talloc_abort_unknown_value(void)
 {
        talloc_abort("Bad talloc magic value - unknown value");
@@ -208,6 +223,26 @@ do { \
        if ((p) && ((p) != (list))) (p)->next = (p)->prev = NULL; \
 } while (0)
 
+static inline bool talloc_parent_is_no_owner(const void *ptr)
+{
+       struct talloc_chunk *tc;
+       struct talloc_chunk *no_owner_tc = TALLOC_MAGIC_NO_OWNER_TC;
+
+       tc = talloc_chunk_from_ptr(ptr);
+
+       if (no_owner_context != TALLOC_MAGIC_NO_OWNER) {
+               no_owner_tc = talloc_chunk_from_ptr(no_owner_context);
+       }
+
+       if (tc->parent == no_owner_tc) {
+               return true;
+       }
+
+       return false;
+}
+
+#define TALLOC_INVALID_PARENT(tc) \
+       ((((tc)->parent == NULL)||((tc)->parent == TALLOC_MAGIC_NO_OWNER_TC)))
 
 /*
   return the parent chunk of a pointer
@@ -223,6 +258,10 @@ static inline struct talloc_chunk *talloc_parent_chunk(const void *ptr)
        tc = talloc_chunk_from_ptr(ptr);
        while (tc->prev) tc=tc->prev;
 
+       if (unlikely(TALLOC_INVALID_PARENT(tc))) {
+               return NULL;
+       }
+
        return tc->parent;
 }
 
@@ -241,6 +280,27 @@ const char *talloc_parent_name(const void *ptr)
        return tc? tc->name : NULL;
 }
 
+static inline void talloc_remove_from_parent(struct talloc_chunk *tc,
+                                            bool reset_parent)
+{
+       if (!TALLOC_INVALID_PARENT(tc)) {
+               _TLIST_REMOVE(tc->parent->child, tc);
+               if (tc->parent->child) {
+                       tc->parent->child->parent = tc->parent;
+               }
+       } else {
+               if (tc->prev) tc->prev->next = tc->next;
+               if (tc->next) tc->next->prev = tc->prev;
+       }
+       tc->prev = tc->next = NULL;
+       /*
+        * Note: we only reset tc->parent = NULL when the caller asked for it
+        */
+       if (reset_parent) {
+               tc->parent = NULL;
+       }
+}
+
 /*
   A pool carries an in-pool object count count in the first 16 bytes.
   bytes. This is done to support talloc_steal() to a parent outside of the
@@ -428,6 +488,18 @@ 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 &&
+           talloc_parent_is_no_owner(handle->ptr)) {
+               int ret = _talloc_free(handle->ptr);
+               if (ret != 0) {
+                       _TLIST_ADD(ptr_tc->refs, handle);
+                       return ret;
+               }
+       }
        return 0;
 }
 
@@ -488,6 +560,41 @@ void *_talloc_reference(const void *context, const void *ptr)
        return handle->ptr;
 }
 
+static inline void _talloc_free_children(struct talloc_chunk *tc)
+{
+       void *ptr = TC_PTR_FROM_CHUNK(tc);
+
+       tc->flags |= TALLOC_FLAG_LOOP;
+
+       while (tc->child) {
+               void *child = TC_PTR_FROM_CHUNK(tc->child);
+               if (unlikely(_talloc_free(child) == -1)) {
+                       const void *new_parent = no_owner_context;
+                       struct talloc_chunk *p = talloc_parent_chunk(ptr);
+
+                       /*
+                        * we need to work out who will own an abandoned child
+                        * if it cannot be freed. In priority order, the first
+                        * choice is our parent, and the final choice is the
+                        * no owner context.
+                        */
+                       if (p) {
+                               new_parent = TC_PTR_FROM_CHUNK(p);
+                       }
+
+                       /*
+                        * if talloc_free fails because of refs and
+                        * not due to the destructor, it will already
+                        * have a new owner no_owner_context
+                        */
+                       if (!talloc_parent_is_no_owner(child)) {
+                               talloc_steal(new_parent, child);
+                       }
+               }
+       }
+
+       tc->flags &= ~TALLOC_FLAG_LOOP;
+}
 
 /* 
    internal talloc_free call
@@ -512,9 +619,24 @@ static inline int _talloc_free(void *ptr)
                 * pointer.
                 */
                is_child = talloc_is_parent(tc->refs, ptr);
-               _talloc_free(tc->refs);
                if (is_child) {
+                       _talloc_free(tc->refs);
                        return _talloc_free(ptr);
+               } else if (talloc_parent_is_no_owner(ptr)) {
+                       /*
+                        * if no_owner_context is already the owner
+                        * we free the last reference (first in the list)
+                        *
+                        * This path is ownly reached if
+                        * talloc_set_strict_owner_mode() wasn't used
+                        */
+                       return _talloc_free(tc->refs);
+               } else {
+                       /*
+                        * we can't free if we have refs,
+                        * so no_owner_context becomes the owner
+                        */
+                       _talloc_steal(no_owner_context, ptr);
                }
                return -1;
        }
@@ -537,38 +659,15 @@ static inline int _talloc_free(void *ptr)
                tc->destructor = NULL;
        }
 
-       if (tc->parent) {
-               _TLIST_REMOVE(tc->parent->child, tc);
-               if (tc->parent->child) {
-                       tc->parent->child->parent = tc->parent;
-               }
-       } else {
-               if (tc->prev) tc->prev->next = tc->next;
-               if (tc->next) tc->next->prev = tc->prev;
-       }
-
-       tc->flags |= TALLOC_FLAG_LOOP;
+       /*
+        * Note: we ask talloc_remove_from_parent() to keep
+        *       tc->parent valid, because _talloc_free_children()
+        *       needs it to find the parent for child where
+        *       the destructor rejects the _talloc_free()
+        */
+       talloc_remove_from_parent(tc, false);
 
-       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);
-               }
-               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);
-               }
-       }
+       _talloc_free_children(tc);
 
        tc->flags |= TALLOC_FLAG_FREE;
 
@@ -606,11 +705,24 @@ static inline int _talloc_free(void *ptr)
 void *_talloc_steal(const void *new_ctx, const void *ptr)
 {
        struct talloc_chunk *tc, *new_tc;
+       bool no_owner_null = false;
 
        if (unlikely(!ptr)) {
                return NULL;
        }
 
+       if (unlikely(strict_owner_mode &&
+                    talloc_parent_is_no_owner(ptr) &&
+                    new_ctx != no_owner_context)) {
+               talloc_abort_no_owner_free();
+               return NULL;
+       }
+
+       if (unlikely(new_ctx == TALLOC_MAGIC_NO_OWNER)) {
+               no_owner_null = true;
+               new_ctx = NULL;
+       }
+
        if (unlikely(new_ctx == NULL)) {
                new_ctx = null_context;
        }
@@ -618,17 +730,11 @@ void *_talloc_steal(const void *new_ctx, const void *ptr)
        tc = talloc_chunk_from_ptr(ptr);
 
        if (unlikely(new_ctx == NULL)) {
-               if (tc->parent) {
-                       _TLIST_REMOVE(tc->parent->child, tc);
-                       if (tc->parent->child) {
-                               tc->parent->child->parent = tc->parent;
-                       }
-               } else {
-                       if (tc->prev) tc->prev->next = tc->next;
-                       if (tc->next) tc->next->prev = tc->prev;
+               talloc_remove_from_parent(tc, true);
+
+               if (unlikely(no_owner_null)) {
+                       tc->parent = TALLOC_MAGIC_NO_OWNER_TC;
                }
-               
-               tc->parent = tc->next = tc->prev = NULL;
                return discard_const_p(void, ptr);
        }
 
@@ -638,15 +744,7 @@ void *_talloc_steal(const void *new_ctx, const void *ptr)
                return discard_const_p(void, ptr);
        }
 
-       if (tc->parent) {
-               _TLIST_REMOVE(tc->parent->child, tc);
-               if (tc->parent->child) {
-                       tc->parent->child->parent = tc->parent;
-               }
-       } else {
-               if (tc->prev) tc->prev->next = tc->next;
-               if (tc->next) tc->next->prev = tc->prev;
-       }
+       talloc_remove_from_parent(tc, true);
 
        tc->parent = new_tc;
        if (new_tc->child) new_tc->child->parent = NULL;
@@ -692,8 +790,7 @@ static inline int talloc_unreference(const void *context, const void *ptr)
 */
 int talloc_unlink(const void *context, void *ptr)
 {
-       struct talloc_chunk *tc_p, *new_p;
-       void *new_parent;
+       struct talloc_chunk *tc_p;
 
        if (ptr == NULL) {
                return -1;
@@ -723,19 +820,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(no_owner_context, ptr);
        return 0;
 }
 
@@ -907,26 +992,7 @@ void talloc_free_children(void *ptr)
 
        tc = talloc_chunk_from_ptr(ptr);
 
-       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);
-               }
-               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);
-               }
-       }
+       _talloc_free_children(tc);
 
        if ((tc->flags & TALLOC_FLAG_POOL)
            && (*talloc_pool_objectcount(tc) == 1)) {
@@ -974,6 +1040,25 @@ 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 (unlikely(tc->flags & TALLOC_FLAG_LOOP)) {
+               /* we have a free loop - stop looping */
+               return 0;
+       }
+
+       if (unlikely(strict_owner_mode &&
+                    talloc_parent_is_no_owner(ptr))) {
+               talloc_abort_no_owner_free();
+               return -1;
+       }
+
        return _talloc_free(ptr);
 }
 
@@ -1061,7 +1146,7 @@ void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *n
        if (malloced) {
                tc->flags &= ~TALLOC_FLAG_POOLMEM;
        }
-       if (tc->parent) {
+       if (!TALLOC_INVALID_PARENT(tc)) {
                tc->parent->child = tc;
        }
        if (tc->child) {
@@ -1223,7 +1308,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_is_no_owner(ptr)) {
+                       fprintf(f, "%*sreference to: %s [no owner]\n", depth*4, "", name);
+               } else {
+                       fprintf(f, "%*sreference to: %s\n", depth*4, "", name);
+               }
                return;
        }
 
@@ -1312,6 +1401,8 @@ void talloc_enable_null_tracking(void)
 {
        if (null_context == NULL) {
                null_context = _talloc_named_const(NULL, 0, "null_context");
+               no_owner_context = _talloc_named_const(null_context, 0,
+                                                      "no_owner_context");
        }
 }
 
@@ -1322,6 +1413,7 @@ void talloc_disable_null_tracking(void)
 {
        _talloc_free(null_context);
        null_context = NULL;
+       no_owner_context = TALLOC_MAGIC_NO_OWNER;
 }
 
 /*
@@ -1753,6 +1845,9 @@ void *talloc_find_parent_byname(const void *context, const char *name)
                }
                while (tc && tc->prev) tc = tc->prev;
                if (tc) {
+                       if (TALLOC_INVALID_PARENT(tc)) {
+                               break;
+                       }
                        tc = tc->parent;
                }
        }
@@ -1777,6 +1872,10 @@ void talloc_show_parents(const void *context, FILE *file)
                fprintf(file, "\t'%s'\n", talloc_get_name(TC_PTR_FROM_CHUNK(tc)));
                while (tc && tc->prev) tc = tc->prev;
                if (tc) {
+                       if (tc->parent == TALLOC_MAGIC_NO_OWNER_TC) {
+                               fprintf(file, "\t'no_owner_context'\n");
+                               break;
+                       }
                        tc = tc->parent;
                }
        }
@@ -1799,6 +1898,9 @@ int talloc_is_parent(const void *context, const void *ptr)
                if (TC_PTR_FROM_CHUNK(tc) == ptr) return 1;
                while (tc && tc->prev) tc = tc->prev;
                if (tc) {
+                       if (TALLOC_INVALID_PARENT(tc)) {
+                               break;
+                       }
                        tc = tc->parent;
                }
        }

diff --git a/lib/talloc/talloc.h b/lib/talloc/talloc.h
index a4b33c3ed4ab5ab317f0b297a8d1e866592ce25c..30dc9b7eb41c6f753a423da82d4db5da5c232e6c 100644 (file)
--- a/lib/talloc/talloc.h
+++ b/lib/talloc/talloc.h
@@ -187,4 +187,7 @@ char *talloc_asprintf_append_buffer(char *s, const char *fmt, ...) PRINTF_ATTRIB
 
 void talloc_set_abort_fn(void (*abort_fn)(const char *reason));
 
+#define talloc_set_strict_owner_mode() _talloc_set_strict_owner_mode(__location__)
+void _talloc_set_strict_owner_mode(const char *location);
+
 #endif

diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
index 426865a945fbefe2cf4f0074d28aecfef872f153..7ee60cc767f7a2ee247c6e3f35041b8da01beec2 100644 (file)
--- a/lib/talloc/testsuite.c
+++ b/lib/talloc/testsuite.c
@@ -1127,6 +1127,7 @@ static void test_reset(void)
        test_abort_stop();
        talloc_disable_null_tracking();
        talloc_enable_null_tracking();
+       talloc_set_strict_owner_mode();
 }
 
 struct torture_context;