BUG: talloc reads freed memory.

Rusty Russell rusty at rustcorp.com.au
Mon Dec 20 20:26:35 MST 2010

Not sure how to fix this one; valgrind with my new failtest module
caught it in CCAN:

	root = talloc_new(NULL);
	p1 = talloc_strdup(root, "foo");

The problem is here in _talloc_free_internal:

	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;

Note that we leave tc->prev set, even though we've removed ourselves
from the list.

	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);
		if (unlikely(_talloc_free_internal(child, location) == -1)) {

The child frees the reference, which is our sibling (ie. tc->prev).

			if (new_parent == null_context) {
				struct talloc_chunk *p = talloc_parent_chunk(ptr);

talloc_parent_chunk follows tc->prev through the (freed) reference.

				if (p) new_parent = TC_PTR_FROM_CHUNK(p);
			_talloc_steal_internal(new_parent, child);

Now, simply clearing tc->prev (and tc->next) just breaks this reparenting
logic.  Getting the parent earlier in case the destructor fails is
O(siblings) and we'd be better off just keeping all the parent pointers
uptodate (ie. make talloc_steal() O(siblings)).

PS.  I haven't actually verified this with the current SAMBA talloc, but
     the code is basically unchanged.

More information about the samba-technical mailing list