On talloc_new(NULL) (ab)use in SSSD

[Simo Sorce]

I've always had a beef with the use of this construct in recent sssd
code:

fn()
{
	tmp_ctx = talloc_new(NULL);

	use tmp_ctx ...
	steal stuff on mem_ctx;

	talloc_free(tmp_ctx);
	return
}

The main reason this style was adopted is the claim that using
allocation on NULL makes it easier to valgrind stuff and find leaks, and
that leaks are easier to spot than huge memory growths.

I agree that this code makes that specific task easier, but it comes at
a cost, a cost we always bear while instead this code is useufl only
once in a while when we run valgrind.

It makes memory leaks much more dangerous, because if you forget to free
tmp_ctx it really is leaked
It requires a malloc() for even tiny functions and the context on it's
own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
Malloc is not free and can be quite expensive when some library tricks
glibc to go in paranoid threadsafe mode (which is surprisingly easy to
do) and mutexes start to be used for every allocation/free.


I'd like to propose a compromise going forward where we use a couple of
macros to avoid the cost of a temp context when not necessary.

These are the macros to me called when we enter and exit the functions:

SSS_MEM_CTX(loc_ctx, main_ctx) do { \
    if (debug_level & SSSDBG_TRACE_INTERNAL) { \
	loc_ctx = talloc_new(NULL); \
    } else { \
	loc_ctx = main_ctx; \
    } \
} while(0)

SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
    if (loc_ctx != main_ctx) talloc_free(loc_ctx)

Basically what we do here is that if we set debug level to
SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
otherwise we use the passed in memory context and save the overhead.

This is just to avoid the useless overhead and avoid minor mem leaks in
actual production when they happen (as the memory will eventually be
freed by the main context) but still allow for them to be foiund easily
with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
when running under valgrind.

IF the function needs a temporary memory context because it handles a
lot of memory allocations for stuff it need not return it would *Still*
use a local tmp_ctx, but it would be something like a local structure
then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
NULL! ].

You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
they coincide.

So this would be an example:

CURRENT STYLE:
        char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
        {
            TALLOC_CTX *tmp_ctx;
            char *bar;
        
            tmp_ctx = talloc_new(NULL);
        
            bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
        
            talloc_steal(mem_ctx, bar);
        
            talloc_free(tmp_ctx);
            return bar;
        }
        
NEW STYLE:
        char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
        {
        	TALLOC_CTX *loc_ctx;
        	char *bar;
        
        	SSS_MEM_CTX(loc_ctx, mem_ctx);
        
        	bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
        
        	talloc_steal(mem_ctx, bar);
        
        	SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
        	return bar;
        }


Discuss.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Stephen Gallagher]

