Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-03-06 Thread Tomas Babej 


On 03/05/2015 02:45 PM, Petr Spacek wrote:

On 26.2.2015 17:01, Martin Basti wrote:

On 26/02/15 13:06, Petr Spacek wrote:

Hello Martin,

thank you for patch! This NACK is only aesthetic :-)

On 25.2.2015 14:21, Martin Basti wrote:

   if (!check_return_value(rv, import_wrapped_key: key unwrapping)) {
+error = 1;
+goto final;
+}

This exact sequence is repeated many times in the code.

I would prefer a C macro like this:
#define GOTO_FAIL\
  do {\
  error = 1;\
  goto final;\
  } while(0)

This allows more dense code like:
if (!test)
 GOTO_FAIL;

and does not have the risk of missing error = 1 somewhere.


+final:
   if (pkey != NULL)
   EVP_PKEY_free(pkey);
+if (label != NULL) PyMem_Free(label);
+if (error){
+return NULL;
+}
   return ret;
   }

Apparently, this is inconsistent with itself.

Please pick one style and use it, e.g.
if (label != NULL)
 PyMem_Free(label)

... and do not add curly braces when unnecessary.

If you want, we can try running $ indent on current sources and committing
changes separately so you do not have to make changes like this by hand.


Thanks. Updated patch attached.

ACK, it works for me.



Pushed to master, ipa-4-1.

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

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-03-05 Thread Petr Spacek 
On 26.2.2015 17:01, Martin Basti wrote:
 On 26/02/15 13:06, Petr Spacek wrote:
 Hello Martin,

 thank you for patch! This NACK is only aesthetic :-)

 On 25.2.2015 14:21, Martin Basti wrote:
   if (!check_return_value(rv, import_wrapped_key: key unwrapping)) {
 +error = 1;
 +goto final;
 +}

 This exact sequence is repeated many times in the code.

 I would prefer a C macro like this:
 #define GOTO_FAIL\
  do {\
  error = 1;\
  goto final;\
  } while(0)

 This allows more dense code like:
 if (!test)
 GOTO_FAIL;

 and does not have the risk of missing error = 1 somewhere.

 +final:
   if (pkey != NULL)
   EVP_PKEY_free(pkey);
 +if (label != NULL) PyMem_Free(label);
 +if (error){
 +return NULL;
 +}
   return ret;
   }
 Apparently, this is inconsistent with itself.

 Please pick one style and use it, e.g.
 if (label != NULL)
 PyMem_Free(label)

 ... and do not add curly braces when unnecessary.

 If you want, we can try running $ indent on current sources and committing
 changes separately so you do not have to make changes like this by hand.

 Thanks. Updated patch attached.

ACK, it works for me.

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-02-26 Thread Martin Basti 

On 26/02/15 13:06, Petr Spacek wrote:

Hello Martin,

thank you for patch! This NACK is only aesthetic :-)

On 25.2.2015 14:21, Martin Basti wrote:

  if (!check_return_value(rv, import_wrapped_key: key unwrapping)) {
+error = 1;
+goto final;
+}


This exact sequence is repeated many times in the code.

I would prefer a C macro like this:
#define GOTO_FAIL   \
 do {   \
 error = 1; \
 goto final;\
 } while(0)

This allows more dense code like:
if (!test)
GOTO_FAIL;

and does not have the risk of missing error = 1 somewhere.


+final:
  if (pkey != NULL)
  EVP_PKEY_free(pkey);
+if (label != NULL) PyMem_Free(label);
+if (error){
+return NULL;
+}
  return ret;
  }

Apparently, this is inconsistent with itself.

Please pick one style and use it, e.g.
if (label != NULL)
PyMem_Free(label)

... and do not add curly braces when unnecessary.

If you want, we can try running $ indent on current sources and committing
changes separately so you do not have to make changes like this by hand.


Thanks. Updated patch attached.

--
Martin Basti

From a92e2b4cc7826c9bcaf61dc422fe5108e1a19c1c Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Tue, 24 Feb 2015 19:25:31 +0100
Subject: [PATCH] Fix memory leaks in ipap11helper

Ticket: https://fedorahosted.org/freeipa/ticket/4657
---
 ipapython/ipap11helper/p11helper.c | 311 +++--
 1 file changed, 194 insertions(+), 117 deletions(-)

diff --git a/ipapython/ipap11helper/p11helper.c b/ipapython/ipap11helper/p11helper.c
index 9a7b3ce56b4a40c23c461e40a8e1ded28a2d7c49..99534b16d68e71e79693a9f5c40b49a94aeff7dd 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -160,6 +160,12 @@ static PyObject *ipap11helperDuplicationError; //key already exists
  * Support functions
  */
 
