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

Stephen Gallagher sgallagh at redhat.com
Mon Jan 7 18:04:30 UTC 2013


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.



More information about the sssd-devel mailing list