On Mon 07 Jan 2013 11:47:21 AM EST, Simo Sorce wrote:
>  I've always had a beef with the use of this construct in recent
> sssd
>  code:
>
>  fn()
>  {
>  	tmp_ctx = talloc_new(NULL);
>
>  	use tmp_ctx ...
>  	steal stuff on mem_ctx;
>
>  	talloc_free(tmp_ctx);
>  	return
>  }
>
>  The main reason this style was adopted is the claim that using
>  allocation on NULL makes it easier to valgrind stuff and find leaks, and
>  that leaks are easier to spot than huge memory growths.
>
>  I agree that this code makes that specific task easier, but it comes at
>  a cost, a cost we always bear while instead this code is useufl only
>  once in a while when we run valgrind.
>
>  It makes memory leaks much more dangerous, because if you forget to free
>  tmp_ctx it really is leaked
>  It requires a malloc() for even tiny functions and the context on it's
>  own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
>  Malloc is not free and can be quite expensive when some library tricks
>  glibc to go in paranoid threadsafe mode (which is surprisingly easy to
>  do) and mutexes start to be used for every allocation/free.
>
>
>  I'd like to propose a compromise going forward where we use a couple of
>  macros to avoid the cost of a temp context when not necessary.
>
>  These are the macros to me called when we enter and exit the functions:
>
>  SSS_MEM_CTX(loc_ctx, main_ctx) do { \
>       if (debug_level & SSSDBG_TRACE_INTERNAL) { \
>  	loc_ctx = talloc_new(NULL); \
>       } else { \
>  	loc_ctx = main_ctx; \
>       } \
>  } while(0)
>
>  SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
>       if (loc_ctx != main_ctx) talloc_free(loc_ctx)
>
>  Basically what we do here is that if we set debug level to
>  SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
>  otherwise we use the passed in memory context and save the overhead.
>
>  This is just to avoid the useless overhead and avoid minor mem leaks in
>  actual production when they happen (as the memory will eventually be
>  freed by the main context) but still allow for them to be foiund easily
>  with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
>  when running under valgrind.
>
>  IF the function needs a temporary memory context because it handles a
>  lot of memory allocations for stuff it need not return it would *Still*
>  use a local tmp_ctx, but it would be something like a local structure
>  then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
>  NULL! ].
>
>  You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
>  they coincide.
>
>  So this would be an example:
>
>  CURRENT STYLE:
>           char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>           {
>               TALLOC_CTX *tmp_ctx;
>               char *bar;
>
>               tmp_ctx = talloc_new(NULL);
>
>               bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>
>               talloc_steal(mem_ctx, bar);
>
>               talloc_free(tmp_ctx);
>               return bar;
>           }
>
>  NEW STYLE:
>           char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>           {
>           	TALLOC_CTX *loc_ctx;
>           	char *bar;
>
>           	SSS_MEM_CTX(loc_ctx, mem_ctx);
>
>           	bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>
>           	talloc_steal(mem_ctx, bar);
>
>           	SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
>           	return bar;
>           }
>
>
>  Discuss.
>
>  Simo.
>  
Well, the other thing this construct introduces that I'm not sure I 
like very much is that it requires a higher memory context to be passed 
into every function. For functions that aren't returning 
explicitly-requested allocated memory, this doesn't seem right.

If a function creates memory for its own purposes that are not of 
interest to the caller, it should NOT be allocating that memory on a 
parent context. I suppose we can just opt to use SSS_MEM_CTX(loc_ctx, 
NULL) explicitly in these cases, but that's really just collapsing this 
behavior down to the current state.

I don't really like passing nonsensical arguments to a function just to 
*work around* sloppy coding.

[Simo Sorce]

On Mon, 2013-01-07 at 13:04 -0500, Stephen Gallagher wrote:
>  On Mon 07 Jan 2013 11:47:21 AM EST, Simo Sorce wrote:
>  > I've always had a beef with the use of this construct in recent sssd
>  > code:
>  >
>  > fn()
>  > {
>  > 	tmp_ctx = talloc_new(NULL);
>  >
>  > 	use tmp_ctx ...
>  > 	steal stuff on mem_ctx;
>  >
>  > 	talloc_free(tmp_ctx);
>  > 	return
>  > }
>  >
>  > The main reason this style was adopted is the claim that using
>  > allocation on NULL makes it easier to valgrind stuff and find leaks, and
>  > that leaks are easier to spot than huge memory growths.
>  >
>  > I agree that this code makes that specific task easier, but it comes at
>  > a cost, a cost we always bear while instead this code is useufl only
>  > once in a while when we run valgrind.
>  >
>  > It makes memory leaks much more dangerous, because if you forget to free
>  > tmp_ctx it really is leaked
>  > It requires a malloc() for even tiny functions and the context on it's
>  > own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
>  > Malloc is not free and can be quite expensive when some library tricks
>  > glibc to go in paranoid threadsafe mode (which is surprisingly easy to
>  > do) and mutexes start to be used for every allocation/free.
>  >
>  >
>  > I'd like to propose a compromise going forward where we use a couple of
>  > macros to avoid the cost of a temp context when not necessary.
>  >
>  > These are the macros to me called when we enter and exit the functions:
>  >
>  > SSS_MEM_CTX(loc_ctx, main_ctx) do { \
>  >      if (debug_level & SSSDBG_TRACE_INTERNAL) { \
>  > 	loc_ctx = talloc_new(NULL); \
>  >      } else { \
>  > 	loc_ctx = main_ctx; \
>  >      } \
>  > } while(0)
>  >
>  > SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
>  >      if (loc_ctx != main_ctx) talloc_free(loc_ctx)
>  >
>  > Basically what we do here is that if we set debug level to
>  > SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
>  > otherwise we use the passed in memory context and save the overhead.
>  >
>  > This is just to avoid the useless overhead and avoid minor mem leaks in
>  > actual production when they happen (as the memory will eventually be
>  > freed by the main context) but still allow for them to be foiund easily
>  > with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
>  > when running under valgrind.
>  >
>  > IF the function needs a temporary memory context because it handles a
>  > lot of memory allocations for stuff it need not return it would *Still*
>  > use a local tmp_ctx, but it would be something like a local structure
>  > then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
>  > NULL! ].
>  >
>  > You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
>  > they coincide.
>  >
>  > So this would be an example:
>  >
>  > CURRENT STYLE:
>  >          char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>  >          {
>  >              TALLOC_CTX *tmp_ctx;
>  >              char *bar;
>  >
>  >              tmp_ctx = talloc_new(NULL);
>  >
>  >              bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>  >
>  >              talloc_steal(mem_ctx, bar);
>  >
>  >              talloc_free(tmp_ctx);
>  >              return bar;
>  >          }
>  >
>  > NEW STYLE:
>  >          char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>  >          {
>  >          	TALLOC_CTX *loc_ctx;
>  >          	char *bar;
>  >
>  >          	SSS_MEM_CTX(loc_ctx, mem_ctx);
>  >
>  >          	bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>  >
>  >          	talloc_steal(mem_ctx, bar);
>  >
>  >          	SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
>  >          	return bar;
>  >          }
>  >
>  >
>  > Discuss.
>  >
>  > Simo.
>  >
>  
>  Well, the other thing this construct introduces that I'm not sure I 
>  like very much is that it requires a higher memory context to be passed 
>  into every function. For functions that aren't returning 
>  explicitly-requested allocated memory, this doesn't seem right. 
Those functions need not be changed if not necessary, I think passing in
a memory context is a very small price to pay and allows better memory
management.

