From: Sam Liddicott Date: Fri, 10 Jul 2009 10:35:16 +0000 (+0100) Subject: prevent reference promotion to owner X-Git-Url: http://git.samba.org/?p=metze%2Fsamba%2Fwip.git;a=commitdiff_plain;h=95c37860b2d58c34717139f70d0632e1165451f4 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 --- diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c index d5b0c3bfe532..140da5304767 100644 --- 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;