Daniel Shahaf wrote on Tue, Jan 24, 2012 at 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:
>
> + if (result == -1);
> goto error;
My version of the patch contained a bug.
When fixed, the memory usage in the pastebin test remained constant, so
I went ahead and applied the C part of the patch in r1235264.
I'll look at the py part next.
Thanks,
Daniel
