Re: [Freeipa-devel] [PATCH 0182] Fix pkcs11 python extension reference counting
Hi, Dne 12.1.2015 v 13:11 Martin Basti napsal(a): Part of DNSSEC (https://fedorahosted.org/freeipa/ticket/4657) Patch attached. 1) In P11_Helper_set_attribute, the return value can be only Py_None or NULL, so why don't you use Py_XINCREF instead of the if/Py_INCREF? 2) In P11_Helper_find_keys, you sometimes return without decreasing reference count on result_list or any items it may contain. Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0182] Fix pkcs11 python extension reference counting
On 10/02/15 10:11, Jan Cholasta wrote:
Hi,
Dne 12.1.2015 v 13:11 Martin Basti napsal(a):
Part of DNSSEC (https://fedorahosted.org/freeipa/ticket/4657)
Patch attached.
1) In P11_Helper_set_attribute, the return value can be only Py_None
or NULL, so why don't you use Py_XINCREF instead of the if/Py_INCREF?
2) In P11_Helper_find_keys, you sometimes return without decreasing
reference count on result_list or any items it may contain.
Honza
Thank you,
updated patch attached.
--
Martin Basti
From 99b61643e0613988d7037fc5cc28cf4fc27bd881 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Mon, 12 Jan 2015 13:05:53 +0100
Subject: [PATCH] Fix reference counting in pkcs11 extension
* removed unneeded reference increment
* added increment of Py_None
Part of ticket: https://fedorahosted.org/freeipa/ticket/4657
---
ipapython/ipap11helper/p11helper.c | 53 ++
1 file changed, 25 insertions(+), 28 deletions(-)
diff --git a/ipapython/ipap11helper/p11helper.c b/ipapython/ipap11helper/p11helper.c
index c7c259590a53589de7bdd995395681615f3cae1f..4e0f262057b377124793f1e3091a8c9df4794164 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -142,9 +142,7 @@ void convert_py2bool(PyObj2Bool_mapping_t* mapping, int length) {
for (i = 0; i length; ++i) {
PyObject* py_obj = mapping[i].py_obj;
if (py_obj != NULL) {
-Py_INCREF(py_obj);
mapping[i].bool = pyobj_to_bool(py_obj);
-Py_DECREF(py_obj);
}
}
}
@@ -156,21 +154,32 @@ void convert_py2bool(PyObj2Bool_mapping_t* mapping, int length) {
* Returns NULL if an error occurs, else pointer to string
*/
unsigned char* unicode_to_char_array(PyObject *unicode, Py_ssize_t *l) {
+unsigned char* result = NULL;
PyObject* utf8_str = PyUnicode_AsUTF8String(unicode);
if (utf8_str == NULL) {
PyErr_SetString(ipap11helperError, Unable to encode UTF-8);
return NULL;
}
-Py_XINCREF(utf8_str);
unsigned char* bytes = (unsigned char*) PyString_AS_STRING(utf8_str);
if (bytes == NULL) {
PyErr_SetString(ipap11helperError, Unable to get bytes from string);
*l = 0;
} else {
*l = PyString_Size(utf8_str);
+
+/* 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);
+if (result == NULL){
+PyErr_SetString(ipap11helperError, Memory allocation error);
+} else {
+memcpy(result, bytes, *l);
+}
+
}
-Py_XDECREF(utf8_str);
-return bytes;
+Py_DECREF(utf8_str);
+return result;
}
/**
@@ -546,7 +555,7 @@ P11_Helper_finalize(P11_Helper* self) {
CK_RV rv;
if (self-p11 == NULL)
-return Py_None;
+Py_RETURN_NONE;
/*
* Logout
@@ -576,7 +585,7 @@ P11_Helper_finalize(P11_Helper* self) {
self-slot = 0;
self-module_handle = NULL;
-return Py_None;
+Py_RETURN_NONE;
}
/
@@ -636,10 +645,8 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
return NULL;
}
-Py_XINCREF(label_unicode);
CK_BYTE *label = (unsigned char*) unicode_to_char_array(label_unicode,
label_length);
-Py_XDECREF(label_unicode);
CK_MECHANISM mechanism = { //TODO param?
CKM_AES_KEY_GEN, NULL_PTR, 0 };
@@ -688,7 +695,7 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
if (!check_return_value(rv, generate master key))
return NULL;
-return Py_BuildValue(k, master_key);;
+return Py_BuildValue(k, master_key);
}
/**
@@ -772,9 +779,7 @@ P11_Helper_generate_replica_key_pair(P11_Helper* self, PyObject *args,
return NULL;
}
-Py_XINCREF(label_unicode);
CK_BYTE *label = unicode_to_char_array(label_unicode, label_length);
-Py_XDECREF(label_unicode);
CK_OBJECT_HANDLE public_key, private_key;
CK_MECHANISM mechanism = {
@@ -887,30 +892,24 @@ P11_Helper_find_keys(P11_Helper* self, PyObject *args, PyObject *kwds) {
}
if (label_unicode != NULL) {
-Py_INCREF(label_unicode);
label = (unsigned char*) unicode_to_char_array(label_unicode,
label_length); //TODO verify signed/unsigned
-Py_DECREF(label_unicode);
}
if (cka_wrap_bool != NULL) {
-Py_INCREF(cka_wrap_bool);
if (PyObject_IsTrue(cka_wrap_bool)) {
ckawrap = true;
} else {
ckawrap = false;
}
-Py_DECREF(cka_wrap_bool);
}
if (cka_unwrap_bool != NULL) {
-Py_INCREF(cka_unwrap_bool);
if (PyObject_IsTrue(cka_unwrap_bool)) {
ckaunwrap = true;
} else {
ckaunwrap =
Re: [Freeipa-devel] [PATCH 0182] Fix pkcs11 python extension reference counting
Dne 10.2.2015 v 14:56 Martin Basti napsal(a): On 10/02/15 10:11, Jan Cholasta wrote: Hi, Dne 12.1.2015 v 13:11 Martin Basti napsal(a): Part of DNSSEC (https://fedorahosted.org/freeipa/ticket/4657) Patch attached. 1) In P11_Helper_set_attribute, the return value can be only Py_None or NULL, so why don't you use Py_XINCREF instead of the if/Py_INCREF? 2) In P11_Helper_find_keys, you sometimes return without decreasing reference count on result_list or any items it may contain. Honza Thank you, updated patch attached. Thanks, ACK. Pushed to: master: dd607a35bcdad125c098f0e80c8b25efbe935acc ipa-4-1: c198b5ab6537f26936554a9e2e5bfcbe3f371320 -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0182] Fix pkcs11 python extension reference counting
Part of DNSSEC (https://fedorahosted.org/freeipa/ticket/4657)
Patch attached.
--
Martin Basti
From 28b3ca9f12e75afc581d0ac6c462c8647e054a07 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Mon, 12 Jan 2015 13:05:53 +0100
Subject: [PATCH] Fix reference counting in pkcs11 extension
* removed unneeded reference increment
* added increment of Py_None
Part of ticket: https://fedorahosted.org/freeipa/ticket/4657
---
ipapython/ipap11helper/p11helper.c | 53 ++
1 file changed, 25 insertions(+), 28 deletions(-)
diff --git a/ipapython/ipap11helper/p11helper.c b/ipapython/ipap11helper/p11helper.c
index c7c259590a53589de7bdd995395681615f3cae1f..6840b34a08d8178f70fda41125e66d5e83443d64 100644
--- a/ipapython/ipap11helper/p11helper.c
+++ b/ipapython/ipap11helper/p11helper.c
@@ -142,9 +142,7 @@ void convert_py2bool(PyObj2Bool_mapping_t* mapping, int length) {
for (i = 0; i length; ++i) {
PyObject* py_obj = mapping[i].py_obj;
if (py_obj != NULL) {
-Py_INCREF(py_obj);
mapping[i].bool = pyobj_to_bool(py_obj);
-Py_DECREF(py_obj);
}
}
}
@@ -156,21 +154,32 @@ void convert_py2bool(PyObj2Bool_mapping_t* mapping, int length) {
* Returns NULL if an error occurs, else pointer to string
*/
unsigned char* unicode_to_char_array(PyObject *unicode, Py_ssize_t *l) {
+unsigned char* result = NULL;
PyObject* utf8_str = PyUnicode_AsUTF8String(unicode);
if (utf8_str == NULL) {
PyErr_SetString(ipap11helperError, Unable to encode UTF-8);
return NULL;
}
-Py_XINCREF(utf8_str);
unsigned char* bytes = (unsigned char*) PyString_AS_STRING(utf8_str);
if (bytes == NULL) {
PyErr_SetString(ipap11helperError, Unable to get bytes from string);
*l = 0;
} else {
*l = PyString_Size(utf8_str);
+
+/* 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);
+if (result == NULL){
+PyErr_SetString(ipap11helperError, Memory allocation error);
+} else {
+memcpy(result, bytes, *l);
+}
+
}
-Py_XDECREF(utf8_str);
-return bytes;
+Py_DECREF(utf8_str);
+return result;
}
/**
@@ -546,7 +555,7 @@ P11_Helper_finalize(P11_Helper* self) {
CK_RV rv;
if (self-p11 == NULL)
-return Py_None;
+Py_RETURN_NONE;
/*
* Logout
@@ -576,7 +585,7 @@ P11_Helper_finalize(P11_Helper* self) {
self-slot = 0;
self-module_handle = NULL;
-return Py_None;
+Py_RETURN_NONE;
}
/
@@ -636,10 +645,8 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
return NULL;
}
-Py_XINCREF(label_unicode);
CK_BYTE *label = (unsigned char*) unicode_to_char_array(label_unicode,
label_length);
-Py_XDECREF(label_unicode);
CK_MECHANISM mechanism = { //TODO param?
CKM_AES_KEY_GEN, NULL_PTR, 0 };
@@ -688,7 +695,7 @@ P11_Helper_generate_master_key(P11_Helper* self, PyObject *args, PyObject *kwds)
if (!check_return_value(rv, generate master key))
return NULL;
-return Py_BuildValue(k, master_key);;
+return Py_BuildValue(k, master_key);
}
/**
@@ -772,9 +779,7 @@ P11_Helper_generate_replica_key_pair(P11_Helper* self, PyObject *args,
return NULL;
}
-Py_XINCREF(label_unicode);
CK_BYTE *label = unicode_to_char_array(label_unicode, label_length);
-Py_XDECREF(label_unicode);
CK_OBJECT_HANDLE public_key, private_key;
CK_MECHANISM mechanism = {
@@ -887,30 +892,24 @@ P11_Helper_find_keys(P11_Helper* self, PyObject *args, PyObject *kwds) {
}
if (label_unicode != NULL) {
-Py_INCREF(label_unicode);
label = (unsigned char*) unicode_to_char_array(label_unicode,
label_length); //TODO verify signed/unsigned
-Py_DECREF(label_unicode);
}
if (cka_wrap_bool != NULL) {
-Py_INCREF(cka_wrap_bool);
if (PyObject_IsTrue(cka_wrap_bool)) {
ckawrap = true;
} else {
ckawrap = false;
}
-Py_DECREF(cka_wrap_bool);
}
if (cka_unwrap_bool != NULL) {
-Py_INCREF(cka_unwrap_bool);
if (PyObject_IsTrue(cka_unwrap_bool)) {
ckaunwrap = true;
} else {
ckaunwrap = false;
}
-Py_DECREF(cka_unwrap_bool);
}
if (class == CKO_VENDOR_DEFINED)
@@ -940,7 +939,7 @@ P11_Helper_find_keys(P11_Helper* self, PyObject *args, PyObject *kwds) {
free(objects);
return NULL;
}
-Py_INCREF(result_list);
+
for (int i = 0; i objects_len; ++i) {
if (PyList_SetItem(result_list, i, Py_BuildValue(k, objects[i]))
