[SSSD] On talloc_new(NULL) (ab)use in SSSD

Jakub Hrozek jhrozek at redhat.com
Mon Jan 7 19:06:45 UTC 2013


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.



More information about the sssd-devel mailing list