On Tue, Jan 24, 2012 at 5:58 AM, Trent Nelson <tr...@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 <tr...@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/
