From 5fe1d8dc1275e43d96da1297f5fb0d0088a1c3ab Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 1 Jul 2009 14:53:01 +1000 Subject: 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. --- lib/talloc/talloc.c | 119 +++++++++++++++++++++++++++++++++++++++++++--------- lib/talloc/talloc.h | 16 ++++--- 2 files changed, 109 insertions(+), 26 deletions(-) (limited to 'lib') diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index 33cbfd7d26..bf2fb1f72e 100644 --- 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 a4b33c3ed4..9d1aa0df04 100644 --- 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); -- cgit