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

[Tomas Babej]

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?

Thanks,
Tomas