[PATCHES] pyldb: reference counting, error handling and future-proofing fixes
Petr Viktorin
pviktori at redhat.com
Thu Feb 26 05:26:18 MST 2015
Hello,
While investigating pyldb, I came across some reference-counting issues,
areas where the error handling could be better, and Python changes that
have been backported to Python 2.6+.
See the patches for more info on the individual changes.
--
Petr Viktorin
-------------- next part --------------
From c2bb47133128a97eebf0d302ca174048e963d69f Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Wed, 25 Feb 2015 18:02:08 +0100
Subject: [PATCH 1/9] pyldb: Correct reference counting when returning bools
Simply returning Py_True/PyFalse doesn't increment the bool object's
reference count.
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index 5bcff72..9bbd4ba 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -457,7 +457,7 @@ static PyObject *py_ldb_dn_check_special(PyLdbDnObject *self, PyObject *args)
if (!PyArg_ParseTuple(args, "s", &name))
return NULL;
- return ldb_dn_check_special(self->dn, name)?Py_True:Py_False;
+ return PyBool_FromLong(ldb_dn_check_special(self->dn, name));
}
static int py_ldb_dn_compare(PyLdbDnObject *dn1, PyLdbDnObject *dn2)
@@ -507,7 +507,7 @@ static PyObject *py_ldb_dn_add_child(PyLdbDnObject *self, PyObject *args)
if (!pyldb_Object_AsDn(NULL, py_other, dn_ldb_ctx(dn), &other))
return NULL;
- return ldb_dn_add_child(dn, other)?Py_True:Py_False;
+ return PyBool_FromLong(ldb_dn_add_child(dn, other));
}
static PyObject *py_ldb_dn_add_base(PyLdbDnObject *self, PyObject *args)
@@ -522,7 +522,7 @@ static PyObject *py_ldb_dn_add_base(PyLdbDnObject *self, PyObject *args)
if (!pyldb_Object_AsDn(NULL, py_other, dn_ldb_ctx(dn), &other))
return NULL;
- return ldb_dn_add_base(dn, other)?Py_True:Py_False;
+ return PyBool_FromLong(ldb_dn_add_base(dn, other));
}
static PyObject *py_ldb_dn_remove_base_components(PyLdbDnObject *self, PyObject *args)
@@ -534,7 +534,7 @@ static PyObject *py_ldb_dn_remove_base_components(PyLdbDnObject *self, PyObject
dn = pyldb_Dn_AsDn((PyObject *)self);
- return ldb_dn_remove_base_components(dn, i)?Py_True:Py_False;
+ return PyBool_FromLong(ldb_dn_remove_base_components(dn, i));
}
static PyObject *py_ldb_dn_is_child_of(PyLdbDnObject *self, PyObject *args)
--
2.1.0
From e3d3ad745249e79daf8741c817287ac65af148a0 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Wed, 25 Feb 2015 18:30:05 +0100
Subject: [PATCH 2/9] pyldb: Use the Py_TYPE macro
The "ob_type" member of Python objects is going away
in Python 3 for compliance with C's strict aliasing rules.
The Py_TYPE macro should be used instead.
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index 9bbd4ba..4f452fa 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -88,7 +88,7 @@ static void py_ldb_control_dealloc(PyLdbControlObject *self)
talloc_free(self->mem_ctx);
}
self->data = NULL;
- self->ob_type->tp_free(self);
+ Py_TYPE(self)->tp_free(self);
}
static PyObject *py_ldb_control_get_oid(PyLdbControlObject *self)
@@ -1993,7 +1993,7 @@ static PyObject *PyLdb_FromLdbContext(struct ldb_context *ldb_ctx)
static void py_ldb_dealloc(PyLdbObject *self)
{
talloc_free(self->mem_ctx);
- self->ob_type->tp_free(self);
+ Py_TYPE(self)->tp_free(self);
}
static PyTypeObject PyLdb = {
@@ -2017,7 +2017,7 @@ static void py_ldb_result_dealloc(PyLdbResultObject *self)
Py_DECREF(self->msgs);
Py_DECREF(self->referals);
Py_DECREF(self->controls);
- self->ob_type->tp_free(self);
+ Py_TYPE(self)->tp_free(self);
}
static PyObject *py_ldb_result_get_msgs(PyLdbResultObject *self, void *closure)
--
2.1.0
From f6f25dc94c9fe78254560fac4d28745a280421df Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Wed, 25 Feb 2015 18:54:39 +0100
Subject: [PATCH 3/9] pyldb: Properly preallocate result control list
The list was always allocated with size 1, so
if there is more than 1 result control, the additional
items would not be added in the list, and would
become leaked references.
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index 4f452fa..9ee71eb 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -269,7 +269,11 @@ static PyObject *PyLdbResult_FromResult(struct ldb_result *result)
ret->msgs = list;
if (result->controls) {
- controls = PyList_New(1);
+ i = 0;
+ while (result->controls[i]) {
+ i++;
+ }
+ controls = PyList_New(i);
if (controls == NULL) {
Py_DECREF(ret);
PyErr_NoMemory();
--
2.1.0
From 96848a1690b37538389282c5e906e2581e672304 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Thu, 26 Feb 2015 10:17:50 +0100
Subject: [PATCH 4/9] pyldb: Remove use of staticforward macro
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index 9ee71eb..67a3577 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -48,7 +48,7 @@ static PyTypeObject PyLdb;
static PyTypeObject PyLdbMessageElement;
#define pyldb_MessageElement_Check(ob) PyObject_TypeCheck(ob, &PyLdbMessageElement)
-staticforward PyTypeObject PyLdbTree;
+static PyTypeObject PyLdbTree;
static PyObject *PyLdb_FromLdbContext(struct ldb_context *ldb_ctx);
static PyObject *PyLdbModule_FromModule(struct ldb_module *mod);
static struct ldb_message_element *PyObject_AsMessageElement(
--
2.1.0
From b1a093e39c3599da010b6335005e980e7092911b Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Thu, 26 Feb 2015 10:34:36 +0100
Subject: [PATCH 5/9] pyldb: Fix reference leaks
The parse_ldif and MessageElement.__iter__ functions leaked references
to intermediate lists whose iterators they return.
The MessageElement repr used the undocumented macro PyObject_REPR, which
leaks references. (It was used internally in CPython before fatal errors,
and will be removed in Python 3.5.)
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index 67a3577..adcde0c 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -1546,7 +1546,7 @@ static PyObject *py_ldb_write_ldif(PyLdbObject *self, PyObject *args)
static PyObject *py_ldb_parse_ldif(PyLdbObject *self, PyObject *args)
{
- PyObject *list;
+ PyObject *list, *ret;
struct ldb_ldif *ldif;
const char *s;
@@ -1573,7 +1573,9 @@ static PyObject *py_ldb_parse_ldif(PyLdbObject *self, PyObject *args)
}
}
talloc_free(mem_ctx); /* The pyobject already has a reference to the things it needs */
- return PyObject_GetIter(list);
+ ret = PyObject_GetIter(list);
+ Py_DECREF(list);
+ return ret;
}
static PyObject *py_ldb_msg_diff(PyLdbObject *self, PyObject *args)
@@ -2450,7 +2452,9 @@ static PyObject *py_ldb_msg_element_iter(PyLdbMessageElementObject *self)
{
PyObject *el = ldb_msg_element_to_set(NULL,
pyldb_MessageElement_AsMessageElement(self));
- return PyObject_GetIter(el);
+ PyObject *ret = PyObject_GetIter(el);
+ Py_DECREF(el);
+ return ret;
}
static PyObject *PyLdbMessageElement_FromMessageElement(struct ldb_message_element *el, TALLOC_CTX *mem_ctx)
@@ -2562,14 +2566,16 @@ static PyObject *py_ldb_msg_element_repr(PyLdbMessageElementObject *self)
char *element_str = NULL;
Py_ssize_t i;
struct ldb_message_element *el = pyldb_MessageElement_AsMessageElement(self);
- PyObject *ret;
+ PyObject *ret, *repr;
for (i = 0; i < el->num_values; i++) {
PyObject *o = py_ldb_msg_element_find(self, i);
+ repr = PyObject_Repr(o);
if (element_str == NULL)
- element_str = talloc_strdup(NULL, PyObject_REPR(o));
+ element_str = talloc_strdup(NULL, PyString_AsString(repr));
else
- element_str = talloc_asprintf_append(element_str, ",%s", PyObject_REPR(o));
+ element_str = talloc_asprintf_append(element_str, ",%s", PyString_AsString(repr));
+ Py_DECREF(repr);
}
if (element_str != NULL) {
--
2.1.0
From 9a5cd884425107ea62e1ba3b9da346b996721c52 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Wed, 25 Feb 2015 18:33:22 +0100
Subject: [PATCH 6/9] pyldb: Type-check arguments parsed with PyArg_ParseTuple*
PyObject* arguments need to be type-checked before they're
cast to subtypes.
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index adcde0c..30cb836 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -121,9 +121,9 @@ static PyObject *py_ldb_control_new(PyTypeObject *type, PyObject *args, PyObject
TALLOC_CTX *mem_ctx;
struct ldb_context *ldb_ctx;
- if (!PyArg_ParseTupleAndKeywords(args, kwargs, "Os",
+ if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O!s",
discard_const_p(char *, kwnames),
- &py_ldb, &data))
+ &PyLdb, &py_ldb, &data))
return NULL;
mem_ctx = talloc_new(NULL);
@@ -2141,9 +2141,9 @@ static PyObject *py_ldb_module_search(PyLdbModuleObject *self, PyObject *args, P
const char * const*attrs;
/* type "int" rather than "enum" for "scope" is intentional */
- if (!PyArg_ParseTupleAndKeywords(args, kwargs, "OiOO",
+ if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O!iOO",
discard_const_p(char *, kwnames),
- &py_base, &scope, &py_tree, &py_attrs))
+ &PyLdbDn, &py_base, &scope, &py_tree, &py_attrs))
return NULL;
mod = self->mod;
@@ -2185,7 +2185,7 @@ static PyObject *py_ldb_module_add(PyLdbModuleObject *self, PyObject *args)
int ret;
struct ldb_module *mod;
- if (!PyArg_ParseTuple(args, "O", &py_message))
+ if (!PyArg_ParseTuple(args, "O!", &PyLdbMessage, &py_message))
return NULL;
req = talloc_zero(NULL, struct ldb_request);
@@ -2207,7 +2207,7 @@ static PyObject *py_ldb_module_modify(PyLdbModuleObject *self, PyObject *args)
PyObject *py_message;
struct ldb_module *mod;
- if (!PyArg_ParseTuple(args, "O", &py_message))
+ if (!PyArg_ParseTuple(args, "O!", &PyLdbMessage, &py_message))
return NULL;
req = talloc_zero(NULL, struct ldb_request);
@@ -2228,7 +2228,7 @@ static PyObject *py_ldb_module_delete(PyLdbModuleObject *self, PyObject *args)
struct ldb_request *req;
PyObject *py_dn;
- if (!PyArg_ParseTuple(args, "O", &py_dn))
+ if (!PyArg_ParseTuple(args, "O!", &PyLdbDn, &py_dn))
return NULL;
req = talloc_zero(NULL, struct ldb_request);
@@ -2248,7 +2248,7 @@ static PyObject *py_ldb_module_rename(PyLdbModuleObject *self, PyObject *args)
struct ldb_request *req;
PyObject *py_dn1, *py_dn2;
- if (!PyArg_ParseTuple(args, "OO", &py_dn1, &py_dn2))
+ if (!PyArg_ParseTuple(args, "O!O!", &PyLdbDn, &py_dn1, &PyLdbDn, &py_dn2))
return NULL;
req = talloc_zero(NULL, struct ldb_request);
--
2.1.0
From ec8f92320a233699e0551dd38682a408cf6baebb Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Thu, 26 Feb 2015 10:20:02 +0100
Subject: [PATCH 7/9] pyldb: Better error reporting
Provide more useful error messages for some type errors
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index 30cb836..a3ffde9 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -1213,7 +1213,7 @@ static struct ldb_message *PyDict_AsMessage(TALLOC_CTX *mem_ctx,
msg_el = PyObject_AsMessageElement(msg->elements, value,
mod_flags, key_str);
if (msg_el == NULL) {
- PyErr_SetString(PyExc_TypeError, "unable to import element");
+ PyErr_Format(PyExc_TypeError, "unable to import element '%s'", key_str);
return NULL;
}
memcpy(&msg->elements[msg_pos], msg_el, sizeof(*msg_el));
@@ -2968,7 +2968,7 @@ static int py_ldb_msg_set_dn(PyLdbMessageObject *self, PyObject *value, void *cl
{
struct ldb_message *msg = pyldb_Message_AsMessage(self);
if (!pyldb_Dn_Check(value)) {
- PyErr_SetNone(PyExc_TypeError);
+ PyErr_SetString(PyExc_TypeError, "expected dn");
return -1;
}
--
2.1.0
From 71abac3d75ece2c9e9a7a32d4f0456a671e7c157 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Wed, 25 Feb 2015 18:59:16 +0100
Subject: [PATCH 8/9] pyldb: Report errors converting controls list to char**
With this change, passing an unexpected type to the CRUD methods
will result in an informative TypeError.
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/pyldb.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/lib/ldb/pyldb.c b/lib/ldb/pyldb.c
index a3ffde9..f18e06e 100644
--- a/lib/ldb/pyldb.c
+++ b/lib/ldb/pyldb.c
@@ -1109,6 +1109,10 @@ static PyObject *py_ldb_modify(PyLdbObject *self, PyObject *args, PyObject *kwar
parsed_controls = NULL;
} else {
const char **controls = PyList_AsStringList(mem_ctx, py_controls, "controls");
+ if (controls == NULL) {
+ talloc_free(mem_ctx);
+ return NULL;
+ }
parsed_controls = ldb_parse_control_strings(ldb_ctx, mem_ctx, controls);
talloc_free(controls);
}
@@ -1254,6 +1258,10 @@ static PyObject *py_ldb_add(PyLdbObject *self, PyObject *args, PyObject *kwargs)
parsed_controls = NULL;
} else {
const char **controls = PyList_AsStringList(mem_ctx, py_controls, "controls");
+ if (controls == NULL) {
+ talloc_free(mem_ctx);
+ return NULL;
+ }
parsed_controls = ldb_parse_control_strings(ldb_ctx, mem_ctx, controls);
talloc_free(controls);
}
@@ -1343,6 +1351,10 @@ static PyObject *py_ldb_delete(PyLdbObject *self, PyObject *args, PyObject *kwar
parsed_controls = NULL;
} else {
const char **controls = PyList_AsStringList(mem_ctx, py_controls, "controls");
+ if (controls == NULL) {
+ talloc_free(mem_ctx);
+ return NULL;
+ }
parsed_controls = ldb_parse_control_strings(ldb_ctx, mem_ctx, controls);
talloc_free(controls);
}
@@ -1417,6 +1429,10 @@ static PyObject *py_ldb_rename(PyLdbObject *self, PyObject *args, PyObject *kwar
parsed_controls = NULL;
} else {
const char **controls = PyList_AsStringList(mem_ctx, py_controls, "controls");
+ if (controls == NULL) {
+ talloc_free(mem_ctx);
+ return NULL;
+ }
parsed_controls = ldb_parse_control_strings(ldb_ctx, mem_ctx, controls);
talloc_free(controls);
}
@@ -1717,6 +1733,10 @@ static PyObject *py_ldb_search(PyLdbObject *self, PyObject *args, PyObject *kwar
parsed_controls = NULL;
} else {
const char **controls = PyList_AsStringList(mem_ctx, py_controls, "controls");
+ if (controls == NULL) {
+ talloc_free(mem_ctx);
+ return NULL;
+ }
parsed_controls = ldb_parse_control_strings(ldb_ctx, mem_ctx, controls);
talloc_free(controls);
}
--
2.1.0
From eb46927fdca037d374a45423ea34cb15f4f0a2ac Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori at redhat.com>
Date: Thu, 26 Feb 2015 13:02:42 +0100
Subject: [PATCH 9/9] pyldb: Add tests for type errors
Signed-off-by: Petr Viktorin <pviktori at redhat.com>
---
lib/ldb/tests/python/api.py | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/lib/ldb/tests/python/api.py b/lib/ldb/tests/python/api.py
index 7f5c504..d101de8 100755
--- a/lib/ldb/tests/python/api.py
+++ b/lib/ldb/tests/python/api.py
@@ -776,6 +776,48 @@ class LdbResultTests(TestCase):
self.assertTrue(found)
+class BadTypeTests(TestCase):
+ def test_control(self):
+ l = ldb.Ldb()
+ self.assertRaises(TypeError, ldb.Control, '<bad type>', 'relax:1')
+ self.assertRaises(TypeError, ldb.Control, ldb, 1234)
+
+ def test_modify(self):
+ l = ldb.Ldb()
+ dn = ldb.Dn(l, 'a=b')
+ m = ldb.Message(dn)
+ self.assertRaises(TypeError, l.modify, '<bad type>')
+ self.assertRaises(TypeError, l.modify, m, '<bad type>')
+
+ def test_add(self):
+ l = ldb.Ldb()
+ dn = ldb.Dn(l, 'a=b')
+ m = ldb.Message(dn)
+ self.assertRaises(TypeError, l.add, '<bad type>')
+ self.assertRaises(TypeError, l.add, m, '<bad type>')
+
+ def test_delete(self):
+ l = ldb.Ldb()
+ dn = ldb.Dn(l, 'a=b')
+ self.assertRaises(TypeError, l.add, '<bad type>')
+ self.assertRaises(TypeError, l.add, dn, '<bad type>')
+
+ def test_rename(self):
+ l = ldb.Ldb()
+ dn = ldb.Dn(l, 'a=b')
+ self.assertRaises(TypeError, l.add, '<bad type>', dn)
+ self.assertRaises(TypeError, l.add, dn, '<bad type>')
+ self.assertRaises(TypeError, l.add, dn, dn, '<bad type>')
+
+ def test_search(self):
+ l = ldb.Ldb()
+ self.assertRaises(TypeError, l.search, base=1234)
+ self.assertRaises(TypeError, l.search, scope='<bad type>')
+ self.assertRaises(TypeError, l.search, expression=1234)
+ self.assertRaises(TypeError, l.search, attrs='<bad type>')
+ self.assertRaises(TypeError, l.search, controls='<bad type>')
+
+
class VersionTests(TestCase):
def test_version(self):
--
2.1.0
More information about the samba-technical
mailing list