hmm.. Re: talloc issues

[Michael Adam]

Volker Lendecke wrote:
> On Tue, Jul 28, 2009 at 04:20:39PM +0100, Sam Liddicott wrote:
> > > Sorry, I'm lost here. If talloc_free is deprecated, 
> > 
> > It's not.
> > 
> > It's just been used so much on purpose to "get rid of random latest
> > parent" and these uses need to be removed.
> 
> I don't even fully understand what you mean by this.

If I get him right, Sam is saying here that the old implementation of
talloc_free() was broken when there were references around and that
many users of talloc_reference() were using this brokenness by calling
talloc_free() when they should have called talloc_unreference().


> So I am not able to do this code audit. To me talloc is
> strictly hierarchical and there is no such thing as "random
> latest parent". Having to think about possible security
> holes or memory leaks by calling talloc_free (How weird is
> that. Creating a memory leak by calling talloc_FREE(!!) in a
> subtly wrong context) makes it unusable to me.

Ok, Volker brought to the point the thing that I tried
to claim as the necessary state of affairs that we should
try to achieve as soon as possible.

Let me rephrase it once more.

And yes, I am again taking the high level point of view.
If we meet technical obstacles and required changes of
existing code using talloc, then so be it.

This is my basic axiom:

Axiom:

   The basic use of talloc is the talloc()/talloc_free() pair
   of calls. It must always be possible and safe to use,
   irrespective of other features of talloc.

This means that you should always be able to talloc_free()
a pointer that you talloc()'ed yourself, no matter
what the functions you gave the pointer to did with
it (take references and whatnot).

Corollary:

   It is not an option to make talloc_free() fail if
   someone has e.g. taken references of a pointer
   we talloc()'ed.

Corollary:

   The only way to honour the Axiom and live with Tridge's change
   to talloc_free() is to always talloc_unreference() a
   talloc_referenced() object before freeing the original parent
   context, i.e. usually before returning from the function that
   creates references
   (or more easily: to not use talloc_reference() ever).

Initially I claimed (as Volker and Jeremy just did),
that all other stuff like talloc_reference() that complicates
things and introduces such bugs and troubles, should be removed.

I still can see no real necessity for talloc_reference(),
since Samba3 can basically live without, but it may of course
be a nice feature if implemented properly.
And there is a huge user community of talloc_reference()
in the Samba 4 code, and possibly we have to keep it for this
reason if not for others.

But if we really want talloc_reference(), we must make
sure that it does not make the use of the basic scheme more
dangerous or more difficult. It is really not an option
to require the basic users to have knowledge about other
games the functions they call might play with the given
talloc contexts. This would just completely contradict my
undestanding of good design, since the implementation of
the called functions might change.

Corollary:

   More advanced features may not impose the need for
   knowledge about the implementation of the users
   of talloc()'ed memory on the basic users.

So, it is not acceptable that a fix to the brokenness of
talloc_free with references requires to change users of
the basic talloc scheme -- it is the users of talloc_reference()
that need changing: They need to use talloc_unreference()
instead of a talloc_free() which is broken for use with
references.

Hence a good way to fix this is to make talloc_free()
fail not for pointers with references but when called
on the references itself. And consequently for
talloc_free() -- when called on the original parent -- to
remove the parent and only keep the memory alive as
long as there are references.
(This is basically the Sam/Metze approach).

from my point of view, it also not an option to _not_ make a
good and correct change, just because there are many users
of the broken code that relied on the brokenness.

So if we really want to fix the issue, let's do it properly,
and if it is hard to fix all the users of talloc_reference()
in Samba 4, then so be it.

This is my naive understanding of the things.
Please excuse my boldness.
I am more than willing to participate in fixing
callers should we come to a satisfactory solution.

Cheers - Michael

> Talloc_reference needs to go. It might be useful in your
> context, but for mere mortals like me this is just not
> acceptable. In projects with more complexity than Samba 3
> has and where all the smart cookies do programming this
> might be common practice, but it is way over my head.
> 
> Volker

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20090729/f8481576/attachment.pgp>