talloc quiz, and: dangling talloc references and inconsistencies in the talloc model

[Sam Liddicott]

* tridge wrote, On 23/01/09 02:28:
> Hi Sam,
>
>  > I think this is a good assumption, but I think it is less likely to hold
>  > true in a system of asynchronous modules (like vfs layers) when interest
>  > in structures isn't necessarily nested, and where it is nearly
>  > impossible to test whether or not a particular use will be safe at runtime.
>
> right, but the same will be true of any other rule, just in different
> circumstances.
>
> One thing we could do is make the "cannot talloc_free when a pointer
> has a reference" be dynamically settable. For example, we could have:
>
>   talloc_set_flag(ptr, TALLOC_NO_REFERENCE_FREE);
>
> and have the flag inherited by children. We have plenty of room in the
> flags field of the talloc_chunk structure to hold a flag like this.
>
> The semantics would be that if you set that flag, then talloc_free()
> when there is more than one parent would fail, and you would need
> talloc_unlink().
>
> Cheers, Tridge
>
> PS: Sorry for the slow dribble of replies - I'm at a conference this
> week, then away for 2 weeeks after that. This will be an extended
> conversation :-)
>   
np. This suggestion to set a flag would certainly take away some of the 
pain.

The rule I propose which prevents references becoming owner, but 
preserves owner-less allocations for the lifetime of references will 
behave exactly the same as your original intended behaviour, for those 
talloc operations that don't imply special owner treatment. Thus, as far 
is possible, parents and references are equal.

The differences in behaviour which my proposal introduces are to make 
the effect of talloc operations that depend on the owner be more 
predictable by making the owner more predictable.

I can't see why you are opposed to this other than that you thought that 
you had already covered it.

* tridge wrote, On 23/01/09 03:11:
> Hi Sam,
>
>  > Also, the test I supplied called "test_dangling_loop" shows a bug,
>  > described just above the test definition.
>
> I don't see it as a bug. The test is an extension of a simple
> loop. The simple loop 
>
>
>                              p1
>                             /  \     
>                             |   |
>                             |   |    
>                              \ /
>                              p2
>
> The diagram is supposed to show that p1 is a parent of p2, but p2 is a
> parent of p1. So what do we do if you talloc_free(p1) ? If we follow
> your rules then the free will do nothing. You can't get rid of that
> loop.
>
> So (quite deliberately) I added code so that a talloc_free() on a loop
> like that does free the loop. That seemed to be the only sensible
> thing to do. 
>   

I agree that it is not a bug if that is what you intended.
I also agree that it is probably a quick way out of a problem, but I 
don't think it is a comprehensive solution.

The behaviour that the provided test cases expects is certainly 
reasonable general-case behaviour; it is not generally reasonable that 
dangling references should be introduced to avoid memory leaks even if 
this hasn't yet introduced any detectable error paths in samba.

> ...
>
> I think that is quite sensible. We had a loop, which got removed, but
> one part of the loop had an external reference, and that part of the
> loop got saved, as a child of the pointer that referenced it.
>
>  > I tried to consider if there was any way (without my full blown
>  > solution) ....
>
> What is your proposed full blown solution? I may well have just missed
> it in the various emails. Do you have a working set of code which
> would replace talloc, or a testsuite showing how you think it might be
> used?  That might help me understand what you are pushing for.
>   
The full blown solution is only "full blown" with respect to 
reference-loop detection, it is not intended to replace talloc but to be 
part of it.

It depends on the proposal for a "no owner" context and to stop 
promoting references to owner, and can be described like this:

You can be free'd if you are owned by no_owner and have no references, 
or if this is also true of your owner and/or references

psuedo code:
CAN_FREE(allocation):
if parent==no_owner || CAN_FREE(parent)
and for_each_reference(CAN_FREE(reference))
return true
else return false

(and it will need to use TALLOC_LOOP to catch when allocations are 
parents of each-other, or references refer to each-other)

This means that we don't need to leave dangling references in order to 
break loops, and has the advantage that the complexity of can_free 
depends only on the complexity of the referencing - simple allocations 
can be dealt with simply.

Sam