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 <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.
OK, done.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org