git svn clone segmentation faul issue

* git svn clone segmentation faul issue
@ 2016-06-28  8:50 Ioannis.Kappas
  2016-06-28 11:47 ` Johannes Schindelin
  2016-06-28 19:48 ` Eric Wong
  0 siblings, 2 replies; 12+ messages in thread
From: Ioannis.Kappas @ 2016-06-28  8:50 UTC (permalink / raw)
  To: git

Hello,

Git can fail with a "malformed index nnn" error or cause a segmentation fault when executing the "git svn clone" command. 

This is because the git-svn perl script calling into the external subversion library during fetch, wherein a bug exists that can corrupt the Perl Interpreter's stack maintained on the heap. The bug is in a custom swig typemap around the "result_digest" argument which is not handled in a safe manner: An absolute address on the perl stack is calculated beforehand to store the result of svn_swig_pl_from_md5(), but the function can trigger the stack to be reallocated elsewhere in memory (i.e. when trying to grow it but there is not enough space left) thus invaliding the address calculated earlier, causing memory corruption or segmentation fault.

This call is made once for each file retrieved from subversion. The fault is caused when the free available perl stack size is very small at the call, triggering a realloc() moving it elsewhere in memory. In my test environment --MSYS2 on windows 7--, this was caused when the available stack size left was 24 SV* slots. This means that a subversion repository with many file entries has a higher probability to fail while cloning than one with only a few file entries big.

Perl stack trace to the entry point of the offending code is:

        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
        Git::SVN::Ra::gs_do_update(Git::SVN::Ra=HASH(0x60101f4f0), 384103, 384103, Git::SVN=HASH(0x60101fc58), Git::SVN::Fetcher=HASH(0x6010236a0)) called at /usr/share/perl5/site_perl/Git/SVN.pm line 1205
        Git::SVN::do_fetch(Git::SVN=HASH(0x60101fc58), HASH(0x6010e9b20), 384103) called at /usr/share/perl5/site_perl/Git/SVN/Ra.pm line 475
        Git::SVN::Ra::gs_fetch_loop_common(Git::SVN::Ra=HASH(0x60101f4f0), 384103, 384103, ARRAY(0x600788cc8), ARRAY(0x600788ce0)) called at /usr/share/perl5/site_perl/Git/SVN.pm line 179
        Git::SVN::fetch_all("svn") called at /usr/lib/git-core/git-svn line 522
        main::cmd_clone("http://...", "debug") called at /usr/lib/git-core/git-svn line 386
        eval {...} called at /usr/lib/git-core/git-svn line 384

The solution is to change the swig typemap in subversion to delay calculating the stack address where the result is stored after the call to the md5 function:

In subversion's subversion/subversion/bindings/swig/include/svn_types.swg, one can change

From 

%typemap(argout) unsigned char *result_digest {
    %append_output($1);
}

To

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

Swig will translate the above typemap in subversion/subversion/bindings/swig/perl/native/svn_delta.c

From

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

to

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

(The stack address calculation is the results of calling the macro ST(argvi) ).

I am not a Perl expert, but ST(n) = any_function() seems to be a dangerous idiom because of the potential of any_function() to cause a Perl stack reallocation invaliding the address calculated beforehand by ST(n).

The above patch has been tested to work in my codebase both on windows 7 (with MSYS2) and on Arch Linux, where it would have caused a segmentation fault otherwise.

Fortunately, a patch has already been submitted to subversion with (github) revision a074af86c8764404b28ce99d0bedcb668a321408 (at https://github.com/apache/subversion/commit/a074af86c8764404b28ce99d0bedcb668a321408 ) on the trunk to handle this and a couple of other similar cases. But by the looks of it has not been picked up yet in the latest subversion 1.9.4 release or the 1.9.x branch, perhaps because this patch was identified in sanity checks rather than coming out from a perceivable production issue?

Although this issue is not in the Git source code, I thought reporting this error here first since it is Git that stresses the subversion perl bindings in such a level to cause the failure. Perhaps the patch can be applied first in those git binary releases that include subversion (I believe Git for windows is such a case) while in the mean time I submit this case for consideration in the subversion bug tracking system.

Yannis

*********************************************************************************** 
The Royal Bank of Scotland plc. Registered in Scotland No 90312. 
Registered Office: 36 St Andrew Square, Edinburgh EH2 2YB. 
Authorised by the Prudential Regulation Authority and regulated 
by the Financial Conduct Authority and Prudential Regulation Authority. 
The Royal Bank of Scotland N.V. is authorised and regulated by the 
De Nederlandsche Bank and has its seat at Amsterdam, the 
Netherlands, and is registered in the Commercial Register under 
number 33002587. Registered Office: Gustav Mahlerlaan 350, 
Amsterdam, The Netherlands. The Royal Bank of Scotland N.V. and 
The Royal Bank of Scotland plc are authorised to act as agent for each 
other in certain jurisdictions. 

This e-mail message is confidential and for use by the addressee only. 
If the message is received by anyone other than the addressee, please 
return the message to the sender by replying to it and then delete the 
message from your computer. Internet e-mails are not necessarily 
secure. The Royal Bank of Scotland plc and The Royal Bank of Scotland 
N.V. including its affiliates ("RBS group") does not accept responsibility 
for changes made to this message after it was sent. For the protection
of RBS group and its clients and customers, and in compliance with
regulatory requirements, the contents of both incoming and outgoing
e-mail communications, which could include proprietary information and
Non-Public Personal Information, may be read by authorised persons
within RBS group other than the intended recipient(s). 

Whilst all reasonable care has been taken to avoid the transmission of 
viruses, it is the responsibility of the recipient to ensure that the onward 
transmission, opening or use of this message and any attachments will 
not adversely affect its systems or data. No responsibility is accepted 
by the RBS group in this regard and the recipient should carry out such 
virus and other checks as it considers appropriate. 

Visit our website at www.rbs.com 
***********************************************************************************  

^ permalink raw reply	[flat|nested] 12+ messages in thread