Re: Segfault in ruby tests

On Tue, Feb 17, 2009 at 8:27 AM, David James <james82_at_gmail.com> wrote:
> 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?
>

I have 3 ideas:

1) Follow the precedent of the Python bindings and just remove the digest.
2) Create an appropriately sized buffer in a member variable and pass
it into Delta.txdelta_apply_wrapper (changing SWIG to use it instead
of creating one)
3) Use kou's idea from the client context, create a pool associated
with the object and allocate the buffer with that instead of on the
stack.

I'm thinking 1 is quickest and has a precedent. 2 and 3 might be over my head.

I'll try to produce a patch that Hyrum can try for 1.

--
Joe
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1187944