[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Segfault in ruby tests

From: David James <james82_at_gmail.com>
Date: Tue, 17 Feb 2009 08:27:10 -0800

On Mon, Feb 16, 2009 at 10:30 PM, Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
>
> On Thu, Jan 29, 2009 at 10:27 AM, Hyrum K. Wright
> <hyrum_wright_at_mail.utexas.edu> wrote:
> > Joe Swatosh wrote:
> >>
>
> blah, blah
>
> >> I'm not much help. Maybe try running the tests with a -v so we will
> >> at least know the name of the test that causes the segfault?
> >
> > hwright_at_orac:~/dev/svn-trunk/subversion/bindings/swig/ruby$ /usr/bin/ruby -I
> > /home/hwright/dev/svn-trunk/subversion/bindings/swig/ruby
> > /home/hwright/dev/svn-trunk/subversion/bindings/swig/ruby/test/run-test.rb
> > -v
> > Loaded suite .
> > Started
> > ...
> > test_apply(SvnDeltaTest):
> > /home/hwright/dev/svn-trunk/subversion/bindings/swig/ruby/svn/util.rb:86:
> > [BUG] Segmentation fault
> > ruby 1.8.7 (2008-08-11 patchlevel 72) [x86_64-linux]
> >
> > The buildbot is back up, too, so you can see the error in the binding test
> > output.
>
> Since the tests are failing on the build bot again, I just wanted to
> put this note out there (I don't know what to do with it yet):
>
> [[[
> D:\SVN\src-trunk>svn log -c21423
> ------------------------------------------------------------------------
> r21423 | djames | 2006-09-11 09:48:15 -0700 (Mon, 11 Sep 2006) | 32 lines
>
> Fix segfault in Python tests by deleting the broken
> argout typemap for the 'result_digest' parameter in
> svn_txdelta_apply. Instead, ignore the parameter
> altogether.
>
> svn_txdelta_apply should not return 'result_digest'
> as a String, because the result digest is not ready
> yet when svn.delta.tx_apply is finished. Instead,
> we should probably allocate a StringIO buffer,
> so that the user can check the return values as
> needed.
>
> (FIXME: We should define a compatibility policy for
> the SWIG bindings. svn_txdelta_apply is not the only
> function whose interface will need to be changed
> to make it actually work [i.e., not segfault]. If
> we actually made an "official list" of the tested
> APIs, with documentation, it would probably be quite
> useful for both users and developers. I'm +1 on
> defining any semi-broken APIs, such as this one,
> as "experimental, and subject to change".)
>
>
> * subversion/bindings/swig/include/svn_types.swg
> (result_digest): Remove typemap. Instead, ignore
> the parameter.
>
> * subversion/bindings/swig/python/tests/delta.py:
> Adjust to compensate for removed 'digest'
> parameter.

Hi Joe,

Thanks for bringing up this history! svn_txdelta_apply has an unusual
API so it is difficult to wrap.
Here's more details, which I added in r21423 to a comment in svn_types.swg

   'unsigned char *result_digest'
     This parameter serves as a buffer for the digest results of
     svn_txdelta_apply. As you supply data to this function via
     the handler callback, this digest gets filled. Since
     filling of this buffer is delayed, it is quite difficult
     to wrap.

    FIXME: Both Ruby and Perl have broken typemaps for result_digest.
    The Python bindings ignore this parameter altogether.

Looking in svn_types.swg, you can still see my warning about the Ruby
typemap being broken:

/* FIXME: This typemap doesn't work, because svn_txdelta_apply saves
 * away this local pointer to be used later. When the pointer
 * is finally used, we get memory corruption / segfaults.
 */
%apply unsigned char digest[ANY] { unsigned char *result_digest };

To be clear, the issue here is that the argument that the argument
that the Ruby bindings pass in to svn_txdelta_apply has the wrong
lifetime. svn_txdelta_apply saves the pointer to result_digest away in
the hander_baton, and expects that this pointer will still be valid
later when the handler function is invoked. However, the pointer we
pass in is a pointer to a variable on the stack, and therefore it will
no longer be valid when svn_txdelta_apply exits.

Fixing this issue properly is a little complex. For the digest
functionality to work, the caller to svn_txdelta_apply should allocate
the memory for holding the digest from a pool that lives just as long
as the delta window handler. Further, the Ruby bindings would need to
be given a pointer to this memory so that they can access the digest
after it has been set by the window handler.

In the Python bindings, you can see that I bypassed the problem by
teaching the bindings to ignore the parameter altogether (see r21423).
I justified this compatibility break on the basis that the function
was previously completely broken due to the segfault issue. Hopefully,
in the future, someone will design a better fix that will allow the
md5 digest to be accessed.

What do you think the Ruby bindings should do?

Cheers,

David

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1179700
Received on 2009-02-17 17:27:33 CET

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.