Initially I was thinking of using that as a method to change the style
for the better and let caller handle the context (wheteher to make a
temp one or not etc...) but it would have made some of the code be
really ugly, so it wouldn't have be 'better' at all :-).

>  If a function creates memory for its own purposes that are not of 
>  interest to the caller, it should NOT be allocating that memory on a 
>  parent context. 
Why not, please justify your *should NOT*, this is how talloc has been
designed, to make it simpler to handle temporary memory in a
hierarchical set of functions. Relations do not need to be strictly a
structure or a function, it can be a tree of functions as well.

>   I suppose we can just opt to use SSS_MEM_CTX(loc_ctx, 
>  NULL) explicitly in these cases, but that's really just collapsing this 
>  behavior down to the current state. 
NO, actually I think I will change the macro to *assert* if main_ctx is
NULL.

The purpose of this function is to *NOT* alloc on NULL.

>  I don't really like passing nonsensical arguments to a function
> just to 
>  *work around* sloppy coding. 
I can also arrange this macro differently so that it looks like:

loc_ctx = SSS_MEM_CTX(mem_ctx)

But then we'd have to not use ';' at then end which is almost always
going to be added.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

On Mon, Jan 07, 2013 at 11:47:21AM -0500, Simo Sorce wrote:
>  I've always had a beef with the use of this construct in recent
> sssd
>  code:
>  
>  fn()
>  {
>  	tmp_ctx = talloc_new(NULL);
>  
>  	use tmp_ctx ...
>  	steal stuff on mem_ctx;
>  
>  	talloc_free(tmp_ctx);
>  	return
>  }
>  
>  The main reason this style was adopted is the claim that using
>  allocation on NULL makes it easier to valgrind stuff and find leaks, and
>  that leaks are easier to spot than huge memory growths.
>  
>  I agree that this code makes that specific task easier, but it comes at
>  a cost, a cost we always bear while instead this code is useufl only
>  once in a while when we run valgrind.
>  
>  It makes memory leaks much more dangerous, because if you forget to free
>  tmp_ctx it really is leaked
>  It requires a malloc() for even tiny functions and the context on it's
>  own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
>  Malloc is not free and can be quite expensive when some library tricks
>  glibc to go in paranoid threadsafe mode (which is surprisingly easy to
>  do) and mutexes start to be used for every allocation/free.
>  
>  
>  I'd like to propose a compromise going forward where we use a couple of
>  macros to avoid the cost of a temp context when not necessary.
>  
>  These are the macros to me called when we enter and exit the functions:
>  
>  SSS_MEM_CTX(loc_ctx, main_ctx) do { \
>      if (debug_level & SSSDBG_TRACE_INTERNAL) { \
>  	loc_ctx = talloc_new(NULL); \
>      } else { \
>  	loc_ctx = main_ctx; \
>      } \
>  } while(0)
>  
>  SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
>      if (loc_ctx != main_ctx) talloc_free(loc_ctx)
>  
>  Basically what we do here is that if we set debug level to
>  SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
>  otherwise we use the passed in memory context and save the overhead.
>  
>  This is just to avoid the useless overhead and avoid minor mem leaks in
>  actual production when they happen (as the memory will eventually be
>  freed by the main context) but still allow for them to be foiund easily
>  with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
>  when running under valgrind.
>  
>  IF the function needs a temporary memory context because it handles a
>  lot of memory allocations for stuff it need not return it would *Still*
>  use a local tmp_ctx, but it would be something like a local structure
>  then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
>  NULL! ].
>  
>  You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
>  they coincide.
>  
>  So this would be an example:
>  
>  CURRENT STYLE:
>          char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>          {
>              TALLOC_CTX *tmp_ctx;
>              char *bar;
>          
>              tmp_ctx = talloc_new(NULL);
>          
>              bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>          
>              talloc_steal(mem_ctx, bar);
>          
>              talloc_free(tmp_ctx);
>              return bar;
>          }
>          
>  NEW STYLE:
>          char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>          {
>          	TALLOC_CTX *loc_ctx;
>          	char *bar;
>          
>          	SSS_MEM_CTX(loc_ctx, mem_ctx);
>          
>          	bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>          
>          	talloc_steal(mem_ctx, bar);
>          
>          	SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
>          	return bar;
>          }
>  
>  
>  Discuss.
>  
>  Simo. 
One concern I expressed to Simo on IRC is that we'd have to be more
careful with long-lived requests such as resolving deep nested group
memberships, otherwise the parent context will grow out of control.

