On 02/12/2013 04:26 PM, Simo Sorce wrote:
> On Tue, 2013-02-12 at 16:14 +0100, Martin Kosek wrote:
>> Explained in the commit description - this may not be super-critical, I just
>> followed info in ldap_search_ext() man page:
>>
>> ...
>>
>>        Note that res parameter  of  ldap_search_ext_s()  and  ldap_search_s()
>> should  be  freed  with
>>        ldap_msgfree() regardless of return value of these functions.
>> ...
> 
> OK.
> 
>>> This snippet seem to change the logic.
>>>
>>> Before the condition was !ipadb_need_retry() and no you change it to
>>> ipadb_need_retry() so it looks reversed, was this on purpose ?
>>
>> As noted in commit description, this check was wrong and it was causing the
>> function to _always_ retry at least once because ipadb_need_retry returns 
>> true
>> when we need to retry and not 0 -> thus no "!" is needed.
> 
> I do not like much 'sneaking' this kind of change in the same patch that
> fixes memory leaks, so if you want to plit in 2 patches it would be
> nice, but I am of course ok with the change.

I agree with that. Lets do it right as we have not pushed it yet. I moved this
change to a separate patch (attached).

Martin
From 5b21acec01466a4500453770328602e180cf9256 Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Tue, 12 Feb 2013 11:59:22 +0100
Subject: [PATCH 1/2] ipa-kdb: remove memory leaks

All known memory leaks caused by unfreed allocated memory or unfreed
LDAP results (which should be also done after unsuccessful searches)
are fixed.

