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

Simo Sorce simo at redhat.com
Mon Jan 7 18:23:39 UTC 2013


On Mon, 2013-01-07 at 13:04 -0500, Stephen Gallagher wrote:
> 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.

Those functions need not be changed if not necessary, I think passing in
a memory context is a very small price to pay and allows better memory
management.

Initially I was thinking of using that as a method to change the style
for the better and let caller handle the context (wheteher to make a
temp one or not etc...) but it would have made some of the code be
really ugly, so it wouldn't have be 'better' at all :-).

> 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.

Why not, please justify your *should NOT*, this is how talloc has been
designed, to make it simpler to handle temporary memory in a
hierarchical set of functions. Relations do not need to be strictly a
structure or a function, it can be a tree of functions as well.

>  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.

NO, actually I think I will change the macro to *assert* if main_ctx is
NULL.

The purpose of this function is to *NOT* alloc on NULL.

> I don't really like passing nonsensical arguments to a function just to 
> *work around* sloppy coding.

I can also arrange this macro differently so that it looks like:

loc_ctx = SSS_MEM_CTX(mem_ctx)

But then we'd have to not use ';' at then end which is almost always
going to be added.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list