If SSS_FREE_MEM_CTX was used rigorously, then it would be much less of
an issue. Maybe we could write a small GCC plugin to issue a warning for
functions that call SSS_MEM_CTX but not SSS_FREE_MEM_CTX.

[Stephen Gallagher]

On Mon 07 Jan 2013 02:06:45 PM EST, Jakub Hrozek wrote:
>  On Mon, Jan 07, 2013 at 11:47:21AM -0500, Simo Sorce wrote:
> > I've always had a beef with the use of this construct in recent sssd
> > code:
> >
> > fn()
> > {
> > 	tmp_ctx = talloc_new(NULL);
> >
> > 	use tmp_ctx ...
> > 	steal stuff on mem_ctx;
> >
> > 	talloc_free(tmp_ctx);
> > 	return
> > }
> >
> > The main reason this style was adopted is the claim that using
> > allocation on NULL makes it easier to valgrind stuff and find leaks, and
> > that leaks are easier to spot than huge memory growths.
> >
> > I agree that this code makes that specific task easier, but it comes at
> > a cost, a cost we always bear while instead this code is useufl only
> > once in a while when we run valgrind.
> >
> > It makes memory leaks much more dangerous, because if you forget to free
> > tmp_ctx it really is leaked
> > It requires a malloc() for even tiny functions and the context on it's
> > own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
> > Malloc is not free and can be quite expensive when some library tricks
> > glibc to go in paranoid threadsafe mode (which is surprisingly easy to
> > do) and mutexes start to be used for every allocation/free.
> >
> >
> > I'd like to propose a compromise going forward where we use a couple of
> > macros to avoid the cost of a temp context when not necessary.
> >
> > These are the macros to me called when we enter and exit the functions:
> >
> > SSS_MEM_CTX(loc_ctx, main_ctx) do { \
> >      if (debug_level & SSSDBG_TRACE_INTERNAL) { \
> > 	loc_ctx = talloc_new(NULL); \
> >      } else { \
> > 	loc_ctx = main_ctx; \
> >      } \
> > } while(0)
> >
> > SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
> >      if (loc_ctx != main_ctx) talloc_free(loc_ctx)
> >
> > Basically what we do here is that if we set debug level to
> > SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
> > otherwise we use the passed in memory context and save the overhead.
> >
> > This is just to avoid the useless overhead and avoid minor mem leaks in
> > actual production when they happen (as the memory will eventually be
> > freed by the main context) but still allow for them to be foiund easily
> > with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
> > when running under valgrind.
> >
> > IF the function needs a temporary memory context because it handles a
> > lot of memory allocations for stuff it need not return it would *Still*
> > use a local tmp_ctx, but it would be something like a local structure
> > then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
> > NULL! ].
> >
> > You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
> > they coincide.
> >
> > So this would be an example:
> >
> > CURRENT STYLE:
> >          char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
> >          {
> >              TALLOC_CTX *tmp_ctx;
> >              char *bar;
> >
> >              tmp_ctx = talloc_new(NULL);
> >
> >              bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
> >
> >              talloc_steal(mem_ctx, bar);
> >
> >              talloc_free(tmp_ctx);
> >              return bar;
> >          }
> >
> > NEW STYLE:
> >          char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
> >          {
> >          	TALLOC_CTX *loc_ctx;
> >          	char *bar;
> >
> >          	SSS_MEM_CTX(loc_ctx, mem_ctx);
> >
> >          	bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
> >
> >          	talloc_steal(mem_ctx, bar);
> >
> >          	SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
> >          	return bar;
> >          }
> >
> >
> > Discuss.
> >
> > Simo.
>
>  One concern I expressed to Simo on IRC is that we'd have to be more
>  careful with long-lived requests such as resolving deep nested group
>  memberships, otherwise the parent context will grow out of control.
>
>  If SSS_FREE_MEM_CTX was used rigorously, then it would be much less of
>  an issue. Maybe we could write a small GCC plugin to issue a warning for
>  functions that call SSS_MEM_CTX but not SSS_FREE_MEM_CTX. 
I'm not really familiar with GCC plugins, but can we model that to 
detect whether a branch decision could allow it to exit without calling 
SSS_FREE_MEM_CTX? I think some of the static analysis tools like 
Coverity can do this, but I don't know if GCC itself can.

