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

Stephen Gallagher sgallagh at redhat.com
Mon Jan 7 19:09:02 UTC 2013


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.



More information about the sssd-devel mailing list