[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix memory leak in SWIG Python bindings

From: Hyrum K Wright <hyrum.wright_at_wandisco.com>
Date: Tue, 24 Jan 2012 09:51:37 +0000

On Tue, Jan 24, 2012 at 5:58 AM, Trent Nelson <trent_at_snakebite.org> wrote:
>
> On Jan 24, 2012, at 12:11 AM, Daniel Shahaf wrote:
>
>> Trent Nelson wrote on Mon, Jan 23, 2012 at 15:07:20 -0500:
>>> Fix a memory leak in convert_rangelist by *always* Py_DECREF'ing the object
>>> returned by convert_to_swigtype (a SWIG wrapper around svn_merge_range_t)
>>> once we've established it's not NULL.
>>>
>>> This is required because PyList_Append takes ownership of references passed
>>> in (i.e. calls Py_INCREF).  Prior to this fix, the reference counts of any
>>
>> Is svn_swig_py_c_strings_to_list() affected too?  It appends a string to
>> a list but doesn't Py_DECREF() the string.
>>
>
> Yeah, I saw that, but scheduled it for another day, as I didn't want to clutter my main patch.  Also, nothing calls that method -- which also factored into my decision to ignore it for this patch.
>
>>> svn_merge_range_t objects would always be off-by-one, preventing them from
>>> ever being garbage collected, having dire effects on the memory usage of
>>> long-running programs calling svn_mergeinfo_parse() on real-world data.
>>>
>>> Patch by: Trent Nelson <trent_at_snakebite.org>
>>> Tested on: FreeBSD, OS X, Windows.
>>>
>>> * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c:
>>>  (convert_rangelist): Make sure we call Py_DECREF on the object returned
>>>   from convert_to_swigtype.  PyList_New might return NULL; check for that.
>>>   Add 'apr_array_header_t *' and 'swig_type_info *' casts where necessary
>>>   to suppress compiler warnings.
>>
>> Those casts shouldn't be needed -- casts from 'void *' to other
>> pointer types are valid C.  What warnings were you seeing?
>
> I'll double-check in the morning.  Side bar: the bindings generate loads of warnings when compiled with the target platform's equivalent of -Wall.

Yes, this is partly due to the bindings themselves, and partly due to
SWIG. Annoying either way.

> I found a bunch of other things whilst traipsing through the SWIG/Python stuff that I'll send follow up patches for this week.  Nothing as bad as the main memory leak, though.

Great! Keep 'em coming!

Though I don't feel qualified to effectively review the patches
(though I suppose I could give it a rough go in a pinch), I am glad
somebody is looking at these problems, and you're making progress.
Thanks for the effort.

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Received on 2012-01-24 10:52:17 CET

This is an archived mail posted to the Subversion Dev mailing list.