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 <tbor...@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 <tbor...@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


>From d742357284c549aec42cde00c8f57c0baca37a2e Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbor...@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..bdb7ee8 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_byval should have sigsev)
+         * but if we end in rc==LDAP_OPERATIONS_ERROR be sure to stop here
+         * because ret is not significant */
+        if (rc != 0) {
+            LOG_OOM();
+            goto free_and_return;
+        }
+
         if (ret == 0) {
             Slapi_Value *cpw[2] = { NULL, NULL };
             Slapi_Value *pw;
-- 
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

Reply via email to