changes to remove the ambiguity in talloc_free() and talloc_steal()

These changes follow from the discussions on samba-technical. The
changes are in several parts, and stem from the inherent ambiguity
that was in talloc_free() and talloc_steal() when the pointer that is
being changes has more than one parent, via references.

The changes are:

 1) when you call talloc_free() on a pointer with more than one parent
 the free will fail, and talloc will log an error to stderr like this:

    ERROR: talloc_free with references at some/foo.c:123
	   reference at other/bar.c:201
	   reference at other/foobar.c:641

 2) Similarly, when you call talloc_steal() on a pointer with more
 than one parent, the steal will fail and talloc will log an error to
 stderr like this:

    ERROR: talloc_steal with references at some/foo.c:123
	   reference at other/bar.c:201

 3) A new function talloc_reparent() has been added to change a parent
 in a controlled fashion. You need to supply both the old parent and
 the new parent. It handles the case whether either the old parent was
 a normal parent or a reference

The use of stderr in the logging is ugly (and potentially dangerous),
and will be removed in a future patch. We'll need to add a debug
registration function to talloc.

diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index 33cbfd7d268dc737e95948c44575ac6c53d835d7..bf2fb1f72e9148c0a7c88dad0c3c2232b3d3b872 100644 (file)
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -107,6 +107,7 @@ static void *autofree_context;
 struct talloc_reference_handle {
        struct talloc_reference_handle *next, *prev;
        void *ptr;
+       const char *location;
 };
 
 typedef int (*talloc_destructor_t)(void *);
