[RFC] Making talloc_reference() safer.

[ronnie sahlberg]

> Also add a warning for talloc_parent() and talloc_parent_name().

Why just a warning and not a run time error?
I think it is almost always a mistake when mixing single-parent
hierarical allocations with a multi-parent graph allocation.
But this is a step in the right direction.


The name of this "talloc_may_reference()" could be changed to
something more explicit, but I dont know exactly what it would be
called.
Maybe rename it to "talloc_make_referenced_ptr()"  to indicate that
what it does is not really
just changing it so that you can, if you want, create references,
but instead indiate more clearly that you MAKE some change to this
object  that hcanges the semantics of it from hereon and forward.


Even better I think would be if you had a new set of creator functions
 where you could set the "no longer hierarical, now its multi-parent
referenced" already from the initial creation. That would be even
better imho  to force the choice of api at object creation time and
not create it as ahierarical object and then later "upgrade" it to a
different model.


I.e. offer something like this
talloc_new_ref(...)
instead of using the pattern
talloc_new(...)
...
talloc_make_reference_ptr(...)


ronnie s




On Mon, Oct 24, 2011 at 5:11 PM, Rusty Russell <rusty at rustcorp.com.au> wrote:
> Hi all,
>
>        git://git.samba.org/rusty/samba.git #talloc-reference-check-wip
>
>        I had the fun of re-arguing talloc_reference() safety with
> Tridge last Thursday.  We agree that talloc_reference fills a real need,
> but we can make it safer by expanding the checks which differentiate
> normal from refcounted objects.
>
> Firstly, note that talloc_reference() has several real use cases:
> anywhere that reference counts would normally be used.  The patterns
> I've seen are:
>
> 1) A "cache" of objects, where the cache may evict objects.  The cache
>   wants to hold a reference, as do the callers, and the object is freed
>   if it's evicted from the cache *and* noone else has referenced it.
>
> 2) A "dealer" of single objects, such as the tdb_wrap code which will
>   keeps track of all tdbs and avoids reopening the same tdb by handing
>   back a referenced tdb.
>
> In both cases, talloc_reference() is appropriate, but like any explicit
> reference counting, it's a bug to simply free the object.  The usability
> problem is that talloc_reference() greatly hides this bug (though less
> than it used to, now talloc_steal and talloc_free are mainly banned).
>
> Thus, the following patches:
>
> 1) Add talloc_may_reference().  This is used to mark an object that may
>   be referenced in future (such as a cache or dealer handing it out the
>   first time).  Turns on all the runtime checks (no talloc_steal, or
>   talloc_free) even if it doesn't *currently* have a reference.  Also
>   add a warning for talloc_parent() and talloc_parent_name().
>
> 2) Then, a global flag to warn if you don'tset talloc_may_reference()
>   before you take a reference.  This means you can't just
>   talloc_reference() on any object.  Since this is an API change, the
>   flag defaults to off.
>
> (Note that the flag infrastructure can be enhanced later to add runtime
> control of things currently ifdefed:
>        - ALWAYS_REALLOC
>        - talloc_steal-to-self warning
>        - talloc_report printing contents)
>
> Thoughts welcome,
> Rusty.
>