[PATCH] Improve talloc security

[Andrew Bartlett]

On Mon, 2015-02-23 at 18:24 -0800, Jeremy Allison wrote:
> On Tue, Feb 24, 2015 at 03:03:34PM +1300, Andrew Bartlett wrote:
> > On Mon, 2015-02-23 at 17:30 -0800, Jeremy Allison wrote:
> > > 
> > > Problem with this I can see is it makes talloc *more* MT-unsafe
> > > when used without the TALLOC_FREE_FILL environment variable,
> > > and without talloc_enable_null_tracking().
> > > 
> > > I know it's a small race condition on initialization but
> > > eventually all these things add up.
> > 
> > Indeed, I can see this might be an issue.  libbsd hit the same thing
> > here trying to play around with the global environ[] to implement
> > setproctitle:
> > 
> > http://cgit.freedesktop.org/libbsd/commit/?id=c5b959028734ca2281250c85773d9b5e1d259bc8
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=66679
> > 
> > In short, we can already be in threads when we are in the library init,
> > if we are invoke for the first time by dlopen(). 
> > 
> > But the above is racing against other parts of the system that can see
> > and use environ[].  What I don't really see is how we get init() to run
> > twice, because dlopen() will only load a library once (otherwise we
> > would be in bigger trouble).  
> > 
> > In any case, dropping the rand() patch would avoid that part of the
> > issue, as it would be deterministic.  I still don't see the race, but I
> > understand why it raised red flags for you.
> 
> Never mind, I initially missed we're in the contructor
> when that gets run (obviously completely ignored the
> first patch which explicitly checks the platform supports
> running constructors :-) which should mean we're safe there.

I currently have a fallback for the non-constructor case, but I'm happy
to remove it, and make it either fail to build or not support this
feature instead. 

> In fact it might be better to move the TALLOC_FREE_FILL
> code also into a library constructor (if we're adding
> one) in order to remove one of the races we already
> have with MT-code.

That sounds like a good idea.  It also removes a branch from the hot
path.  (talloc_free() is a really, really hot path). 

Andrew Bartlett

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba