talloc issues

[Michael Adam]

Hi Tridge,
and Sam, Metze, Simo, ...

tridge at samba.org wrote:
> Hi Michael,
> 
>  > I am deliberately not going into technical details
>  > since I wanted to emphasize this superficial higher level
>  > point of view of the callers.
> 
> The technical details really do matter.

Sure they do.

I was thinking about what the proper design would be (IMHO)
without thinking about existing users.

> As the example I gave shows, the proposals from Sam can create
> security holes in existing code that followed the API
> documentation as given. Regardless of the high level
> aspirations of the proposed changes we can't just ignore this.

Ok, that is of course not good at all and should not be ignored.

But let me return to the higher level view for a bit.
I think we still need to clarify things more generally.

We are in principle facing the following situation:

1. Prior to Tridge's changes, talloc code was broken by design
   with respect to references. So we can say, yes the callers
   correctly used talloc as described told by the API docs.

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?

So whatever solution we finally take (2. or 3.), the change that
fixes the internal workings of talloc, will change the API
documentation and hence potentially break code using this.

With Tridge's changes, we need to change even more callers than
with the Sam/Metze approach. The point I was trying to make in my
previous email was that I think that it is _very_ important
to keep at least the simple "talloc()/talloc_free()" use scheme
as it is. This should really be safe to use, and the caller
should not need to use more complicated calls or even need
to know about how the funcions it calls act on the memory it
has allocated.

> Auditing the entire code base for such a change is an enormous
> task, and it isn't a task that anyone has volunteered to do. 

I will gladly help out in auditing (parts of) the code.

> I also don't think a code inspection audit will be sufficient - we
> need a programmatic way of finding all existing use cases that are
> deprecated.
> 
> That is why I added the location variable to the talloc reference
> structure, so that when code hit a situation that has changed we get a
> very clear message that allows for a quick fix. With the changes
> proposed by Sam and the patches in Metze's tree we would instead
> either get an abort() or we would get code dereferencing memory that
> has been freed. Both are not good outcomes.

Definitely not.

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.

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.

So the general path to proceed should be the following:

1. decide which design we want to implement in principle
2. implement it with error messages like Tridge, already did.
3. audit all the callers, changing deprecared ones to the new
   scheme
4. once all callers are fixed, we can even think about adding
   aborts -- _after_ those helpful error messages
5. we should then (IMHO) re-audit the code to reduce the use of
   talloc_references.

Cheers - Michael
-------------- 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/20090728/83f4aaae/attachment.pgp>