[python-ldap] Suspected memory leak in Modules/errors.c:LDAPerr

[David Malcolm]

On Wed, 2011-12-07 at 15:11 -0700, Bradley McCandless wrote:

> Just removing the DECREF on line on line 129 of errors.c solves the
> issue.

I believe Bradley is reporting on the PyPy issue from this thread:
  http://mail.python.org/pipermail/python-ldap/2011q4/003070.html
Bradley, am I correct?

(sorry, I'm not subscribed to this list).

> http://fedorapeople.org/~dmalcolm/gcc-python-plugin/2011-12-07/errors.c.LDAPinit_errors-refcount-errors.html

The above is another output file from my experimental refcount-checking
tool ([1] below), which I posted to #pypy on Freenode, where we were
discussing the PyPy issue.

(I had to comment out the majority of the seterrobj() macro invocations
in order to workaround a bug in my tool)

> From #pypy.irc.freenode.net:
> 
> 
> exarkun: brads: So, you can point out that errors.c:129 should not be
> Py_DECREF'ing the exception class, because the module re-uses the
> exception class later on.
> 
> exarkun: brads: And it only works on CPython by accident

The code is assuming that the references within the dictionary are going
to keep the LDAPexception_class object alive.  This works in CPython,
but fails under PyPy; my understanding is that this is because PyPy
implements Python.h as a series of thin proxy objects around its "real"
object implementations, and the proxy object for the exception gets
cleaned up when the DECREF happens.

Given that LDAPexception_class is a global, it makes sense to own a
reference to it.

(The tool is also reporting on some possible crashes calling
PyDict_SetItemString with NULL, when running under low memory
conditions)

Hope this is helpful.
Dave

> -brad
> 
> 
> On Wed, Dec 7, 2011 at 2:41 PM, David Malcolm <dmalcolm at redhat.com>
> wrote:
>         I'm running an experimental static analysis tool [1] over
>         python-ldap,
>         and it discovered what looks like a real reference leak:
>         
>         Modules/errors.c: In function ‘LDAPerr’,
>         if it executes this code:
>                else
>                        PyErr_SetObject(LDAPexception_class,
>                            Py_BuildValue("{s:i}", "errnum", errnum));
>         
>         then Py_BuildValue returns a new dictionary with refcount 1,
>         owned by
>         the caller; PyErr_SetObject adds a new ref; the first ref is
>         leaked;
>         hence the dictionary is leaked every time
>         
>         HTML version of the above attached.
>         
>         Having said that, it looks like this branch is only followed
>         when
>         receiving an unexpected error ID.
>         
>         Hope this is helpful
>         Dave
>         
>         
>         [1]
>         http://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html