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.