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...