[QUICK] talloc bugs

tridge at samba.org tridge at samba.org
Wed Jul 1 13:36:50 GMT 2009


Hi Sam,

 > I think they should all be allowed.

you just haven't read the changes carefully enough. Looking just at
the diff and not at the surrounding code paths is not enough.

 > http://git.samba.org/?p=samba.git;a=commitdiff;h=b2c3c08b461042de683b0e49dcaa5f9386c72f9e
 > It should be OK to steal something with references. Steal implicitly
 > works on what I now call a convenience pointer called "parent" that is
 > kept in the talloc chunk. There is nothing ambiguous about that.

That wasn't the problem. Look at the code more carefully and tell me
who was the parent at the point where the talloc_steal() I moved was
called. It turns out that in some cases it was an internal pointer,
and sometimes it was something passed in by a caller! So the result of
the talloc_steal() was completely unpredictable.

I made all these changes with Andrew Bartlett, the author of this
code, sitting next to me. Andrew kept telling me "damn, that's a
bug", and he was the one who worked with me to do the fix. So please
don't tell me that "it's not a bug" based on your cursory reading of
the patch.

... other examples where the same thing applies deleted ...

 > http://git.samba.org/?p=samba.git;a=commitdiff;h=956b5a0003a3ab82d2d7cffb7aee6e5281b4fbb4
 > http://git.samba.org/?p=samba.git;a=commitdiff;h=12510329217dd2b8027794b63258a34797a0f940
 > http://git.samba.org/?p=samba.git;a=commitdiff;h=269b16212a65c9506147db381ecdcbdd58347af6
 > These aren't bugs either, it's just a choice to have import do a steal,
 > implying that the caller of import was/knew the allocator and is giving
 > up ownership.

again, please read the darn code more carefully! These were memory
leaks. In lots and lots of places our python code left two parents on
a pointer but it was only freed once.

 > > Sam, please look at the following bit of code:
 > >
 > > 	int *p1 = talloc_new(NULL);
 > > 	int *p2 = talloc_new(NULL);
 > > 	int *c1 = talloc(p1, int);
 > > 	int *r1 = talloc_reference(p2, c1);
 > > 	talloc_free(p1);
 > >
 > > now tell me what you would have it do. What pointers will remain at
 > > the end of that code, and what parent/child relationship?
 > >   
 > p1==NULL
 > 
 > p2!=NULL // a valid context.
 > 
 > c1==r1 // a valid int* with no tc->parent and one ref
 > talloc_chunk_from_ptr(c1)->parent==no_owner_context
 > talloc_chunk_from_ptr(c1)->refs->ptr==p2

What I was hoping you'd tell me is what you want from the callers
point of view. The internal structure is just that - _internal_. The
fact that it always has a 'parent' pointer is an internal convenience
for code efficiency.

If I have managed to interpret your internal data structure view
correctly then I think what you're giving above is exactly what the
code does now from the point of view of any user of the talloc
API. Right?

 > That difficulty in seeing other sane behaviour is because the
 > distinction "parent" is supposed to be immaterial, but it isn't.

ok, then apart from the talloc_parent() call, exactly how would you
find out if a parent is a 'true' parent or a reference with the
current code?

If it can't be seen externally then why do you care? Is this all
really just about internal re-arrangement of the talloc code, or are
you wanting a different API?

 > Don't leave p2 as "the parent" (requiring promotion) leave it as "a
 > parent" i.e. the reference that it already is. Leave c1 without a "the
 > parent" - it doesn't need one. The reference will keep it alive.

again, that's an internal notational convenience, not part of the API

 > It gives us all these things without breaking the ABI or introducing
 > additional runtime errors or checks, and without additional
 > hard-to-manage API constraints.

The breaking of the ABI has absolutely nothing to do with the change
to refuse ambiguous operations. The ABI broke because I wanted to be
able to give sane debugging messages telling you when a caller is
abusing the API via ambiguous calls. That required making a few
functions into macros so we can get the line of code.

Cheers, Tridge


More information about the samba-technical mailing list