talloc issues

[Michael Adam]

Hi Tridge,

tridge at samba.org wrote:
> 
>  > 2. With Tridge's changes, all users of talloc_free() need to
>  >    be reviewed. Those that conciously want to free a reference
>  >    need changing, but those callers that use  talloc_free()
>  >    on simply allocated talloc contexts are potentially broken
>  >    too, because they need to know whether there are references,
>  >    so they need changing, too.
>  > 
>  > 3. With the changes proposed by Sam and Metze, the "naive" users
>  >    of talloc/talloc_free are ok as they are. But the users of
>  >    references that want to free the reference need to be changed.
>  > 
>  > Have I misunderstood anything basic here?
> 
> yes, I think you have. The cases 2 and 3 above are reversed :-)

Sorry for being ignorant, but I don't see why:

* If I understood your change correctly, your patch changed talloc_free()
  to fails when called on a pointer with more than one
  parent, i.e. with references.

  This does mean that it breaks the basic use of
  talloc()/talloc_free() when someone takes references inbetween.
  This means that all callers of talloc_free() need to be reviewed
  and possibly changed to something else. Doesn't it?

* On the other hand side, with the change Metze proposed,
  talloc_free() will succeed if called on the original parent, so
  the basic users can be kept as they are. But the callers of
  talloc_free() that call it with a reference, must be changed to
  use talloc_unreference() instead.

This is what I meant above. What is my misconception here?

> The point of my changes was that the review would happen automatically
> and be absolutely accurate as we are catching all the cases where
> there are problems using the logic in the code. The only places where
> semantics change are all caught by the warning I added. So if no
> warning is produced then we know the code is doing something that is
> OK.

This is of course a brilliant technical realization.
But couldn't the same technique be applied to Sam's/Metze's
approach?

> For Sam's proposed changes there are no such guarantees. All code
> needs to be manually reviewed, even the ones that seem naive on first
> glance.

When voting in favour of their approach I was not referring to
the complete implementation including aborts and so on, but
the conceptual change to the behaviour of talloc_free() and
references.

> When I added the warning to my changes it took Andrew and me
> quite a while to even find the cause of the warning in some cases, as
> they were spread across multiple files. Some of them looked "obviously
> correct" but they weren't.
> 
> Manual inspection of massive code bases does not work for anything but
> the simplest cases. This is not a simple case.

I agree 100%.

>  > With Tridge's changes, we need to change even more callers than
>  > with the Sam/Metze approach.
> 
> no. I believe we have already identified all the cases we need to
> change for the patches I've put in. There may be some more lurking,
> but if there are we will find them as soon as they trigger.

But this does not seem to be robust to me:
When a user of talloc_free() is safe now, it may fail in the
future, when an implementation of a function called changes
to add references to the context. But IMHO this should not matter.

The discussion of the basic design continues in other mails,
so I stop here now.

Tridge: Sorry for insisting. I am reviving this discussion
again and again now, because I want to understand, and I think
we need to come to a solution that is satisfactory
from the existing and future users point of view as well as
regarding the design of talloc. And therefore, I think we
need to find a solution on this higher level before loosing
ourselves in techincal details.

Cheers - Michael

> With the changes proposed by Sam we have no idea how many there are,
> and we have no way of measuring whether we've found them.
> 
> Sam's changes reverse the sense of talloc_free on a pointer with
> references, with no warning. My changes ban the use of talloc_free on
> references, with a warning.
> 
>  > But the way in which the code reacts to the error is not implied
>  > by the design that the respective approach implements.
>  > We could well change metze's code to thow the same sort of
>  > error messages instead of aborting. Just like yours.
> 
> If that is the proposal then I'll look at it. As it is, the patches
> reverse existing behaviour with no warning. Sam has been pushing this
> as one of the advantages of his patch as he proposes it fixes things
> with no pain. That is not true.
> 
>  > I still think that the benefit of the sam/metze aproach is that
>  > the basic users need no change. And the users of talloc_reference
>  > need to be audited and changed.
> 
> It is a 'basic user' that does the dereference of freed memory in the
> example I posted. The basic user does not know that someone else has
> taken a reference deep in a library routine.
> 
> Cheers, Tridge
-------------- 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/dc57fee6/attachment.pgp>