[Jakub Hrozek]

On Mon, Jan 07, 2013 at 02:09:02PM -0500, Stephen Gallagher wrote:
>  >One concern I expressed to Simo on IRC is that we'd have to
> be more
>  >careful with long-lived requests such as resolving deep nested group
>  >memberships, otherwise the parent context will grow out of control.
>  >
>  >If SSS_FREE_MEM_CTX was used rigorously, then it would be much less of
>  >an issue. Maybe we could write a small GCC plugin to issue a warning for
>  >functions that call SSS_MEM_CTX but not SSS_FREE_MEM_CTX.
>  
>  I'm not really familiar with GCC plugins, but can we model that to
>  detect whether a branch decision could allow it to exit without
>  calling SSS_FREE_MEM_CTX? I think some of the static analysis tools
>  like Coverity can do this, but I don't know if GCC itself can. 
There is a plugin that tracks reference counts in Python plugins written
in C:
https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html#refer...

I haven't seen its source code so I can't comment on how easy/hard it
is, but it seems doable.

[Stephen Gallagher]

On Mon 07 Jan 2013 02:19:49 PM EST, Jakub Hrozek wrote:
>  On Mon, Jan 07, 2013 at 02:09:02PM -0500, Stephen Gallagher wrote:
> >> One concern I expressed to Simo on IRC is that we'd have to be more
> >> careful with long-lived requests such as resolving deep nested group
> >> memberships, otherwise the parent context will grow out of control.
> >>
> >> If SSS_FREE_MEM_CTX was used rigorously, then it would be much less of
> >> an issue. Maybe we could write a small GCC plugin to issue a warning for
> >> functions that call SSS_MEM_CTX but not SSS_FREE_MEM_CTX.
> >
> > I'm not really familiar with GCC plugins, but can we model that to
> > detect whether a branch decision could allow it to exit without
> > calling SSS_FREE_MEM_CTX? I think some of the static analysis tools
> > like Coverity can do this, but I don't know if GCC itself can.
>
>  There is a plugin that tracks reference counts in Python plugins written
>  in C:
>
> https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html#refer...
>
>  I haven't seen its source code so I can't comment on how easy/hard it
>  is, but it seems doable. 
Looks like it was written by Dave Malcolm. I'll talk with him and see 
what his thoughts are on that. Thanks for the pointer.

