talloc: talloc_free_children vs. _talloc_free_internal and memory leaks

[Rusty Russell]

On Sat, 09 Apr 2011 12:09:43 +0200, "Stefan (metze) Metzmacher" <metze at samba.org> wrote:
> Hi,
> 
> I found that talloc_free_children() used to have the same logic as
> _talloc_free_internal(),
> but some fixes went only into _talloc_free_internal().
> 
> That's why I created a  _talloc_free_children_internal() function.
> 
> I think using the TALLOC_FLAG_LOOP detection for the talloc_free_children()
> should not be a problem, if it is we can move it to the caller.
> 
> I got to it because of a memory leak in test_talloc_free_in_destructor().
> There we create a parent child loop and as result the memory chunks
> went out of the scope of the null_context, which means it's hard to detect
> the leak (but valgrind did :-).

OK, first off I think this commit needs a much more verbose explanation:

===
talloc: demonstrate memory leak in test_talloc_free_in_destructor()

The problem is that level3 disappears, it's not reachable from
the null_context anymore. level3 and level5 get into a parent child loop.

I think the correct fix should be that level0 becomes the parent
of level3 after talloc_free(level1).
===

This stuff is subtle, and I want to make sure that I understand exactly
what's going on, and that when we look back at it, I can understand
exactly what we fixed!

Both your fix commits need much more commentry too; I can't understand
what they're doing from reading the patches :(

Why does freeing something in a destructor cause a loop?  Are we
reparenting onto something we are freeing?

Confused,
Rusty.