[PATCH] pyhbac,pysss: fix reference leaks

[Pavel Reichl]

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!

[Lukas Slebodnik]

On (07/10/14 13:32), 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.

If you cannot test with gcc plugin could you write a unit test?

LS

[Jakub Hrozek]

On Tue, Oct 07, 2014 at 01:36:07PM +0200, Lukas Slebodnik wrote:
> On (07/10/14 13:32), 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.
>
> If you cannot test with gcc plugin could you write a unit test?
>
> LS

I thought that pyhbac was one of the rare parts of SSSD with a nice test
coverage? Can you test reference leaks at all?

[Lukas Slebodnik]

On (07/10/14 17:06), Jakub Hrozek wrote:
> On Tue, Oct 07, 2014 at 01:36:07PM +0200, Lukas Slebodnik wrote:
> > On (07/10/14 13:32), 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.
> >
> > If you cannot test with gcc plugin could you write a unit test?
> >
> > LS
>
> I thought that pyhbac was one of the rare parts of SSSD with a nice test
> coverage?

and it will not catch the problem.
> Can you test reference leaks at all?

It needn't be part of our test suite. It can be simple python script which call
problematic funcions(methods) in infinite loop. Without patch, there should be
a leak.

LS

[Jakub Hrozek]

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

[Jakub Hrozek]

On Wed, Oct 22, 2014 at 03:40:25PM +0200, Jakub Hrozek wrote:
> 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 
> > 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.

* master: c52d7c41e4127a84f487777c0efa6996f6389c51