[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