[Freeipa-devel] [PATCH] 370 ipa-kdb: remove memory leaks

2013-02-12 Thread Martin Kosek 
All known memory leaks caused by unfreed allocated memory or unfreed
LDAP results (which should be also done after unsuccessful searches)
are fixed.

One ipadb_need_retry result check was fixed as this function returns
trust in case of a need for retry and not a zero.

https://fedorahosted.org/freeipa/ticket/3413

---

This patch should be combined with Sumit's patch 0093 which would prevent
kdb5kdc from crashing when it is being terminted.

If you want to run krb5kdc in valgrind, check the command in the ticket which
would let valgrind also show symbols in dlopen'ed ipa-kdb plugin.

Martin
From 3927ce572eef0d7fe6668201cec8d519fe7fa815 Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Tue, 12 Feb 2013 11:59:22 +0100
Subject: [PATCH] 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.

One ipadb_need_retry result check was fixed as this function returns
trust in case of a need for retry and not a zero.

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  | 18 +-
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index 3527cefa10df67d3f17c730ab4483410c736a44f..55a932abdc3aeaa48892715196834a25f9b2c0e1 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -40,10 +40,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..956b4b611cbdedbe1a9b0415e9b10f762e46fdae 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..6abd51ec019ef45dd52d317b85dcb9584b49f06f 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,

Re: [Freeipa-devel] [PATCH] 370 ipa-kdb: remove memory leaks

2013-02-12 Thread Sumit Bose 
On Tue, Feb 12, 2013 at 12:24:48PM +0100, Martin Kosek wrote:
 All known memory leaks caused by unfreed allocated memory or unfreed
 LDAP results (which should be also done after unsuccessful searches)
 are fixed.
 
 One ipadb_need_retry result check was fixed as this function returns
 trust in case of a need for retry and not a zero.
 
 https://fedorahosted.org/freeipa/ticket/3413
 
 ---
 
 This patch should be combined with Sumit's patch 0093 which would prevent
 kdb5kdc from crashing when it is being terminted.
 
 If you want to run krb5kdc in valgrind, check the command in the ticket which
 would let valgrind also show symbols in dlopen'ed ipa-kdb plugin.
 
 Martin

I haven't look closely so far, but I see the following compiler warning:

ipa_kdb_common.c: In function 'ipadb_simple_search':
ipa_kdb_common.c:175:9: warning: passing argument 1 of 'ldap_msgfree'
from incompatible pointer type [enabled by default]
In file included from ipa_kdb.h:35:0,
 from ipa_kdb_common.c:23:
/usr/include/ldap.h:1848:1: note: expected 'struct LDAPMessage *' but argument 
is of type 'struct LDAPMessage **'

bye,
Sumit

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

Re: [Freeipa-devel] [PATCH] 370 ipa-kdb: remove memory leaks

2013-02-12 Thread Simo Sorce 
On Tue, 2013-02-12 at 12:24 +0100, Martin Kosek wrote:

Comments inline.

 --- 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);

Why do we need this ?
We get in this branch only if the previous search failed, so res should
always be empty. What am I missing ?

  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);
 +}
  }

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 ?

  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..6abd51ec019ef45dd52d317b85dcb9584b49f06f
  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);
 @@ -1636,14 +1637,24 @@ krb5_error_code
 ipadb_sign_authdata(krb5_context context,
  /* put in signed data */
  ad.magic = KV5M_AUTHDATA;
  ad.ad_type = KRB5_AUTHDATA_WIN2K_PAC;
 -ad.contents = (krb5_octet *)pac_data.data;
 +ad.contents = malloc(pac_data.length);
 +if (ad.contents == NULL) {
 +krb5_free_data_contents(context, pac_data);
 +ad.length = 0;
 +kerr = ENOMEM;
 +goto done;
 +}
 +memcpy(ad.contents, pac_data.data, pac_data.length);
  ad.length = pac_data.length;
 +krb5_free_data_contents(context, pac_data);
 +

Why all this copying around ?
It should be sufficient to call krb5_free_data_contents() at the end at
most.


  authdata[0] = ad;
  
  kerr = krb5_encode_authdata_container(context,
KRB5_AUTHDATA_IF_RELEVANT,
authdata,
signed_auth_data);
 +free(ad.contents);
  if (kerr != 0) {
  goto done;
  }

The rest looks good.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

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