[SSSD] [PATCH] pyhbac,pysss: fix reference leaks

Jakub Hrozek jhrozek at redhat.com
Wed Oct 22 13:40:25 UTC 2014


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 at 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.



More information about the sssd-devel mailing list