talloc_tos() vs talloc_stackframe()

[Andrew Bartlett]

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.

The problem with silently allocating memory on talloc_tos() is 'how
long is this valid for?'.  

At the time it is written, it might be valid quite a way up the C
stack, but then adding in a talloc_stackframe() at any point then
invalidates the assumptions. 

As you know, and we have had 'words' about this before, I dislike
talloc_tos() for this reason, and because more broadly across the
codebase it is not universally true that there is an outer stackframe. 

(While historically hesitant, I do use talloc_stackframe() where
appropriate in new code for talloc memory needed within a function). 

I see that create_conn_struct_tos() was recently pushed though more of
the code, but I'm strongly of the view that this is less clear and the
wrong direction.  It might be better than create_conn_struct() for
other reasons, but I really did think we were moving away from this
implicit memory allocation. 

Likewise, the recent 'pysmbd: remove explicit talloc_stackframe() from
get_conn() and name it get_conn_tos()' just seems wrong to me. 

On the flip side, the approach taken in the recent patch 'pysmbd: fix
some talloc_stackframe() memory leaks and clean up the frame hierarchy
in make_simple_acl().' is the right way.  I'm sorry for the original
bug, but I agree with the fix, provide a memory context and use it. 

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