[Freeipa-devel] [PATCH] 0024 memory leak in ipapwd plugin
Alexander Bokovoy
abokovoy at redhat.com
Wed Aug 10 17:19:27 UTC 2016
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
>
--
/ Alexander Bokovoy
More information about the Freeipa-devel
mailing list