https://fedorahosted.org/freeipa/ticket/3413
---
 daemons/ipa-kdb/ipa_kdb.c        |  4 ++++
 daemons/ipa-kdb/ipa_kdb.h        |  2 ++
 daemons/ipa-kdb/ipa_kdb_common.c | 13 +++++++++++--
 daemons/ipa-kdb/ipa_kdb_mspac.c  |  8 ++++++++
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index 0b769f7ed76488c5febf9e610450815534398f4d..2a344dc69158dbc3f6d0207ab0bb781676240ed9 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -42,10 +42,14 @@ static void ipadb_context_free(krb5_context kcontext,
 {
     if (*ctx != NULL) {
         free((*ctx)->uri);
+        free((*ctx)->base);
+        free((*ctx)->realm_base);
         /* ldap free lcontext */
         if ((*ctx)->lcontext) {
             ldap_unbind_ext_s((*ctx)->lcontext, NULL, NULL);
         }
+        free((*ctx)->supp_encs);
+        ipadb_mspac_struct_free(&(*ctx)->mspac);
         krb5_free_default_realm(kcontext, (*ctx)->realm);
         free(*ctx);
         *ctx = NULL;
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index beff8b208685a7d561fc096c8e734333437bdb58..f472f02458e040b921b9f3f85bcc36fa954785d5 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -237,6 +237,8 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
 
 krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx);
 
+void ipadb_mspac_struct_free(struct ipadb_mspac **mspac);
+
 /* DELEGATION CHECKS */
 
 krb5_error_code ipadb_check_allowed_to_delegate(krb5_context kcontext,
diff --git a/daemons/ipa-kdb/ipa_kdb_common.c b/daemons/ipa-kdb/ipa_kdb_common.c
index e04bae6673ddc97325883708d2bca2805f6ae4fa..121b8096d602d5b84b5d64fe65c2d9dba124554b 100644
--- a/daemons/ipa-kdb/ipa_kdb_common.c
+++ b/daemons/ipa-kdb/ipa_kdb_common.c
@@ -172,7 +172,7 @@ krb5_error_code ipadb_simple_search(struct ipadb_context *ipactx,
     /* first test if we need to retry to connect */
     if (ret != 0 &&
         ipadb_need_retry(ipactx, ret)) {
-
+        ldap_msgfree(*res);
         ret = ldap_search_ext_s(ipactx->lcontext, basedn, scope,
                                 filter, attrs, 0, NULL, NULL,
                                 &std_timeout, LDAP_NO_LIMIT,
@@ -283,6 +283,7 @@ krb5_error_code ipadb_deref_search(struct ipadb_context *ipactx,
     int times;
     int ret;
     int c, i;
+    bool retry;
 
     for (c = 0; deref_attr_names[c]; c++) {
         /* count */ ;
@@ -315,7 +316,8 @@ krb5_error_code ipadb_deref_search(struct ipadb_context *ipactx,
     /* retry once if connection errors (tot. max. 2 tries) */
     times = 2;
     ret = LDAP_SUCCESS;
-    while (!ipadb_need_retry(ipactx, ret) && times > 0) {
+    retry = true;
+    while (retry) {
         times--;
         ret = ldap_search_ext_s(ipactx->lcontext, base_dn,
                                 scope, filter,
@@ -323,11 +325,18 @@ krb5_error_code ipadb_deref_search(struct ipadb_context *ipactx,
                                 ctrl, NULL,
                                 &std_timeout, LDAP_NO_LIMIT,
                                 res);
+        retry = !ipadb_need_retry(ipactx, ret) && times > 0;
+
+        if (retry) {
+            /* Free result before next try */
+            ldap_msgfree(*res);
+        }
     }
 
     kerr = ipadb_simple_ldap_to_kerr(ret);
 
 done:
+    ldap_control_free(ctrl[0]);
     ldap_memfree(derefval.bv_val);
     free(ds);
     return kerr;
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 0780e81cb5507ed590cc9b0646ba5919c0084523..950000349702038e1d55bd257816f7f61e9557a5 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -944,6 +944,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext,
             goto done;
         }
 
+        ldap_msgfree(results);
         kerr = ipadb_deref_search(ipactx, basedn, LDAP_SCOPE_ONE, filter,
                                   entry_attrs, deref_search_attrs,
                                   memberof_pac_attrs, &results);
@@ -1638,12 +1639,14 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
     ad.ad_type = KRB5_AUTHDATA_WIN2K_PAC;
     ad.contents = (krb5_octet *)pac_data.data;
     ad.length = pac_data.length;
+
     authdata[0] = &ad;
 
     kerr = krb5_encode_authdata_container(context,
                                           KRB5_AUTHDATA_IF_RELEVANT,
                                           authdata,
                                           signed_auth_data);
+    krb5_free_data_contents(context, &pac_data);
     if (kerr != 0) {
         goto done;
     }
@@ -1697,7 +1700,9 @@ void ipadb_mspac_struct_free(struct ipadb_mspac **mspac)
             free((*mspac)->trusts[i].sid_blacklist_incoming);
             free((*mspac)->trusts[i].sid_blacklist_outgoing);
         }
+        free((*mspac)->trusts);
     }
+    free(*mspac);
 
     *mspac = NULL;
 }
@@ -2040,14 +2045,17 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx)
             if (ret == 0) {
                 ret = string_to_sid(resstr, &gsid);
                 if (ret) {
+                    free(resstr);
                     kerr = ret;
                     goto done;
                 }
                 ret = sid_split_rid(&gsid, &ipactx->mspac->fallback_rid);
                 if (ret) {
+                    free(resstr);
                     kerr = ret;
                     goto done;
                 }
+                free(resstr);
             }
         }
     }
-- 
1.8.1.2

From ca7dd259d6a74169772a6b8fca2146f3853db0c8 Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Tue, 12 Feb 2013 16:35:51 +0100
Subject: [PATCH] ipa-kdb: fix retry logic in ipadb_deref_search

This function retried an LDAP search when the result was OK due to
flawed logic of retry detection (ipadb_need_retry function which
returns true when we need retry and not 0).

https://fedorahosted.org/freeipa/ticket/3413
---
 daemons/ipa-kdb/ipa_kdb_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_common.c b/daemons/ipa-kdb/ipa_kdb_common.c
index 121b8096d602d5b84b5d64fe65c2d9dba124554b..e227602ea081cc155bfffb80d2fb1758a66fa9a5 100644
--- a/daemons/ipa-kdb/ipa_kdb_common.c
+++ b/daemons/ipa-kdb/ipa_kdb_common.c
@@ -325,7 +325,7 @@ krb5_error_code ipadb_deref_search(struct ipadb_context *ipactx,
                                 ctrl, NULL,
                                 &std_timeout, LDAP_NO_LIMIT,
                                 res);
-        retry = !ipadb_need_retry(ipactx, ret) && times > 0;
+        retry = ipadb_need_retry(ipactx, ret) && times > 0;
 
         if (retry) {
             /* Free result before next try */
-- 
1.8.1.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to