[Patches] Preparation for tevent impersonation (part1)

[David Disseldorp]

On Fri, 15 Jun 2018 09:25:08 +0200, Volker Lendecke wrote:

> On Thu, Jun 14, 2018 at 11:54:50PM +0200, David Disseldorp wrote:
> > On Thu, 14 Jun 2018 20:37:37 +0200, Volker Lendecke wrote:
> >   
> > > > Talloc stackframe handling is broken in many of places, unfortunately.
> > > > Continuing to use it for event based async code will only lead to more
> > > > problems in future IMO.    
> > > 
> > > What do you propose instead?  
> > 
> > In most async cases we already have a memory context for the request
> > state, which makes a perfectly adequate parent for short-lived
> > tallocations. It may mean passing mem_ctx arguments around a bit more,
> > but is IMO far less error prone and easier to follow compared to random
> > usage of the talloc_tos() global and subsequent garbage collection.  
> 
> Manually passing down temporary contexts is just as error-prone: It's
> too easy to accidentially pass down a long-term context where a
> short-lived one is needed. For me talloc_tos() is a clear indication
> that a short-lived one is requested. The guarantees for talloc_tos()
> are that it lives as long as the current function runs and that it
> will be cleaned up "quickly" after the function returned. Clear and
> concise, at least to me.

I disagree. For short-lived heap-backed allocations, creating a new
talloc child under a longer-term context and subsequently freeing it
allows one to easily determine the lifecycle of the variable. This is
not the case for talloc_tos() allocations without analysing the state of
the talloc stackframe through the entire call chain - "quickly" is
completely arbitrary.
A function returning memory allocated on the current stackframe without
taking a talloc_tos() parameter is evil IMO, as it's completely unclear
from the caller where the memory came from.

> I haven't followed precisely, but f4f3abfa0e18bb just committed might
> be a good example of what can go wrong if you explicitly pass around
> meant-to-be-short-lived contexts explicitly.

Short of moving to Rust or Go, I don't think we'll ever be completely
free of memory leaks. talloc_tos() does help reduce the impact of some
memory leaks, but the drawbacks far outweigh the benefits IMO.

> If talloc_tos() is no longer en vogue, we need a similarly concise
> abstraction.

In talloc we already have a very nice hierarchical allocator which,
alongside the stack, is perfectly capable of providing short lived
memory allocations.

Cheers, David