[SSSD] 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