Re: Segfault in Perl bindings when commit touches a large number of files

James McCoy <jamessan_at_debian.org> writes:

> On Wed, Mar 18, 2015 at 02:49:08PM +0000, Philip Martin wrote:
>> I also see that. Given that Perl has realloc'd some memory I suppose
>> it might be a reference counting bug in Subversion's XS code.
>>
>> Or perhaps the bug might be this code in svn_types.swg:
>>
>> %typemap(argout) unsigned char *result_digest {
>> /* FIXME: This code is clearly buggy. The return value of sv_newmortal()
>> is immediately overwritten by the return value
>> of svn_swig_pl_from_md5(). */
>> ST(argvi) = sv_newmortal();
>> ST(argvi++) = svn_swig_pl_from_md5($1);
>> }
>> #endif
>
> Indeed. Reading through some Perl documentation, the original commit,
> and svn_swig_pl_from_md5, I don't see why the svn_newmortal() call is
> there. svn_swig_pl_from_md5 already makes its return value mortal, so
> this is just allocating and overwriting a new SV*.
>
> Removing this resolves the crash and reduces the amount of
> (de)allocations.

Can you show us exactly what you changed. I tried this:

Index: ../src-1.9/subversion/bindings/swig/include/svn_types.swg
===================================================================
--- ../src-1.9/subversion/bindings/swig/include/svn_types.swg (revision 1668117)
+++ ../src-1.9/subversion/bindings/swig/include/svn_types.swg (working copy)
@@ -1119,7 +1119,6 @@
   /* FIXME: This code is clearly buggy. The return value of sv_newmortal()
      is immediately overwritten by the return value
      of svn_swig_pl_from_md5(). */
- ST(argvi) = sv_newmortal();
     ST(argvi++) = svn_swig_pl_from_md5($1);
 }
 #endif

and the crash still occurs in my 1.9 dev build.

I'm not familiar with this code, but looking at other code in the file I
tried this:

Index: ../src-1.9/subversion/bindings/swig/include/svn_types.swg
===================================================================
--- ../src-1.9/subversion/bindings/swig/include/svn_types.swg (revision 1668117)
+++ ../src-1.9/subversion/bindings/swig/include/svn_types.swg (working copy)
@@ -1119,8 +1119,7 @@
   /* FIXME: This code is clearly buggy. The return value of sv_newmortal()
      is immediately overwritten by the return value
      of svn_swig_pl_from_md5(). */
- ST(argvi) = sv_newmortal();
- ST(argvi++) = svn_swig_pl_from_md5($1);
+ %append_output(svn_swig_pl_from_md5($1));
 }
 #endif
 
That does allow my dev build to run the clone for the first few
revisions, including the huge starting revision.

With that the generated code looks like:

    ST(argvi) = sv_newmortal();
    {
      /* FIXME: This code is clearly buggy. The return value of sv_newmortal()
           is immediately overwritten by the return value
           of svn_swig_pl_from_md5(). */
      if (argvi >= items) EXTEND(sp,1); ST(argvi) = svn_swig_pl_from_md5(arg3); argvi++ ;
    }

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*