[PATCH] Improve talloc security
abartlet at samba.org
Mon Feb 23 19:31:54 MST 2015
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
> 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).
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
More information about the samba-technical