TALLOC: Discussion of destructor processing

[Andrew Bartlett]

On Mon, 2012-04-30 at 13:38 -0400, Stephen Gallagher wrote:
> I was having a discussion with Pavel Březina about talloc destructors
> and he made me aware of the underlying algorithm used to process them
> during a nested free operation.
> 
> So, to summarize quickly, the behavior is as follows: (pseudo-code).
> 
> if (destructor(ctx) == -1) {
>      return -1;
> }
> free_children(ctx); /* This recurses */
> free(ctx);
> 
> 
> I see several problems with this approach.
> 
> 1) The results of the destructors for the children are not considered
> when determining whether to report success or failure for the
> talloc_free(). This means that if any part of the hierarchy except the
> top-most context is freed, there will be a memory leak (with probably
> unreachable memory).

This is an interesting issue.  In general, it is a really bad idea for a
destructor to fail (this isn't made very clear in the docs however). 

> 2) It strikes me as strange that we're calling the destructor before
> we're we're handling the destructors of the children. It's contrary to
> (at least my) expectations about the order that destructors should
> execute. Freeing of memory and execution of destructors should be
> happening from the "bottom-up" of the hierarchy.

If we did it in that order, then a top level destructor could not rely
on the contents of the structures that it may be accessing.  This is
because typically the talloc tree follows the pointer tree, and many of
the elements in a structure with a destructor are allocated (and some of
those allocations might be with malloc/etc and a destructor).

> My vision of how this should behave is as follows: We should iterate
> through the hierarchy tree, starting from the leaves. Work its way back
> up, calling destructors where they exist until either we have called all
> of them or any one has failed. If any failed, return -1 to the top-level
> free. If all succeeded, THEN iterate the tree a second time to actually
> perform the deallocation.

Given that talloc_free() is rarely checked for errors, wouldn't this
leave even more memory potentially leaked, as any destructor anywhere
could abort the free of an entire tree?

This area is very much worth thinking about - much of what talloc does
is just a little bit tricky, and it has taken time to work out if the
way it works is really the best way in the long term.  What I mention
above is just some counter-points to help you think over the complexity
of the issue.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org