On Tue, Oct 07, 2014 at 01:32:05PM +0200, Pavel Reichl wrote:
Hello,
please see attached patch.
I haven't run the tool which found these leaks as setting it is not IMO
quite so easy, but I have contacted the reporter and asked him if he is
willing to participate at testing.
Thanks!
From 5b38c8e9e6b48cf86a6ed7d58b572710ca5c867a Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Mon, 29 Sep 2014 16:40:53 +0100
Subject: [PATCH] pyhbac,pysss: fix reference leaks
Resolves:
https://fedorahosted.org/sssd/ticket/1195
---
src/python/pyhbac.c | 22 +++++++++++++---------
src/python/pysss.c | 10 +++++++---
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/python/pyhbac.c b/src/python/pyhbac.c
index e9dce9b01b78401516b5c21141be8007b4a94aec..dd345a6eb4db8ac6104251d5b9c8f11a160e280d
100644
--- a/src/python/pyhbac.c
+++ b/src/python/pyhbac.c
@@ -139,14 +139,17 @@ sequence_as_string_list(PyObject *seq, const char *paramname)
utf_item = get_utf8_string(item, p);
if (utf_item == NULL) {
+ Py_DECREF(item);
return NULL;
}
ret[i] = py_strdup(PyString_AsString(utf_item));
Py_DECREF(utf_item);
if (!ret[i]) {
+ Py_DECREF(item);
return NULL;
}
+ Py_DECREF(item);
item is returned from PySequence_GetItem(), the official documentation
says:
PyObject* PySequence_GetItem(PyObject *o, Py_ssize_t i)
Return value: New reference.
So this change is correct.
}
ret[i] = NULL;
@@ -242,10 +245,7 @@ str_concat_sequence(PyObject *seq, const char *delim)
if (item == NULL) goto fail;
part = PyString_AsString(item);
- if (part == NULL) {
- Py_DECREF(item);
- goto fail;
- }
+ if (part == NULL) goto fail;
if (s) {
s = py_strcat_realloc(s, delim);
@@ -260,7 +260,9 @@ str_concat_sequence(PyObject *seq, const char *delim)
}
return s;
+
fail:
+ Py_XDECREF(item);
Same here.
PyMem_Free(s);
return NULL;
}
@@ -269,11 +271,13 @@ fail:
static void
set_hbac_exception(PyObject *exc, struct hbac_info *error)
{
- PyErr_SetObject(exc,
- Py_BuildValue(sss_py_const_p(char, "(i,s)"),
- error->code,
- error->rule_name ? \
- error->rule_name : "no rule"));
+ PyObject *obj;
+
+ obj = Py_BuildValue(sss_py_const_p(char, "(i,s)"), error->code,
+ error->rule_name ? error->rule_name : "no
rule");
+
+ PyErr_SetObject(exc, obj);
+ Py_XDECREF(obj);
This change would be nice to have unit tested, but I didn't see an
obvious way to do so..so I just did a poor man's test:
diff --git a/src/python/pyhbac.c b/src/python/pyhbac.c
index dd345a6..1d3308e 100644
--- a/src/python/pyhbac.c
+++ b/src/python/pyhbac.c
@@ -1604,6 +1604,7 @@ py_hbac_evaluate(HbacRequest *self, PyObject *args)
self->rule_name = NULL;
eres = hbac_evaluate(rules, hbac_req, &info);
+ eres = HBAC_EVAL_ERROR;
switch (eres) {
case HBAC_EVAL_ALLOW:
self->rule_name = sss_python_unicode_from_string(info->rule_name);
This raised an HbacError exception as I expected.
Valgrind didn't show any memory leaks /in the patched code/ before or
after the patch.
All patched code is tested and the changes look good to me.
ACK.