+#define GOTO_FAIL \
+do {  \
+error = 1;\
+goto final;   \
+} while(0);
+
 CK_BBOOL* pyobj_to_bool(PyObject* pyobj) {
 if (PyObject_IsTrue(pyobj))
 return true;
@@ -200,9 +206,11 @@ unsigned char* unicode_to_char_array(PyObject *unicode, Py_ssize_t *l) {
 /* Copy string first, then DECREF
  * https://docs.python.org/2/c-api/string.html#c.PyString_AS_STRING
  */
-result = (unsigned char *) malloc((size_t) *l);
+result = (unsigned char *) PyMem_Malloc((size_t) *l);
 if (result == NULL){
-PyErr_SetString(ipap11helperError, Memory allocation error);
+Py_DECREF(utf8_str);
+PyErr_NoMemory();
+return NULL;
 } else {
 memcpy(result, bytes, *l);
 }
@@ -689,7 +697,9 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 
 PyObject *label_unicode = NULL;
 Py_ssize_t label_length = 0;
+CK_BYTE *label = NULL;
 int r;
+int error = 0;
 static char *kwlist[] = { subject, id, key_length, cka_copyable,
 cka_decrypt, cka_derive, cka_encrypt, cka_extractable,
 cka_modifiable, cka_private, cka_sensitive, cka_sign,
@@ -711,26 +721,26 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 return NULL;
 }
 
-CK_BYTE *label = (unsigned char*) unicode_to_char_array(label_unicode,
+label = (unsigned char*) unicode_to_char_array(label_unicode,
 label_length);
+if (label == NULL) GOTO_FAIL;
+
 CK_MECHANISM mechanism = { //TODO param?
 CKM_AES_KEY_GEN, NULL_PTR, 0 };
 
 if ((key_length != 16)  (key_length != 24)  (key_length != 32)) {
 PyErr_SetString(ipap11helperError,
 generate_master_key: key length allowed values are: 16, 24 and 32);
-return NULL;
+GOTO_FAIL;
 }
 
-//TODO free label if check failed
-//TODO is label freed inside dont we use freed value later
 r = _id_exists(self, id, id_length, CKO_SECRET_KEY);
 if (r == 1) {
 PyErr_SetString(ipap11helperDuplicationError,
 Master key with same ID already exists);
-return NULL;
+GOTO_FAIL;
 } else if (r == -1) {
-return NULL;
+GOTO_FAIL;
 }
 
 /* Process keyword boolean arguments */
@@ -758,9 +768,13 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 
 rv = self-p11-C_GenerateKey(self-session, mechanism, symKeyTemplate,
 sizeof(symKeyTemplate) / sizeof(CK_ATTRIBUTE), master_key);
-if (!check_return_value(rv, generate master key))
-return NULL;
+if (!check_return_value(rv, generate master key)){
+GOTO_FAIL;
+}
+final:
+if (label != NULL) PyMem_Free(label);
 
+if

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-02-26 Thread Petr Spacek 
Hello Martin,

thank you for patch! This NACK is only aesthetic :-)

On 25.2.2015 14:21, Martin Basti wrote:
  if (!check_return_value(rv, import_wrapped_key: key unwrapping)) {
 +error = 1;
 +goto final;
 +}


This exact sequence is repeated many times in the code.

I would prefer a C macro like this:
#define GOTO_FAIL   \
do {\
error = 1;  \
goto final; \
} while(0)

This allows more dense code like:
if (!test)
GOTO_FAIL;

and does not have the risk of missing error = 1 somewhere.

 +final:
  if (pkey != NULL)
  EVP_PKEY_free(pkey);
 +if (label != NULL) PyMem_Free(label);
 +if (error){
 +return NULL;
 +}
  return ret;
  }

Apparently, this is inconsistent with itself.

Please pick one style and use it, e.g.
if (label != NULL)
PyMem_Free(label)

... and do not add curly braces when unnecessary.

If you want, we can try running $ indent on current sources and committing
changes separately so you do not have to make changes like this by hand.

-- 
Petr^2 Spacek

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

[Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-02-25 Thread Martin Basti 

Ticket: https://fedorahosted.org/freeipa/ticket/4657

Patch attached.

--
Martin Basti

From 15509e09b658e9b2460ce42081baa1bb77a4d1e8 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Tue, 24 Feb 2015 19:25:31 +0100
Subject: [PATCH] Fix memory leaks in ipap11helper

Ticket: https://fedorahosted.org/freeipa/ticket/4657
---
 ipapython/ipap11helper/p11helper.c | 295 ++---
 1 file changed, 212 insertions(+), 83 deletions(-)

diff --git a/ipapython/ipap11helper/p11helper.c b/ipapython/ipap11helper/p11helper.c
index c638bbe849f1bbddc8004bd1c41128b1c9e7..e3a7a9399c12759c537d13a291dcdf6ec1a1efa4 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -197,9 +197,11 @@ unsigned char* unicode_to_char_array(PyObject *unicode, Py_ssize_t *l) {
 /* Copy string first, then DECREF
  * https://docs.python.org/2/c-api/string.html#c.PyString_AS_STRING
  */
-result = (unsigned char *) malloc((size_t) *l);
+result = (unsigned char *) PyMem_Malloc((size_t) *l);
 if (result == NULL){
-PyErr_SetString(ipap11helperError, Memory allocation error);
+Py_DECREF(utf8_str);
+PyErr_NoMemory();
+return NULL;
 } else {
 memcpy(result, bytes, *l);
 }
@@ -650,7 +652,9 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 
 PyObject *label_unicode = NULL;
 Py_ssize_t label_length = 0;
+CK_BYTE *label = NULL;
 int r;
+int error = 0;
 static char *kwlist[] = { subject, id, key_length, cka_copyable,
 cka_decrypt, cka_derive, cka_encrypt, cka_extractable,
 cka_modifiable, cka_private, cka_sensitive, cka_sign,
@@ -672,26 +676,28 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 return NULL;
 }
 
-CK_BYTE *label = (unsigned char*) unicode_to_char_array(label_unicode,
+label = (unsigned char*) unicode_to_char_array(label_unicode,
 label_length);
 CK_MECHANISM mechanism = { //TODO param?
 CKM_AES_KEY_GEN, NULL_PTR, 0 };
+if (label == NULL) return NULL;
 
 if ((key_length != 16)  (key_length != 24)  (key_length != 32)) {
 PyErr_SetString(ipap11helperError,
 generate_master_key: key length allowed values are: 16, 24 and 32);
-return NULL;
+error = 1;
+goto final;
 }
 
-//TODO free label if check failed
-//TODO is label freed inside dont we use freed value later
 r = _id_exists(self, id, id_length, CKO_SECRET_KEY);
 if (r == 1) {
 PyErr_SetString(ipap11helperDuplicationError,
 Master key with same ID already exists);
-return NULL;
+error = 1;
+goto final;
 } else if (r == -1) {
-return NULL;
+error = 1;
+goto final;
 }
 
 /* Process keyword boolean arguments */
@@ -719,9 +725,15 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
 
 rv = self-p11-C_GenerateKey(self-session, mechanism, symKeyTemplate,
 sizeof(symKeyTemplate) / sizeof(CK_ATTRIBUTE), master_key);
-if (!check_return_value(rv, generate master key))
+if (!check_return_value(rv, generate master key)){
+error = 1;
+goto final;
+}
+final:
+if (label != NULL) PyMem_Free(label);
+if (error){
 return NULL;
-
+}
 return Py_BuildValue(k, master_key);
 }
 
@@ -740,6 +752,8 @@ P11_Helper_generate_replica_key_pair(P11_Helper* self, PyObject *args,
 int id_length = 0;
 PyObject* label_unicode = NULL;
 Py_ssize_t label_length = 0;
+CK_BYTE *label = NULL;
+int error = 0;
 
 PyObj2Bool_mapping_t attrs_pub[] = { { NULL, true }, //pub_en_cka_copyable
 { NULL, false }, //pub_en_cka_derive
@@ -806,30 +820,33 @@ P11_Helper_generate_replica_key_pair(P11_Helper* self, PyObject *args,
 return NULL;
 }
 
-CK_BYTE *label = unicode_to_char_array(label_unicode, label_length);
+label = unicode_to_char_array(label_unicode, label_length);
+if (label == NULL) return NULL;
 
 CK_OBJECT_HANDLE public_key, private_key;
 CK_MECHANISM mechanism = {
 CKM_RSA_PKCS_KEY_PAIR_GEN, NULL_PTR, 0 };
 
-//TODO free variables
-
 r = _id_exists(self, id, id_length, CKO_PRIVATE_KEY);
 if (r == 1) {
 PyErr_SetString(ipap11helperDuplicationError,
 Private key with same ID already exists);
-return NULL;
+error = 1;
+goto final;
 } else if (r == -1) {
-return NULL;
+error = 1;
+goto final;
 }
 
 r = _id_exists(self, id, id_length, CKO_PUBLIC_KEY);
 if (r == 1) {
 PyErr_SetString(ipap11helperDuplicationError,
 Public key with same ID already exists);
-return NULL;
+error = 1;
+goto final;
 }

Re: [Freeipa-devel] [PATCH 0195] Fix memory leaks in ipapkcs11helper module

2015-02-25 Thread Martin Basti 

On 25/02/15 14:21, Martin Basti wrote:

Ticket: https://fedorahosted.org/freeipa/ticket/4657

Patch attached.



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

I forgot to mention, requires patch mbasti-0190

--
Martin Basti

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