On 12/04/2015 03:37 PM, Jakub Hrozek wrote:
On Thu, Dec 03, 2015 at 03:29:22PM +0100, Pavel Reichl wrote:
On 12/03/2015 02:06 PM, Jakub Hrozek wrote:
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.
Hello Jakub, I amended the patch as you proposed. You can now experience the
segfault yourself - just query a non-existing netgroup.
I attached the backtrace as well.
I can investiage and use talloc reporting if you wish. But still using tmp_ctx
seems as way of the least effort...
I think you are right, the step_ctx hierarchy is tricky, so the
temporary context looks like the easiest solution. Please do one more
change in the patch:
OK, please see attached patch.
From 0160ee92c4d9a9542abfd3e918686526a52e367d 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
[...]
@@ -461,6 +469,11 @@ 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);
+ }
Since talloc_free(NULL) is a noop, we should drop the if completely.
OK, done.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>From 47e73fdd381240b67eba1d7e1de44a589388c503 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 | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
index 9a78c1119c2f4e06e43ebec29ace775adc997e08..4647a20af6b0f8e00c120839bc7e880e89156de6 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,13 @@ 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 +469,8 @@ static errno_t create_negcache_netgr(struct setent_step_ctx *step_ctx)
}
done:
+ talloc_zfree(netgr_to_be_freed);
+
if (ret != EOK) {
talloc_free(netgr);
}
@@ -474,6 +484,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 +510,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 +638,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
