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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>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);
+ 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);
+
if (ret != EOK) {
talloc_free(netgr);
}
@@ -474,6 +483,12 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
struct getent_ctx *netgr;
char *name = NULL;
uint32_t lifetime;
+ TALLOC_CTX *tmp_ctx;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
/* Check each domain for this netgroup name */
while (dom) {
@@ -494,8 +509,7 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
/* make sure to update the dctx if we changed domain */
step_ctx->dctx->domain = dom;
- talloc_free(name);
- name = sss_get_cased_name(step_ctx, step_ctx->name,
+ name = sss_get_cased_name(tmp_ctx, step_ctx->name,
dom->case_sensitive);
if (!name) {
DEBUG(SSSDBG_CRIT_FAILURE, "sss_get_cased_name failed\n");
@@ -623,10 +637,11 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
"create_negcache_netgr failed with: %d:[%s], ignored.\n",
ret, sss_strerror(ret));
}
+
ret = ENOENT;
done:
- talloc_free(name);
+ talloc_free(tmp_ctx);
return ret;
}
--
2.4.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
