Re: [Freeipa-devel] [PATCH] fix memleaks

2011-12-02 Thread Alexander Bokovoy 
On Tue, 29 Nov 2011, Simo Sorce wrote:
 Found a couple of memleaks while reviewing code.
 
 Attached.
 
 Simo.
 
 -- 
 Simo Sorce * Red Hat, Inc * New York

 From 70840691e48e1ac89002499c08a9dd4fdcae7c50 Mon Sep 17 00:00:00 2001
 From: Simo Sorce sso...@redhat.com
 Date: Sun, 20 Nov 2011 20:50:11 -0500
 Subject: [PATCH] ipa-kdb: fix memleaks in ipa_kdb_mspac.c
 
 ---
  daemons/ipa-kdb/ipa_kdb_mspac.c |   13 +++--
  1 files changed, 7 insertions(+), 6 deletions(-)
 
 diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
 index 
 cce1ca9060f0e03d525bb87d843bdd5811e9d20b..0c0da75ca8dbedf12e39e8ec5d87bfd4cd485d4a
  100644
 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c
 +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
 @@ -466,7 +466,7 @@ static krb5_error_code ipadb_get_pac(krb5_context 
 kcontext,
  TALLOC_CTX *tmpctx;
  struct ipadb_e_data *ied;
  struct ipadb_context *ipactx;
 -LDAPMessage *results;
 +LDAPMessage *results = NULL;
  LDAPMessage *lentry;
  DATA_BLOB pac_data;
  krb5_data data;
 @@ -479,11 +479,6 @@ static krb5_error_code ipadb_get_pac(krb5_context 
 kcontext,
  return KRB5_KDB_DBNOTINITED;
  }
  
 -tmpctx = talloc_new(NULL);
 -if (!tmpctx) {
 -return ENOMEM;
 -}
 -
  ied = (struct ipadb_e_data *)client-e_data;
  if (ied-magic != IPA_E_DATA_MAGIC) {
  return EINVAL;
 @@ -493,6 +488,11 @@ static krb5_error_code ipadb_get_pac(krb5_context 
 kcontext,
  return 0;
  }
  
 +tmpctx = talloc_new(NULL);
 +if (!tmpctx) {
 +return ENOMEM;
 +}
 +
  memset(pac_info, 0, sizeof(pac_info));
  pac_info.logon_info.info = talloc_zero(tmpctx, struct PAC_LOGON_INFO);
  if (!tmpctx) {
Here is an issue -- you are allocating off tmpctx which is not empty 
here (we checked it right above) but then you are checking tmpctx 
rather than pac_info.logon_info.info.

It is an older error but needs to be fixed as well.

Also please name the patch file according to 
https://fedorahosted.org/freeipa/wiki/PatchFormat :)

-- 
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH] fix memleaks

2011-12-02 Thread Simo Sorce 
On Fri, 2011-12-02 at 16:04 +0200, Alexander Bokovoy wrote:
 On Tue, 29 Nov 2011, Simo Sorce wrote:
  Found a couple of memleaks while reviewing code.
  
  Attached.
  
  Simo.
  
  -- 
  Simo Sorce * Red Hat, Inc * New York
 
  From 70840691e48e1ac89002499c08a9dd4fdcae7c50 Mon Sep 17 00:00:00 2001
  From: Simo Sorce sso...@redhat.com
  Date: Sun, 20 Nov 2011 20:50:11 -0500
  Subject: [PATCH] ipa-kdb: fix memleaks in ipa_kdb_mspac.c
  
  ---
   daemons/ipa-kdb/ipa_kdb_mspac.c |   13 +++--
   1 files changed, 7 insertions(+), 6 deletions(-)
  
  diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c 
  b/daemons/ipa-kdb/ipa_kdb_mspac.c
  index 
  cce1ca9060f0e03d525bb87d843bdd5811e9d20b..0c0da75ca8dbedf12e39e8ec5d87bfd4cd485d4a
   100644
  --- a/daemons/ipa-kdb/ipa_kdb_mspac.c
  +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
  @@ -466,7 +466,7 @@ static krb5_error_code ipadb_get_pac(krb5_context 
  kcontext,
   TALLOC_CTX *tmpctx;
   struct ipadb_e_data *ied;
   struct ipadb_context *ipactx;
  -LDAPMessage *results;
  +LDAPMessage *results = NULL;
   LDAPMessage *lentry;
   DATA_BLOB pac_data;
   krb5_data data;
  @@ -479,11 +479,6 @@ static krb5_error_code ipadb_get_pac(krb5_context 
  kcontext,
   return KRB5_KDB_DBNOTINITED;
   }
   
  -tmpctx = talloc_new(NULL);
  -if (!tmpctx) {
  -return ENOMEM;
  -}
  -
   ied = (struct ipadb_e_data *)client-e_data;
   if (ied-magic != IPA_E_DATA_MAGIC) {
   return EINVAL;
  @@ -493,6 +488,11 @@ static krb5_error_code ipadb_get_pac(krb5_context 
  kcontext,
   return 0;
   }
   
  +tmpctx = talloc_new(NULL);
  +if (!tmpctx) {
  +return ENOMEM;
  +}
  +
   memset(pac_info, 0, sizeof(pac_info));
   pac_info.logon_info.info = talloc_zero(tmpctx, struct PAC_LOGON_INFO);
   if (!tmpctx) {
 Here is an issue -- you are allocating off tmpctx which is not empty 
 here (we checked it right above) but then you are checking tmpctx 
 rather than pac_info.logon_info.info.
 
 It is an older error but needs to be fixed as well.

Ok added this fix.

 Also please name the patch file according to 
 https://fedorahosted.org/freeipa/wiki/PatchFormat :)

I lost count of how many patches I handled, and to be honest I think
this naming convention sucks a bit.

I would be ok changing the number to match a ticket number perhaps, when
there is a ticket attached to it, but for patches like this one all you
really-need it to append a -2 at the end if a new revision is necessary.
Putting arbitrary numbers and names, doesn't make it any easier to
handle to me.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 31e01a3350dfc9f2354c9738f29de23450dac744 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Sun, 20 Nov 2011 20:50:11 -0500
Subject: [PATCH] ipa-kdb: fix memleaks in ipa_kdb_mspac.c

---
 daemons/ipa-kdb/ipa_kdb_mspac.c |   15 ---
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index cce1ca9060f0e03d525bb87d843bdd5811e9d20b..b435efeec0215d50694eeb668c0286b80056016f 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -466,7 +466,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 TALLOC_CTX *tmpctx;
 struct ipadb_e_data *ied;
 struct ipadb_context *ipactx;
-LDAPMessage *results;
+LDAPMessage *results = NULL;
 LDAPMessage *lentry;
 DATA_BLOB pac_data;
 krb5_data data;
@@ -479,11 +479,6 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 return KRB5_KDB_DBNOTINITED;
 }
 
-tmpctx = talloc_new(NULL);
-if (!tmpctx) {
-return ENOMEM;
-}
-
 ied = (struct ipadb_e_data *)client-e_data;
 if (ied-magic != IPA_E_DATA_MAGIC) {
 return EINVAL;
@@ -493,9 +488,14 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 return 0;
 }
 
+tmpctx = talloc_new(NULL);
+if (!tmpctx) {
+return ENOMEM;
+}
+
 memset(pac_info, 0, sizeof(pac_info));
 pac_info.logon_info.info = talloc_zero(tmpctx, struct PAC_LOGON_INFO);
-if (!tmpctx) {
+if (!pac_info.logon_info.info) {
 kerr = ENOMEM;
 goto done;
 }
@@ -542,6 +542,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 kerr = krb5_pac_add_buffer(kcontext, *pac, KRB5_PAC_LOGON_INFO, data);
 
 done:
+ldap_msgfree(results);
 talloc_free(tmpctx);
 return kerr;
 }
-- 
1.7.7.1

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

Re: [Freeipa-devel] [PATCH] fix memleaks

2011-12-02 Thread Alexander Bokovoy 
On Fri, 02 Dec 2011, Simo Sorce wrote:
memset(pac_info, 0, sizeof(pac_info));
pac_info.logon_info.info = talloc_zero(tmpctx, struct 
   PAC_LOGON_INFO);
if (!tmpctx) {
  Here is an issue -- you are allocating off tmpctx which is not empty 
  here (we checked it right above) but then you are checking tmpctx 
  rather than pac_info.logon_info.info.
  
  It is an older error but needs to be fixed as well.
 
 Ok added this fix.
ACK.

  Also please name the patch file according to 
  https://fedorahosted.org/freeipa/wiki/PatchFormat :)
 
 I lost count of how many patches I handled, and to be honest I think
 this naming convention sucks a bit.
 
 I would be ok changing the number to match a ticket number perhaps, when
 there is a ticket attached to it, but for patches like this one all you
 really-need it to append a -2 at the end if a new revision is necessary.
 Putting arbitrary numbers and names, doesn't make it any easier to
 handle to me.
I'm fine with any way that does not introduce file names conflicts.
We can even write a simple script that takes information from the 
patch itself and renames the files in a directory according to the 
established practice.

But I think better solution would be to use Gerrit to handle patch 
reviews. Then the issue of patch names will go away as you'll be 
dealing with git pull/push mostly.

-- 
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH] fix memleaks

2011-12-02 Thread Rob Crittenden 

Simo Sorce wrote:

I lost count of how many patches I handled, and to be honest I think
this naming convention sucks a bit.

I would be ok changing the number to match a ticket number perhaps, when
there is a ticket attached to it, but for patches like this one all you
really-need it to append a -2 at the end if a new revision is necessary.
Putting arbitrary numbers and names, doesn't make it any easier to
handle to me.


The numbers can help at commit time when ordering matters. Most of the 
time they are just decorators.


Being able to identify the owner of a patch in the file name is quite 
handy IMHO.


rob

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

Re: [Freeipa-devel] [PATCH] fix memleaks

2011-12-02 Thread Simo Sorce 
On Fri, 2011-12-02 at 16:39 +0200, Alexander Bokovoy wrote:
 On Fri, 02 Dec 2011, Simo Sorce wrote:
 memset(pac_info, 0, sizeof(pac_info));
 pac_info.logon_info.info = talloc_zero(tmpctx, struct 
PAC_LOGON_INFO);
 if (!tmpctx) {
   Here is an issue -- you are allocating off tmpctx which is not empty 
   here (we checked it right above) but then you are checking tmpctx 
   rather than pac_info.logon_info.info.
   
   It is an older error but needs to be fixed as well.
  
  Ok added this fix.
 ACK.

Ok pushed to master.

   Also please name the patch file according to 
   https://fedorahosted.org/freeipa/wiki/PatchFormat :)
  
  I lost count of how many patches I handled, and to be honest I think
  this naming convention sucks a bit.
  
  I would be ok changing the number to match a ticket number perhaps, when
  there is a ticket attached to it, but for patches like this one all you
  really-need it to append a -2 at the end if a new revision is necessary.
  Putting arbitrary numbers and names, doesn't make it any easier to
  handle to me.
 I'm fine with any way that does not introduce file names conflicts.
 We can even write a simple script that takes information from the 
 patch itself and renames the files in a directory according to the 
 established practice.
 
 But I think better solution would be to use Gerrit to handle patch 
 reviews. Then the issue of patch names will go away as you'll be 
 dealing with git pull/push mostly.

Yes we are working on that with SSSD, if experimentation goes well we
will propose using gerrit for freeIPA too.

Simo.

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

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

[Freeipa-devel] [PATCH] fix memleaks

2011-11-29 Thread Simo Sorce 
Found a couple of memleaks while reviewing code.

Attached.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 70840691e48e1ac89002499c08a9dd4fdcae7c50 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Sun, 20 Nov 2011 20:50:11 -0500
Subject: [PATCH] ipa-kdb: fix memleaks in ipa_kdb_mspac.c

---
 daemons/ipa-kdb/ipa_kdb_mspac.c |   13 +++--
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index cce1ca9060f0e03d525bb87d843bdd5811e9d20b..0c0da75ca8dbedf12e39e8ec5d87bfd4cd485d4a 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -466,7 +466,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 TALLOC_CTX *tmpctx;
 struct ipadb_e_data *ied;
 struct ipadb_context *ipactx;
-LDAPMessage *results;
+LDAPMessage *results = NULL;
 LDAPMessage *lentry;
 DATA_BLOB pac_data;
 krb5_data data;
@@ -479,11 +479,6 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 return KRB5_KDB_DBNOTINITED;
 }
 
-tmpctx = talloc_new(NULL);
-if (!tmpctx) {
-return ENOMEM;
-}
-
 ied = (struct ipadb_e_data *)client-e_data;
 if (ied-magic != IPA_E_DATA_MAGIC) {
 return EINVAL;
@@ -493,6 +488,11 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 return 0;
 }
 
+tmpctx = talloc_new(NULL);
+if (!tmpctx) {
+return ENOMEM;
+}
+
 memset(pac_info, 0, sizeof(pac_info));
 pac_info.logon_info.info = talloc_zero(tmpctx, struct PAC_LOGON_INFO);
 if (!tmpctx) {
@@ -542,6 +542,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 kerr = krb5_pac_add_buffer(kcontext, *pac, KRB5_PAC_LOGON_INFO, data);
 
 done:
+ldap_msgfree(results);
 talloc_free(tmpctx);
 return kerr;
 }
-- 
1.7.7.1

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