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 <preichl(a)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);
> }