talloc issues

[tridge at samba.org]

Hi Michael,

 > 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 :-)

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.

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 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.

 > 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.

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