talloc_tos() vs NULL and threads

[Andrew Bartlett]

On Thu, 2014-04-03 at 09:34 -0700, Jeremy Allison wrote:
> On Thu, Apr 03, 2014 at 01:45:47PM +0200, David Disseldorp wrote:
> > On Wed, 2 Apr 2014 10:18:10 +0200, Michael Adam wrote:
> > 
> > > > > No, I'm not willing to add any more talloc_tos() to this area of the
> > > > > code.  Almost all the odd unexpected failures caused by this patch set
> > > > > were due to new talloc_tos() calls, because not all callers had a
> > > > > talloc_stackframe().    
> > > 
> > > Well, those is a bug then, and the developer mode to panic
> > > in that case was introduced so that we can fix the callers up.
> > > 
> > > I think adding talloc_tos() is way better than adding talloc(NULL,...)
> > > since this way we can at least easily spot the leaks.
> > 
> > I disagree. IMO tallocations on the null context are much more readable
> > and debugable. For talloc_tos() tallocations one needs to consider
> > whether a stackframe is around, and when the next garbage collection
> > could take place.
> 
> NULL talloc context allocations are utterly thread-unsafe,
> that's my problem with them.
> 
> Now I know we don't do much with threads, but we do need
> to get there eventually.. :-).

It's not nearly as clear cut as that.  talloc_stackframe() it totally
thread-unsafe, until the caller provides the thread hooks.  This has
bitten our openchange/evolution-mapi users in real life (who where told
to put Samba into a single thread). 

talloc() on NULL is thread safe until NULL tracking is enabled.

What would, I think, be a reliable solution that can co-exist with
threads is to move from talloc_tos() to talloc_stackframe(), with an
additional feature.  talloc_stackframe() would behave like talloc_init()
until the thread safety hooks are called.  

That would retain the memory leak protections in the main smbd where we
really need them, and allow the use of the talloc pool for faster
per-request allocations, and be thread and memory leak safe in
libraries, without the risk of missing having a talloc_stackframe() in a
library call path.  

(In developer mode, we assert that each talloc_stackframe() is cleaned
up in order, so we have quite a high degree of confidence that we clean
up all stackframe memory, with or without actually using the stack). 

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