On Mon, Nov 30, 2015 at 06:45:00PM +0100, Pavel Reichl wrote: > > > On 11/30/2015 06:02 PM, Jakub Hrozek wrote: > >>> > >>>Wouldn't it then be better to see if another same object is already in > >>>the hashtable and free it before replacing? > >> > >>I agree it would be best. I tried that before and failed because I could > >>not decipher out the relation of talloc contexts. > >> > >>I tried that again. Seems that leaks are gone. Segfaults were not happening > >>during my testing. > > > >>Code got even messier :-( > > > >I think the code would look nice if it was placed in the else branch of > >create_negcache_netgr() :-) > > Done, thanks for hint. > > > > >I also don't think we need the tmp_ctx change.. > > I dare to disagree here. With prev. versions of patch I was experiencing > segfaults. IIRC step_context is being allocated on context of the netgroup > that is freed. I also think that it's not a good idea to allocate local data > on context that does not hold any reference to that data. But it might be > matter of personal taste.
What kind of segfaults, what was the backtrace? I think it might be good to add talloc_zfree() instead of talloc_free() or explicitly NULL the pointer, that should solve the issue without adding a new context... Since it's per-domain, I wonder if step_ctx->dctx might be better candidate than step_ctx either way. Some more comments inline. > From 8f2d4cdac0f5bee1847d7b13e904e84e7d706841 Mon Sep 17 00:00:00 2001 > From: Pavel Reichl <prei...@redhat.com> > Date: Fri, 27 Nov 2015 07:53:00 -0500 > Subject: [PATCH] NSS: Fix memory leak netgroup > > If netgroup cannot be found in setnetgrent_retry() function > set_netgroup_entry() is called which steals getent_ctx directly to > nss_ctx->netgroups. > Subsequently function lookup_netgr_step() is called that (in case of > nenexisting group) will call create_negcache_netgr() which creates > a new dummy object to serve as negative cache. While doing so it calls > again set_netgroup_entry() for the same netgroup and it calls > hash_enter. > > hash_enter will remove previously hashed entry for netgroup (created in > setnetgrent_retry()) from hash table but it won't be freed and thus it > leaks. This patch marks such netgroup and freed it after the call of > create_negcache_netgr(). > > Resolves: > https://fedorahosted.org/sssd/ticket/2865 > --- > src/responder/nss/nsssrv_netgroup.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/src/responder/nss/nsssrv_netgroup.c > b/src/responder/nss/nsssrv_netgroup.c > index > 9a78c1119c2f4e06e43ebec29ace775adc997e08..27ae1474a70858efb8040b864dd0e21a8bb674de > 100644 > --- a/src/responder/nss/nsssrv_netgroup.c > +++ b/src/responder/nss/nsssrv_netgroup.c > @@ -434,6 +434,7 @@ static errno_t create_negcache_netgr(struct > setent_step_ctx *step_ctx) > { > errno_t ret; > struct getent_ctx *netgr; > + struct getent_ctx *netgr_to_be_freed = NULL; > > netgr = talloc_zero(step_ctx->nctx, struct getent_ctx); > if (netgr == NULL) { > @@ -441,6 +442,11 @@ static errno_t create_negcache_netgr(struct > setent_step_ctx *step_ctx) > ret = ENOMEM; > goto done; > } else { > + /* Is there already netgroup with such name? */ > + ret = get_netgroup_entry(step_ctx->nctx, step_ctx->name, > + &netgr_to_be_freed); Bad indenting > + if (ret != EOK) netgr_to_be_freed = NULL; > + > netgr->ready = true; > netgr->found = false; > netgr->entries = NULL; > @@ -461,6 +467,9 @@ static errno_t create_negcache_netgr(struct > setent_step_ctx *step_ctx) > } > > done: > + /* Free netgroup after step_ctx is not needed. */ > + if (netgr_to_be_freed) talloc_zfree(netgr_to_be_freed); In new code we should prefer { } even for one-line statements. > + > if (ret != EOK) { > talloc_free(netgr); > } _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
