talloc_tos() vs talloc_stackframe()

[Andrew Bartlett]

On Fri, 2018-06-22 at 07:46 +0200, Volker Lendecke wrote:
> On Fri, Jun 22, 2018 at 03:35:22PM +1200, Andrew Bartlett wrote:
> > On Fri, 2018-06-15 at 08:36 +0200, Volker Lendecke via samba-technical
> > wrote:
> > > On Fri, Jun 15, 2018 at 12:10:50AM +0200, David Disseldorp via samba-technical wrote:
> > > > Hi Metze,
> > > > 
> > > > On Thu, 14 Jun 2018 20:54:26 +0200, Stefan Metzmacher wrote:
> > > > 
> > > > > For async code we always have the 'state' variable to use for temporary
> > > > > memory, so we don't need talloc_stackframes...
> > > > 
> > > > Indeed, we just need to encourage people to use it instead of the
> > > > talloc_tos() context, and begin migrating over helper functions :)
> > > 
> > > Do you really want to pass down temporary talloc contexts everywhere?
> > > That's exactly what talloc_tos() was designed for.
> > 
> > Yes, we should.  If memory is returned, it should be on a context.
> 
> Just to clarify: We used to have a pattern where we handed down a
> talloc context for temporary use within a function. This is what I
> object to and want to see it replaced by talloc_tos(). 

I would prefer a new talloc_stackframe(), but yes.

> Once a function
> returns allocated memory, we must always pass down the talloc context
> to put this on. 

Good.

> We must never return memory for later use on top of
> talloc_tos().

I strongly agree.  Can someone please have a look at
source3/smbd/posix_acls.c?  Every function within that file seems to
assume that talloc_tos() memory lasts until some top-level caller, and
returns memory on that context.

In related news, we did find the sysvolreset performance issue, it is
actually the uid_entry_in_group() call hidden in there, re-building the
user's SID list (tokenGroups) for every ACL just to see if the user
might be in a group! 

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