[Freeipa-devel] [PATCH 142] extdom: fix memory leak

[Jan Cholasta]

Dne 10.3.2015 v 12:14 Tomas Babej napsal(a):
>
> On 03/10/2015 12:10 PM, Sumit Bose wrote:
>> On Tue, Mar 10, 2015 at 11:59:45AM +0100, Tomas Babej wrote:
>>> On 03/05/2015 08:00 AM, Alexander Bokovoy wrote:
>>>> On Wed, 04 Mar 2015, Nathan Kinder wrote:
>>>>>
>>>>> On 03/04/2015 10:34 PM, Alexander Bokovoy wrote:
>>>>>> On Wed, 04 Mar 2015, Sumit Bose wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> while running 389ds with valgrind to see if my other patches
>>>>>>> introduced
>>>>>>> a memory leak I found an older one which is fixed by this patch.
>>>>>>>
>>>>>>> bye,
>>>>>>> Sumit
>>>>>> >From bb02cdc135fecc1766b17edd61554dbde9bccd0b Mon Sep 17 00:00:00
>>>>>> 2001
>>>>>>> From: Sumit Bose <sbose at redhat.com>
>>>>>>> Date: Wed, 4 Mar 2015 17:53:08 +0100
>>>>>>> Subject: [PATCH] extdom: fix memory leak
>>>>>>>
>>>>>>> ---
>>>>>>> daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
>>>>>>> b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
>>>>>>> index
>>>>>>> a040f2beba073d856053429face2f464347b2524..708d0e4a2fc9da4f87a24a49c945587049f7280f
>>>>>>>
>>>>>>>
>>>>>>> 100644
>>>>>>> --- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
>>>>>>> +++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
>>>>>>> @@ -156,6 +156,7 @@ done:
>>>>>>>         LOG("%s", err_msg);
>>>>>>>     }
>>>>>>>     slapi_send_ldap_result(pb, rc, NULL, err_msg, 0, NULL);
>>>>>>> +    ber_bvfree(ret_val);
>>>>>>>     free_req_data(req);
>>>>>>>     return SLAPI_PLUGIN_EXTENDED_SENT_RESULT;
>>>>>>> }
>>>>>> I can see in 389-ds code that it actually tries to remove the
>>>>>> value in
>>>>>> the end of extended operation handling:
>>>>> This below code snippet is freeing the extended operation request
>>>>> value
>>>>> (SLAPI_EXT_OP_REQ_VALUE), not the return value (SLAPI_EXT_OP_RET_VAL).
>>>>>
>>>>> If you look at check_and_send_extended_result() in the 389-ds code,
>>>>> you'll see where the extended operation return value is sent, and it
>>>>> doesn't perform a free.  It is up to the plug-in to perform the
>>>>> free.  A
>>>>> good example of this in the 389-ds code is in the
>>>>> passwd_modify_extop()
>>>>> function.
>>>>>
>>>>>
>>>>> Sumit's code looks good to me.  ACK.
>>>> Argh. Sorry for confusion of RET vs REQ. Morning, I need coffee!
>>>>
>>>> ACK.
>>>>
>>>>> -NGK
>>>>>
>>>>>>        slapi_pblock_set( pb, SLAPI_EXT_OP_REQ_OID, extoid );
>>>>>>        slapi_pblock_set( pb, SLAPI_EXT_OP_REQ_VALUE, &extval );
>>>>>>        slapi_pblock_set( pb, SLAPI_REQUESTOR_ISROOT,
>>>>>> &pb->pb_op->o_isroot);
>>>>>>
>>>>>>        rc = plugin_call_exop_plugins( pb, extoid );
>>>>>>
>>>>>>        if ( SLAPI_PLUGIN_EXTENDED_SENT_RESULT != rc ) {
>>>>>>                if ( SLAPI_PLUGIN_EXTENDED_NOT_HANDLED == rc ) {
>>>>>>                        lderr = LDAP_PROTOCOL_ERROR;    /* no plugin
>>>>>> handled the op */
>>>>>>                        errmsg = "unsupported extended operation";
>>>>>>                } else {
>>>>>>                        errmsg = NULL;
>>>>>>                        lderr = rc;
>>>>>>                }
>>>>>>                send_ldap_result( pb, lderr, NULL, errmsg, 0, NULL );
>>>>>>        }
>>>>>> free_and_return:
>>>>>>        if (extoid)
>>>>>>                slapi_ch_free((void **)&extoid);
>>>>>>        if (extval.bv_val)
>>>>>>                slapi_ch_free((void **)&extval.bv_val);
>>>>>>        return;
>>>>>>
>>>>>>
>>> The patch does not apply to the current master branch.
>>>
>>> Sumit, can you send a updated version?
>> sure, new version attached.
>>
>> bye,
>> Sumit
>>
>>> Thanks,
>>> Tomas
> Thanks,
>
> Pushed to master: 8dac096ae3a294dc55b32b69b873013fd687e945
>

and to ipa-4-1: 179be3c222a9d27a147d5c0ff4be45e7def9b2d5

-- 
Jan Cholasta