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

[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

[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