Request to pick up fix with revision 1683266 (swig perl typemap update) with next subversion release

[Ioannis Kappas <io...@rbs.com>]

Hello,

Can you please kindly consider picking up the fix with revision 1683266 
(http://svn.apache.org/viewvc?view=revision&revision=1683266) in the 
next patch/official subversion release? 

It fixes a long standing issue in the subversion Perl bindings, whereby 
a client stressing the interface can cause a Perl stack 
corruption/segfault in the Swig wrapper to subversion.

The patch fixes, at least, the Swig typemap definition around the 
“result _digest” argument, so that it is handled in a safe manner: 
instead of calculating beforehand the Perl stack address where the 
result of the svn_swig_pl_from_md5() is going to be stored, is rather 
calculated after the function has been called. The md5 function has the 
potential to reallocate the Perl stack elsewhere in memory, so any stack 
addresses calculated before the function call can become invalid after 
the call. 

In my use case, I have git calling the subversion perl binding to 
retrieve the files from a moderately large subversion repository, 
whereby I am getting consistent segmentation faults at that point where 
svn_txdelta_apply() is being called:

        SVN::TxDelta::apply(GLOB(0x60107ece8), GLOB(0x6011ee3a0), undef, 
SVN::Pool=REF(0x6011ee268)) called at 
/usr/share/perl5/site_perl/Git/SVN/Fetcher.pm line 368
        
Git::SVN::Fetcher::apply_textdelta(Git::SVN::Fetcher=HASH(0x6010236a0), 
HASH(0x6011ee2e0), undef, _p_apr_pool_t=SCALAR(0x60113e510)) called at 
/usr/lib/perl5/vendor_perl/SVN/Ra.pm line 623
        
SVN::Ra::Reporter::AUTOLOAD(SVN::Ra::Reporter=ARRAY(0x6010e9ee0), 
SVN::Pool=REF(0x6010ea228)) called at 
/usr/share/perl5/site_perl/Git/SVN/Ra.pm 
line 312

Swig, taking in account the result_digest typemap, generates the 
following code in the wrapper _wrap_svn_txdelta_apply function at 
subversion/subversion/bindings/swig/perl/native/svn_delta.c:

      if (argvi >= items) EXTEND(sp,1);  ST(argvi) = 
svn_swig_pl_from_md5(arg3); argvi++  ;

When the available perl stack size happens to be at  24 SV* just before 
the call  on my system (MSYS2 on windows 7), the svn_swig_pl_from_md5 
will cause reallocation of the perl stack to a different memory address. 
The perl stack address calculated beforehand with the ST(argvi) macro 
will not be valid any more after the call, thus causing a segmentation 
fault when trying to write the result of the function there. The same 
fault happens when I try the same job on Arch Linux, thus it is also 
reproducible across platforms.

The fix makes sure that the result of svn_swig_pl_from_md5 is captured 
in a temporary variable, and stored in ST(argvi) just afterwards:

%typemap(argout) unsigned char *result_digest {
    SV *tmp;
    PUTBACK;
    tmp = svn_swig_pl_from_md5($1);
    SPAGAIN;
    %append_output(tmp);
}

I have tested the fix to work in my case.

A more detailed analysis of the issue with regards to this particular 
client can be found at http://www.mail-
archive.com/git@vger.kernel.org/msg97227.html .

Thank you,
Yannis

Re: Request to pick up fix with revision 1683266 (swig perl typemap update) with next subversion release

[Stefan <lu...@posteo.de>]

On 7/1/2016 07:11, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, Jun 30, 2016 at 22:26:31 +0200:
>> It just needs one more vote from a committer to make it in,
>> and no vetoes.
> It needs no more votes: bindings changes require one +1 and one +0, and
> r1683266 has that.  You can move the change to the "Approved" section.
>
> This means r1683266 will, barring unexpectancies, be included in 1.9.5.

I took the liberty and moved the nomination to the approved section.

Regards,
Stefan

Re: Request to pick up fix with revision 1683266 (swig perl typemap update) with next subversion release

[Daniel Shahaf <d....@daniel.shahaf.name>]

Stefan Sperling wrote on Thu, Jun 30, 2016 at 22:26:31 +0200:
> It just needs one more vote from a committer to make it in,
> and no vetoes.

It needs no more votes: bindings changes require one +1 and one +0, and
r1683266 has that.  You can move the change to the "Approved" section.

This means r1683266 will, barring unexpectancies, be included in 1.9.5.

Re: Request to pick up fix with revision 1683266 (swig perl typemap update) with next subversion release

[Stefan <lu...@posteo.de>]

On 6/30/2016 22:26, Stefan Sperling wrote:
> On Thu, Jun 30, 2016 at 11:21:30AM +0000, Ioannis Kappas wrote:
>> Hello,
>>
>> Can you please kindly consider picking up the fix with revision 1683266 
>> (http://svn.apache.org/viewvc?view=revision&revision=1683266) in the 
>> next patch/official subversion release? 
> It has been nominated for 1.9.5. I can't guarantee the fix will
> be in 1.9.5 but I don't see a reason why it shouldn't make it.
> It just needs one more vote from a committer to make it in,
> and no vetoes.

I'm not an expert with SWIG and Perl, but from a plain code change point
of view, this fix seems suspicious to also be applicable to the
1.8-branch, isn't it?

Regards,
Stefan

Re: Request to pick up fix with revision 1683266 (swig perl typemap update) with next subversion release

[Stefan Sperling <st...@elego.de>]

On Thu, Jun 30, 2016 at 11:21:30AM +0000, Ioannis Kappas wrote:
> Hello,
> 
> Can you please kindly consider picking up the fix with revision 1683266 
> (http://svn.apache.org/viewvc?view=revision&revision=1683266) in the 
> next patch/official subversion release? 

It has been nominated for 1.9.5. I can't guarantee the fix will
be in 1.9.5 but I don't see a reason why it shouldn't make it.
It just needs one more vote from a committer to make it in,
and no vetoes.