@@ -465,7 +466,7 @@ static inline void *_talloc_named_const(const void *context, size_t size, const
   same underlying data, and you want to be able to free the two instances separately,
   and in either order
 */
-void *_talloc_reference(const void *context, const void *ptr)
+void *_talloc_reference(const void *context, const void *ptr, const char *location)
 {
        struct talloc_chunk *tc;
        struct talloc_reference_handle *handle;
@@ -482,6 +483,7 @@ void *_talloc_reference(const void *context, const void *ptr)
           own destructor on the context if they want to */
        talloc_set_destructor(handle, talloc_reference_destructor);
        handle->ptr = discard_const_p(void, ptr);
+       handle->location = location;
        _TLIST_ADD(tc->refs, handle);
        return handle->ptr;
 }
@@ -490,7 +492,7 @@ void *_talloc_reference(const void *context, const void *ptr)
 /* 
    internal talloc_free call
 */
-static inline int _talloc_free(void *ptr)
+static inline int _talloc_free_internal(void *ptr)
 {
        struct talloc_chunk *tc;
 
@@ -510,9 +512,9 @@ static inline int _talloc_free(void *ptr)
                 * pointer.
                 */
                is_child = talloc_is_parent(tc->refs, ptr);
-               _talloc_free(tc->refs);
+               _talloc_free_internal(tc->refs);
                if (is_child) {
-                       return _talloc_free(ptr);
+                       return _talloc_free_internal(ptr);
                }
                return -1;
        }
@@ -559,12 +561,12 @@ static inline int _talloc_free(void *ptr)
                        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 (unlikely(_talloc_free_internal(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_steal_internal(new_parent, child);
                }
        }
 
@@ -600,7 +602,7 @@ static inline int _talloc_free(void *ptr)
    ptr on success, or NULL if it could not be transferred.
    passing NULL as ptr will always return NULL with no side effects.
 */
-void *_talloc_steal(const void *new_ctx, const void *ptr)
+void *_talloc_steal_internal(const void *new_ctx, const void *ptr)
 {
        struct talloc_chunk *tc, *new_tc;
 
@@ -653,6 +655,66 @@ void *_talloc_steal(const void *new_ctx, const void *ptr)
 }
 
 
+/* 
+   move a lump of memory from one talloc context to another return the
+   ptr on success, or NULL if it could not be transferred.
+   passing NULL as ptr will always return NULL with no side effects.
+*/
+void *_talloc_steal(const void *new_ctx, const void *ptr, const char *location)
+{
+       struct talloc_chunk *tc;
+
+       if (unlikely(ptr == NULL)) {
+               return NULL;
+       }
+       
+       tc = talloc_chunk_from_ptr(ptr);
+       
+       if (unlikely(tc->refs != NULL) && talloc_parent(ptr) != new_ctx) {
+               struct talloc_reference_handle *h;
+               fprintf(stderr, "ERROR: talloc_steal with references at %s\n", location);
+               for (h=tc->refs; h; h=h->next) {
+                       fprintf(stderr, "\treference at %s\n", h->location);
+               }
+               return NULL;
+       }
+       
+       return _talloc_steal_internal(new_ctx, ptr);
+}
+
+/* 
+   this is like a talloc_steal(), but you must supply the old
+   parent. This resolves the ambiguity in a talloc_steal() which is
+   called on a context that has more than one parent (via references)
+
+   The old parent can be either a reference or a parent
+*/
+void *talloc_reparent(const void *old_parent, const void *new_parent, const void *ptr)
+{
+       struct talloc_chunk *tc;
+       struct talloc_reference_handle *h;
+
+       if (unlikely(ptr == NULL)) {
+               return NULL;
+       }
+
+       if (old_parent == talloc_parent(ptr)) {
+               return _talloc_steal_internal(new_parent, ptr);
+       }
+
+       tc = talloc_chunk_from_ptr(ptr);
+       for (h=tc->refs;h;h=h->next) {
+               if (talloc_parent(h) == old_parent) {
+                       if (_talloc_steal_internal(new_parent, h) != h) {
+                               return NULL;
+                       }
+                       return ptr;
+               }
+       }       
+
+       /* it wasn't a parent */
+       return NULL;
+}
 
 /*
   remove a secondary reference to a pointer. This undo's what
@@ -680,7 +742,7 @@ static inline int talloc_unreference(const void *context, const void *ptr)
                return -1;
        }
 
-       return _talloc_free(h);
+       return _talloc_free_internal(h);
 }
 
 /*
@@ -717,7 +779,7 @@ int talloc_unlink(const void *context, void *ptr)
        tc_p = talloc_chunk_from_ptr(ptr);
 
        if (tc_p->refs == NULL) {
-               return _talloc_free(ptr);
+               return _talloc_free_internal(ptr);
        }
 
        new_p = talloc_parent_chunk(tc_p->refs);
@@ -731,7 +793,7 @@ int talloc_unlink(const void *context, void *ptr)
                return -1;
        }
 
-       talloc_steal(new_parent, ptr);
+       _talloc_steal_internal(new_parent, ptr);
 
        return 0;
 }
@@ -784,7 +846,7 @@ void *talloc_named(const void *context, size_t size, const char *fmt, ...)
        va_end(ap);
 
        if (unlikely(name == NULL)) {
-               _talloc_free(ptr);
+               _talloc_free_internal(ptr);
                return NULL;
        }
 
@@ -882,7 +944,7 @@ void *talloc_init(const char *fmt, ...)
        va_end(ap);
 
        if (unlikely(name == NULL)) {
-               _talloc_free(ptr);
+               _talloc_free_internal(ptr);
                return NULL;
        }
 
@@ -916,12 +978,12 @@ void talloc_free_children(void *ptr)
                        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 (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_steal_internal(new_parent, child);
                }
        }
 
@@ -969,9 +1031,26 @@ void *talloc_named_const(const void *context, size_t size, const char *name)
    will not be freed if the ref_count is > 1 or the destructor (if
    any) returns non-zero
 */
-int talloc_free(void *ptr)
+int _talloc_free(void *ptr, const char *location)
 {
-       return _talloc_free(ptr);
+       struct talloc_chunk *tc;
+
+       if (unlikely(ptr == NULL)) {
+               return -1;
+       }
+       
+       tc = talloc_chunk_from_ptr(ptr);
+       
+       if (unlikely(tc->refs != NULL)) {
+               struct talloc_reference_handle *h;
+               fprintf(stderr, "ERROR: talloc_free with references at %s\n", location);
+               for (h=tc->refs; h; h=h->next) {
+                       fprintf(stderr, "\treference at %s\n", h->location);
+               }
+               return -1;
+       }
+       
+       return _talloc_free_internal(ptr);
 }
 
 
@@ -988,7 +1067,7 @@ void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *n
 
        /* size zero is equivalent to free() */
        if (unlikely(size == 0)) {
-               _talloc_free(ptr);
+               talloc_free(ptr);
                return NULL;
        }
 
@@ -1085,7 +1164,7 @@ void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *n
 void *_talloc_move(const void *new_ctx, const void *_pptr)
 {
        const void **pptr = discard_const_p(const void *,_pptr);
-       void *ret = _talloc_steal(new_ctx, *pptr);
+       void *ret = talloc_steal(new_ctx, *pptr);
        (*pptr) = NULL;
        return ret;
 }
@@ -1306,7 +1385,7 @@ void talloc_enable_null_tracking(void)
 */
 void talloc_disable_null_tracking(void)
 {
-       _talloc_free(null_context);
+       talloc_free(null_context);
        null_context = NULL;
 }
 
@@ -1688,7 +1767,7 @@ static int talloc_autofree_destructor(void *ptr)
 
 static void talloc_autofree(void)
 {
-       _talloc_free(autofree_context);
+       talloc_free(autofree_context);
 }
 
 /*

diff --git a/lib/talloc/talloc.h b/lib/talloc/talloc.h
index a4b33c3ed4ab5ab317f0b297a8d1e866592ce25c..9d1aa0df040a3b189a42966670b05236ed74821d 100644 (file)
--- a/lib/talloc/talloc.h
+++ b/lib/talloc/talloc.h
@@ -69,15 +69,15 @@ typedef void TALLOC_CTX;
        } while(0)
 /* this extremely strange macro is to avoid some braindamaged warning
    stupidity in gcc 4.1.x */
-#define talloc_steal(ctx, ptr) ({ _TALLOC_TYPEOF(ptr) __talloc_steal_ret = (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr)); __talloc_steal_ret; })
+#define talloc_steal(ctx, ptr) ({ _TALLOC_TYPEOF(ptr) __talloc_steal_ret = (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr), __location__); __talloc_steal_ret; })
 #else
 #define talloc_set_destructor(ptr, function) \
        _talloc_set_destructor((ptr), (int (*)(void *))(function))
 #define _TALLOC_TYPEOF(ptr) void *
-#define talloc_steal(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr))
+#define talloc_steal(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr), __location__)
 #endif
 
-#define talloc_reference(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_reference((ctx),(ptr))
+#define talloc_reference(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_reference((ctx),(ptr), __location__)
 #define talloc_move(ctx, ptr) (_TALLOC_TYPEOF(*(ptr)))_talloc_move((ctx),(void *)(ptr))
 
 /* useful macros for creating type checked pointers */
@@ -106,6 +106,8 @@ typedef void TALLOC_CTX;
 #define talloc_get_type_abort(ptr, type) (type *)_talloc_get_type_abort(ptr, #type, __location__)
 
 #define talloc_find_parent_bytype(ptr, type) (type *)talloc_find_parent_byname(ptr, #type)
+#define talloc_free(ctx) _talloc_free(ctx, __location__)
+
 
 #if TALLOC_DEPRECATED
 #define talloc_zero_p(ctx, type) talloc_zero(ctx, type)
@@ -124,7 +126,7 @@ void *talloc_pool(const void *context, size_t size);
 void _talloc_set_destructor(const void *ptr, int (*_destructor)(void *));
 int talloc_increase_ref_count(const void *ptr);
 size_t talloc_reference_count(const void *ptr);
-void *_talloc_reference(const void *context, const void *ptr);
+void *_talloc_reference(const void *context, const void *ptr, const char *location);
 int talloc_unlink(const void *context, void *ptr);
 const char *talloc_set_name(const void *ptr, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
 void talloc_set_name_const(const void *ptr, const char *name);
@@ -137,10 +139,12 @@ void *_talloc_get_type_abort(const void *ptr, const char *name, const char *loca
 void *talloc_parent(const void *ptr);
 const char *talloc_parent_name(const void *ptr);
 void *talloc_init(const char *fmt, ...) PRINTF_ATTRIBUTE(1,2);
-int talloc_free(void *ptr);
+int _talloc_free(void *ptr, const char *location);
 void talloc_free_children(void *ptr);
 void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *name);
-void *_talloc_steal(const void *new_ctx, const void *ptr);
+void *_talloc_steal(const void *new_ctx, const void *ptr, const char *location);
+void *_talloc_steal_internal(const void *new_ctx, const void *ptr);
+void *talloc_reparent(const void *old_parent, const void *new_parent, const void *ptr);
 void *_talloc_move(const void *new_ctx, const void *pptr);
 size_t talloc_total_size(const void *ptr);
 size_t talloc_total_blocks(const void *ptr);