[Stephen Gallagher]

On 01/07/2013 02:31 PM, Stephen Gallagher wrote:
>  On Mon 07 Jan 2013 02:19:49 PM EST, Jakub Hrozek wrote:
> > On Mon, Jan 07, 2013 at 02:09:02PM -0500, Stephen Gallagher wrote:
> >>> One concern I expressed to Simo on IRC is that we'd have to be more
> >>> careful with long-lived requests such as resolving deep nested group
> >>> memberships, otherwise the parent context will grow out of control.
> >>>
> >>> If SSS_FREE_MEM_CTX was used rigorously, then it would be much less of
> >>> an issue. Maybe we could write a small GCC plugin to issue a warning
> >>> for
> >>> functions that call SSS_MEM_CTX but not SSS_FREE_MEM_CTX.
> >>
> >> I'm not really familiar with GCC plugins, but can we model that to
> >> detect whether a branch decision could allow it to exit without
> >> calling SSS_FREE_MEM_CTX? I think some of the static analysis tools
> >> like Coverity can do this, but I don't know if GCC itself can.
> >
> > There is a plugin that tracks reference counts in Python plugins written
> > in C:
> >
> https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html#refer...
> >
> >
> > I haven't seen its source code so I can't comment on how easy/hard it
> > is, but it seems doable.
>
>  Looks like it was written by Dave Malcolm. I'll talk with him and see
>  what his thoughts are on that. Thanks for the pointer. 

Following up on this, I had a conversation with David Malcolm the other 
day. We identified that his latest project (a plugin system for adding 
new glibc warnings and errors).

We identified that it will actually be very easy to write a checker for 
proper talloc_new()/talloc_free() pairings within a function. We're 
planning on hacking on it together at FUDCon next week.

With this in place, we won't need to switch over to Simo's proposed new 
macros (we still can, over time if we want), because we'll get a 
compile-time error if we're ever forgetting to clean up a temp context 
at any exit path.

[Jakub Hrozek]

On Thu, Jan 10, 2013 at 09:16:18AM -0500, Stephen Gallagher wrote:
>  On 01/07/2013 02:31 PM, Stephen Gallagher wrote:
>  >On Mon 07 Jan 2013 02:19:49 PM EST, Jakub Hrozek wrote:
>  >>On Mon, Jan 07, 2013 at 02:09:02PM -0500, Stephen Gallagher wrote:
>  >>>>One concern I expressed to Simo on IRC is that we'd have to be more
>  >>>>careful with long-lived requests such as resolving deep nested group
>  >>>>memberships, otherwise the parent context will grow out of control.
>  >>>>
>  >>>>If SSS_FREE_MEM_CTX was used rigorously, then it would be much less of
>  >>>>an issue. Maybe we could write a small GCC plugin to issue a warning
>  >>>>for
>  >>>>functions that call SSS_MEM_CTX but not SSS_FREE_MEM_CTX.
>  >>>
>  >>>I'm not really familiar with GCC plugins, but can we model that to
>  >>>detect whether a branch decision could allow it to exit without
>  >>>calling SSS_FREE_MEM_CTX? I think some of the static analysis tools
>  >>>like Coverity can do this, but I don't know if GCC itself can.
>  >>
>  >>There is a plugin that tracks reference counts in Python plugins written
>  >>in C:
>
> >>https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html#reference-count-checking
>  >>
>  >>
>  >>I haven't seen its source code so I can't comment on how easy/hard it
>  >>is, but it seems doable.
>  >
>  >Looks like it was written by Dave Malcolm. I'll talk with him and see
>  >what his thoughts are on that. Thanks for the pointer.
>  
>  
>  Following up on this, I had a conversation with David Malcolm the
>  other day. We identified that his latest project (a plugin system
>  for adding new glibc warnings and errors).
>  
>  We identified that it will actually be very easy to write a checker
>  for proper talloc_new()/talloc_free() pairings within a function.
>  We're planning on hacking on it together at FUDCon next week.
>  
>  With this in place, we won't need to switch over to Simo's proposed
>  new macros (we still can, over time if we want), because we'll get a
>  compile-time error if we're ever forgetting to clean up a temp
>  context at any exit path.
>   
That would be really awesome, thank you!

