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

[thierry bordaz]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-44-tbordaz-024-2-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/20160810/fb77a158/attachment.bin>