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:
From 0160ee92c4d9a9542abfd3e918686526a52e367d 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
[...]
@@ -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.