One other thing that might be checked is that if there is a
tevent_req_done() or tevent_req_error call, the function end or call
return after that. I remember a couple of bugs where we called
tevent_req_error() but carried on with the request which resulted in
access-after-free bugs.

[Pavel Březina]

On 01/07/2013 05:47 PM, Simo Sorce wrote:
>  I've always had a beef with the use of this construct in recent
> sssd
>  code:
>
>  fn()
>  {
>  	tmp_ctx = talloc_new(NULL);
>
>  	use tmp_ctx ...
>  	steal stuff on mem_ctx;
>
>  	talloc_free(tmp_ctx);
>  	return
>  }
>
>  The main reason this style was adopted is the claim that using
>  allocation on NULL makes it easier to valgrind stuff and find leaks, and
>  that leaks are easier to spot than huge memory growths.
>
>  I agree that this code makes that specific task easier, but it comes at
>  a cost, a cost we always bear while instead this code is useufl only
>  once in a while when we run valgrind.
>
>  It makes memory leaks much more dangerous, because if you forget to free
>  tmp_ctx it really is leaked
>  It requires a malloc() for even tiny functions and the context on it's
>  own uses quite a bit of space (iirc 80 bytes or so + malloc overhead).
>  Malloc is not free and can be quite expensive when some library tricks
>  glibc to go in paranoid threadsafe mode (which is surprisingly easy to
>  do) and mutexes start to be used for every allocation/free.
>
>
>  I'd like to propose a compromise going forward where we use a couple of
>  macros to avoid the cost of a temp context when not necessary.
>
>  These are the macros to me called when we enter and exit the functions:
>
>  SSS_MEM_CTX(loc_ctx, main_ctx) do { \
>       if (debug_level & SSSDBG_TRACE_INTERNAL) { \
>  	loc_ctx = talloc_new(NULL); \
>       } else { \
>  	loc_ctx = main_ctx; \
>       } \
>  } while(0)
>
>  SSS_FREE_MEM_CTX(loc_ctx, main_ctx) \
>       if (loc_ctx != main_ctx) talloc_free(loc_ctx)
>
>  Basically what we do here is that if we set debug level to
>  SSSDBG_TRACE_INTERNAL then we create brand new contexts every time,
>  otherwise we use the passed in memory context and save the overhead.
>
>  This is just to avoid the useless overhead and avoid minor mem leaks in
>  actual production when they happen (as the memory will eventually be
>  freed by the main context) but still allow for them to be foiund easily
>  with valgrind by simply setting the debug level to SSSDBG_TRACE_INTERNAL
>  when running under valgrind.
>
>  IF the function needs a temporary memory context because it handles a
>  lot of memory allocations for stuff it need not return it would *Still*
>  use a local tmp_ctx, but it would be something like a local structure
>  then explicitly freed or worst case tmp_ctx = talloc_new(loc_ctx) [ NOT
>  NULL! ].
>
>  You also keep stealing from loc_ctx to main_ctx, it will be a no-op when
>  they coincide.
>
>  So this would be an example:
>
>  CURRENT STYLE:
>           char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>           {
>               TALLOC_CTX *tmp_ctx;
>               char *bar;
>
>               tmp_ctx = talloc_new(NULL);
>
>               bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>
>               talloc_steal(mem_ctx, bar);
>
>               talloc_free(tmp_ctx);
>               return bar;
>           }
>
>  NEW STYLE:
>           char *sss_foobar_fn(TALLOC_CTX *mem_ctx, const char *foo)
>           {
>           	TALLOC_CTX *loc_ctx;
>           	char *bar;
>
>           	SSS_MEM_CTX(loc_ctx, mem_ctx);
>
>           	bar = talloc_asprint(tmp_ctx, "%s-bar", foo);
>
>           	talloc_steal(mem_ctx, bar);
>
>           	SSS_FREE_MEM_CTX(loc_ctx, mem_ctx);
>           	return bar;
>           }
>
>
>  Discuss.
>
>  Simo.
>  
I'm not against. But if we do this, we should create a new debug level 
instead of abusing an existing one.