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.
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.
>
>> Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
>> ===================================================================
>> --- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
>> (revision 1234786)
>> +++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
>> (working copy)
>> @@ -653,15 +653,27 @@
>> static PyObject *convert_rangelist(void *value, void *ctx, PyObject *py_pool)
>> {
>> int i;
>> + int result;
>> + PyObject *obj;
>> PyObject *list;
>> - apr_array_header_t *array = value;
>> + apr_array_header_t *array = (apr_array_header_t *)value;
>>
>> list = PyList_New(0);
>> + if (list == NULL)
>> + return NULL;
>> +
>> for (i = 0; i < array->nelts; i++)
>> {
>> svn_merge_range_t *range = APR_ARRAY_IDX(array, i, svn_merge_range_t
>> *);
>> - if (PyList_Append(list, convert_to_swigtype(range, ctx, py_pool)) ==
>> -1)
>> +
>> + obj = convert_to_swigtype(range, (swig_type_info *)ctx, py_pool);
>> + if (obj == NULL)
>> goto error;
>> +
>> + result = PyList_Append(list, obj);
>> + Py_DECREF(obj);
>> + if (result == -1)
>> + goto error;
>> }
>> return list;
>> error:
