[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: Daniel Shahaf <danielsh_at_elego.de>
Date: Tue, 24 Jan 2012 13:36:07 +0200

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

Nice log message.

> 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.
>
> 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;
>

As before: this cast shouldn't need to be explicit. Also, I've moved
the two new declarations to a more inner scope.

> 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:

I then ended up with the following patch:

[[[
Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
===================================================================
--- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (revision 1235202)
+++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (working copy)
@@ -657,10 +657,22 @@ static PyObject *convert_rangelist(void *value, vo
   apr_array_header_t *array = 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)
+ PyObject *obj;
+ int result;
+
+ obj = convert_to_swigtype(range, ctx, py_pool);
+ if (obj == NULL)
+ goto error;
+
+ result = PyList_Append(list, obj);
+ Py_DECREF(obj);
+ if (result == -1);
         goto error;
     }
   return list;
]]]

However, with that patch, 'make check-swig-py' fails:

[[[
======================================================================
ERROR: test_mergeinfo_get (mergeinfo.SubversionMergeinfoTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 109, in test_mergeinfo_get
    False, None, None)
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/repos.py", line 650, in svn_repos_fs_get_mergeinfo
    return _repos.svn_repos_fs_get_mergeinfo(*args)
SystemError: error return without exception set

======================================================================
ERROR: Test svn_mergeinfo_merge()
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 75, in test_mergeinfo_merge
    mergeinfo1 = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1)
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse
    return _core.svn_mergeinfo_parse(*args)
SystemError: error return without exception set

======================================================================
ERROR: Test svn_mergeinfo_parse()
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 61, in test_mergeinfo_parse
    mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1)
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse
    return _core.svn_mergeinfo_parse(*args)
SystemError: error return without exception set

======================================================================
ERROR: test_mergeinfo_sort (mergeinfo.SubversionMergeinfoTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 93, in test_mergeinfo_sort
    mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1)
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse
    return _core.svn_mergeinfo_parse(*args)
SystemError: error return without exception set

======================================================================
ERROR: test_rangelist_merge (mergeinfo.SubversionMergeinfoTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 66, in test_rangelist_merge
    mergeinfo1 = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1)
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse
    return _core.svn_mergeinfo_parse(*args)
SystemError: error return without exception set

======================================================================
ERROR: test_rangelist_reverse (mergeinfo.SubversionMergeinfoTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 82, in test_rangelist_reverse
    mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1)
  File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse
    return _core.svn_mergeinfo_parse(*args)
SystemError: error return without exception set

----------------------------------------------------------------------
]]]

and the failure disappears once I revert the patch.

Could you look into that, please? I suspect the C code is buggy, but
I'm not sure whether it's due to your patch or to my mangling of it.

Thanks,

Daniel
Received on 2012-01-24 12:36:56 CET

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