[Freeipa-devel] [PATCH] 0024 memory leak in ipapwd plugin

[thierry bordaz]

On 08/10/2016 07:19 PM, Alexander Bokovoy wrote:
> On Wed, 10 Aug 2016, thierry bordaz wrote:
>>
>>
>> On 08/10/2016 11:24 AM, Alexander Bokovoy wrote:
>>> On Wed, 10 Aug 2016, thierry bordaz wrote:
>>>>
>>>
>>>>> From 13bb55f9d97f82062f5b496d4164acb562afc7a0 Mon Sep 17 00:00:00 
>>>>> 2001
>>>> From: Thierry Bordaz <tbordaz at redhat.com>
>>>> Date: Tue, 9 Aug 2016 16:46:25 +0200
>>>> Subject: [PATCH] ipa-pwd-extop memory leak during passord update
>>>>
>>>> During an extend op password update, there is a test if the
>>>> user is changing the password is himself. It uses local Slapi_SDN
>>>> variable that are not freed
>>>> ---
>>>> daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c | 8 ++++++--
>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git 
>>>> a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c 
>>>> b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>>>> index 6a87a27..2eda6c6 100644
>>>> --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>>>> +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>>>> @@ -398,16 +398,20 @@ parse_req_done:
>>>>        /* if the user changing the password is self, we must 
>>>> request the
>>>>         * old password and verify it matches the current one before
>>>>         * proceeding with the password change */
>>>> -        bind_sdn = slapi_sdn_new_dn_byref(bindDN);
>>>> -        target_sdn = slapi_sdn_new_dn_byref(dn);
>>>> +        bind_sdn = slapi_sdn_new_dn_byval(bindDN);
>>>> +        target_sdn = slapi_sdn_new_dn_byval(dn);
>>>>        if (!bind_sdn || !target_sdn) {
>>>>            LOG_OOM();
>>>> +            slapi_sdn_free(&bind_sdn);
>>>> +            slapi_sdn_free(&target_sdn);
>>>>            rc = LDAP_OPERATIONS_ERROR;
>>>>            goto free_and_return;
>>>>        }
>>>>        /* this one will normalize and compare, so difference in 
>>>> case will be
>>>>         * correctly handled */
>>>>        ret = slapi_sdn_compare(bind_sdn, target_sdn);
>>>> +        slapi_sdn_free(&bind_sdn);
>>>> +        slapi_sdn_free(&target_sdn);
>>>>        if (ret == 0) {
>>>>            Slapi_Value *cpw[2] = { NULL, NULL };
>>>>            Slapi_Value *pw;
>>> I would prefer to unify memory freeing. Because slapi_sdn_compare() can
>>> be run with NULL arguments (it will return 0), and slapi_sdn_free() is
>>> no-op for NULL argument, you can actually do comparison, then free the
>>> SDNs and then do error handling:
>>>
>>>
>>> bind_sdn = slapi_sdn_new_dn_byval(bindDN);
>>> target_sdn = slapi_sdn_new_dn_byval(dn);
>>>
>>> rc = (!bind_sdn || !target_sdn) ? LDAP_OPERATIONS_ERROR : 0;
>>> ret = slapi_sdn_compare(bind_sdn, target_sdn);
>>>
>>> slapi_sdn_free(&bind_sdn);
>>> slapi_sdn_free(&target_sdn);
>>>
>>> if (rc != 0) {
>>>    LOG_OOM();
>>>    goto free_and_return;
>>> }
>>>
>>> if (ret == 0) {
>>>  ....
>>> }
>>>
>>>
>>>
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>>> -- 
>>>> Manage your subscription for the Freeipa-devel mailing list:
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>> Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
>>>
>>>
>> Thanks again Alexander for the review. Here is the revisited patch
>
>> From db4211d855b4d21354dc619952b2b2e1ad31f3b9 Mon Sep 17 00:00:00 2001
>> From: Thierry Bordaz <tbordaz at redhat.com>
>> Date: Tue, 9 Aug 2016 16:46:25 +0200
>> Subject: [PATCH] ipa-pwd-extop memory leak during passord update
>>
>> During an extend op password update, there is a test if the
>> user is changing the password is himself. It uses local Slapi_SDN
>> variable that are not freed
>> ---
>> .../ipa-pwd-extop/ipa_pwd_extop.c                  | 24 
>> +++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c 
>> b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>> index 6a87a27..eaca0dc 100644
>> --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>> +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>> @@ -398,16 +398,26 @@ parse_req_done:
>>         /* if the user changing the password is self, we must request 
>> the
>>          * old password and verify it matches the current one before
>>          * proceeding with the password change */
>> -        bind_sdn = slapi_sdn_new_dn_byref(bindDN);
>> -        target_sdn = slapi_sdn_new_dn_byref(dn);
>> -        if (!bind_sdn || !target_sdn) {
>> -            LOG_OOM();
>> -            rc = LDAP_OPERATIONS_ERROR;
>> -            goto free_and_return;
>> -        }
>> +        bind_sdn = slapi_sdn_new_dn_byval(bindDN);
>> +        target_sdn = slapi_sdn_new_dn_byval(dn);
>> +
>> +        rc = (!bind_sdn || !target_sdn) ? LDAP_OPERATIONS_ERROR : 0;
>> +
>>         /* this one will normalize and compare, so difference in case 
>> will be
>>          * correctly handled */
>>         ret = slapi_sdn_compare(bind_sdn, target_sdn);
>> +
>> +        slapi_sdn_free(&bind_sdn);
>> +        slapi_sdn_free(&target_sdn);
>> +
>> +        /* rc should always be 0 (else slapi_sdn_new_dn_byref should 
>> have sigsev)
>> +         * but if we end in rc==LDAP_OPERATIONS_ERROR be sure to 
>> stop here
>> +         * because ret is not significant */
> A short note here. You talk about slapi_sdn_new_dn_byref() but your
> patch replaces that with slapi_sdn_new_dn_byval(). Does the comment
> still apply?
>
>> +        if (rc != 0) {
>> +            LOG_OOM();
>> +            goto free_and_return;
>> +        }
>> +
>>         if (ret == 0) {
>>             Slapi_Value *cpw[2] = { NULL, NULL };
>>             Slapi_Value *pw;
>> -- 
>> 2.7.4
>>
>
>
Good catch Alexander. Yes the comment contained a wrong cut/paste


-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-44-tbordaz-024-3-ipa-pwd-extop-memory-leak-during-passord-update.patch
Type: text/x-patch
Size: 2115 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160811/766d2ebe/attachment.bin>