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

[Michael Ströder]

Hi Bradley and David,

thanks for your analysis and experiments with PyPy.

I'm not familiar with all this. If you prepare a patch (until tomorrow
evening) I will test and commit it but I have currently no time during the
next weeks to dive into that.

Ciao, Michael.

David Malcolm wrote:
> 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
>