ruby-rice/rice

Issue when "Array" is returned from an extension with "keepAlive" applied

Closed this issue · 6 comments

maxirmx commented

Hello, Rice developers.

Thank you very much for making a great library. It has always worked like charm after reading documentation but now I am facing an issue that I cannot explain.
This example is a minimized version of data structure generated by https://github.com/camertron/antlr4-native-rb[antlr4-native] gem

This code implements method(s) that return arrays of items created by and inside C++ extension like getPets and getPets2 functions. In my example getPets returns Rice::Array and getPets2 returns wrapped std::vector.
Both functions work until I apply Return.keepAlive(). With keepAlive the function that returns Rice::Array crashes

Any help will be much appreciated

Extension:

#include "ruby.h"
#include <iostream>
#include <vector>
#include <rice/rice.hpp>
#include <rice/stl.hpp>

using namespace Rice;

VALUE rb_mRtest;

class Animal {
public:
  virtual char const * speak() = 0;
  virtual char const * name() = 0;
};

class AnimalProxy {
  Animal* orig;
public:
  AnimalProxy(Animal * o) : orig(o) { }
  char const * name()  {  return orig == nullptr ? "Noname" : orig->name();  }
  char const * speak() {  return orig == nullptr ? "..." : orig->speak();    }
};

class Bear : public Animal {
public:
  char const * name() override   {  return "Bear";  }
  char const * speak() override  {  return "I'm smarter than the average bear";  }
};

class Dog : public Animal {
public:
  char const * name() override   { return "Dog";        }
  char const * speak() override  { return "Woof woof";  }
};

class Rabbit: public Animal {
public:
  char const * name() override    { return "Rabbit";            }
  char const * speak() override   { return "What's up, doc?";   }
};


class Zoo {
  std::vector<Animal*> pets;
  public:
    Zoo(void)  {
      pets.push_back(new Bear);
      pets.push_back(new Dog);
      pets.push_back(new Rabbit);
    }

    ~Zoo() {
      for(auto pet : pets) {
        delete pet;
      }
      pets.clear();
    }

    void visit(AnimalProxy& ani)   {
      std::cout << "A " <<  ani.name() << " says: " << ani.speak() << std::endl;
    }

    Object getPets(void)    {
      Array aPets;
      for(auto p: pets) {
        aPets.push(AnimalProxy(p));
      }
      return aPets;
    }

    Object getPets2(void)  {
      std::vector<AnimalProxy> vPets;
      for(auto p: pets) {
        vPets.push_back(AnimalProxy(p));
      }
      return detail::To_Ruby<std::vector<AnimalProxy>>().convert(vPets);
    }
};

extern "C"
void Init_rtest(void)
{
    rb_mRtest = rb_define_module("Rtest");

    Data_Type<Animal> rb_cAnimal = define_class_under<AnimalProxy>(rb_mRtest, "Animal")
     .define_method("name", &AnimalProxy::name)
     .define_method("speak", &AnimalProxy::speak);

    define_class_under<Bear, Animal>(rb_mRtest, "Bear")
     .define_constructor(Constructor<Bear>());

    define_class_under<Dog, Animal>(rb_mRtest, "Dog")
     .define_constructor(Constructor<Dog>());

    define_class_under<Rabbit, Animal>(rb_mRtest, "Rabbit")
     .define_constructor(Constructor<Rabbit>());

    define_class_under<Zoo>(rb_mRtest, "Zoo")
     .define_constructor(Constructor<Zoo>())
     .define_method("get_pets", &Zoo::getPets, Return().keepAlive())
     .define_method("get_pets2", &Zoo::getPets2, Return().keepAlive())
     .define_method("visit", &Zoo::visit);

    define_vector<std::vector<AnimalProxy>>("AnimalVector");
}

Spec:

RSpec.describe Rtest do
  it "returns wrapped vector" do
    GC.stress = true
    zoo = Rtest::Zoo.new
    pets = zoo.get_pets2
    pets.each do |pet|
      zoo.visit(pet)
    end
    GC.stress = false
  end

  it "returns an Array" do
    GC.stress = true
    zoo = Rtest::Zoo.new
    pets = zoo.get_pets
    pets.each do |pet|
      zoo.visit(pet)
    end
    GC.stress = false
  end
end

Crash dump

Rtest
A Bear says: I'm smarter than the average bear
A Dog says: Woof woof
A Rabbit says: What's up, doc?
  returns wrapped vector
A Bear says: I'm smarter than the average bear
A Dog says: Woof woof
/home/maxirmx/Projects/rtest/spec/rtest_spec.rb:19: [BUG] Segmentation fault at 0x0000563cccb21010
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]

-- Control frame information -----------------------------------------------
c:0030 p:---- s:0148 e:000147 CFUNC  :visit
c:0029 p:0006 s:0143 e:000142 BLOCK  /home/maxirmx/Projects/rtest/spec/rtest_spec.rb:19 [FINISH]
c:0028 p:---- s:0139 e:000138 CFUNC  :each
c:0027 p:0024 s:0135 e:000134 BLOCK  /home/maxirmx/Projects/rtest/spec/rtest_spec.rb:18 [FINISH]
c:0026 p:---- s:0130 e:000129 CFUNC  :instance_exec
c:0025 p:0022 s:0125 e:000124 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:263
c:0024 p:0002 s:0120 e:000119 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511
c:0023 p:0002 s:0117 e:000116 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468
c:0022 p:0002 s:0114 e:000113 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486
c:0021 p:0018 s:0111 e:000110 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:624
c:0020 p:0104 s:0104 e:000103 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486
c:0019 p:0018 s:0097 e:000096 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468
c:0018 p:0019 s:0092 e:000091 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511
c:0017 p:0076 s:0087 e:000086 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:259
c:0016 p:0037 s:0080 e:000079 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:646 [FINISH]
c:0015 p:---- s:0074 e:000073 CFUNC  :map
c:0014 p:0011 s:0070 e:000069 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642
c:0013 p:0052 s:0065 e:000064 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:607
c:0012 p:0007 s:0056 e:000055 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121 [FINISH]
c:0011 p:---- s:0052 e:000051 CFUNC  :map
c:0010 p:0030 s:0048 e:000047 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121
c:0009 p:0026 s:0045 e:000044 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/configuration.rb:2070
c:0008 p:0007 s:0041 e:000040 BLOCK  /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:116
c:0007 p:0009 s:0037 e:000036 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/reporter.rb:74
c:0006 p:0019 s:0032 e:000031 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:115
c:0005 p:0035 s:0025 e:000024 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:89
c:0004 p:0058 s:0019 e:000018 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:71
c:0003 p:0013 s:0011 e:000010 METHOD /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:45
c:0002 p:0010 s:0006 e:000005 EVAL   /home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/exe/rspec:4 [FINISH]
c:0001 p:0000 s:0003 E:000ab0 DUMMY  [FINISH]

-- Ruby level backtrace information ----------------------------------------
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/exe/rspec:4:in `<main>'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:45:in `invoke'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:71:in `run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:89:in `run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:115:in `run_specs'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/reporter.rb:74:in `report'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:116:in `block in run_specs'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/configuration.rb:2070:in `with_suite_hooks'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `map'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:607:in `run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642:in `run_examples'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642:in `map'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:646:in `block in run_examples'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:259:in `run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511:in `with_around_and_singleton_context_hooks'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468:in `with_around_example_hooks'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486:in `run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486:in `block in run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468:in `block in with_around_example_hooks'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511:in `block in with_around_and_singleton_context_hooks'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:263:in `block in run'
/home/maxirmx/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:263:in `instance_exec'
/home/maxirmx/Projects/rtest/spec/rtest_spec.rb:18:in `block (2 levels) in <top (required)>'
/home/maxirmx/Projects/rtest/spec/rtest_spec.rb:18:in `each'
/home/maxirmx/Projects/rtest/spec/rtest_spec.rb:19:in `block (3 levels) in <top (required)>'
/home/maxirmx/Projects/rtest/spec/rtest_spec.rb:19:in `visit'

-- Machine register context ------------------------------------------------
 RIP: 0x0000563cccb21010 RBP: 0x00007ffd2d18c9e0 RSP: 0x00007ffd2d18c9c8
 RAX: 0x0000563cccf93760 RBX: 0x00007f7401cbfa08 RCX: 0x0000000000000000
 RDX: 0x0000563cccb21010 RDI: 0x0000563cccf93760 RSI: 0x0000000000000020
  R8: 0x00007f740223dbb0  R9: 0x0000563cccb257e0 R10: 0x00007f740684ffe0
 R11: 0x000000000000000c R12: 0x000000000000000b R13: 0x0000563ccd0de508
 R14: 0x00007f7401cbff08 R15: 0x0000563cccb257e0 EFL: 0x0000000000010202

-- C level backtrace information -------------------------------------------
/home/maxirmx/.rbenv/versions/3.2.2/lib/libruby.so.3.2(rb_print_backtrace+0x11) [0x7f740748b521] vm_dump.c:785
/home/maxirmx/.rbenv/versions/3.2.2/lib/libruby.so.3.2(rb_vm_bugreport) vm_dump.c:1080
/home/maxirmx/.rbenv/versions/3.2.2/lib/libruby.so.3.2(rb_bug_for_fatal_signal+0xf4) [0x7f7407280d94] error.c:813
/home/maxirmx/.rbenv/versions/3.2.2/lib/libruby.so.3.2(sigsegv+0x4d) [0x7f74073db49d] signal.c:964
/lib/x86_64-linux-gnu/libpthread.so.0(__restore_rt+0x0) [0x7f7406ed1420] ../sysdeps/pthread/funlockfile.c:28
[0x563cccb21010]

Output withous GC.stress = true

test
A Bear says: I'm smarter than the average bear
A Dog says: Woof woof
A Rabbit says: What's up, doc?
  returns wrapped vector
A Bear says: I'm smarter than the average bear
A Dog says: Woof woof
  returns an Array (FAILED - 1)

Failures:

  1) Rtest returns an Array
     Failure/Error: zoo.visit(pet)
     
     TypeError:
       wrong argument type ??UH??H?? H?}?H?E?H???&???H?E?H? (expected 
                                                                      )
     # ./spec/rtest_spec.rb:19:in `visit'
     # ./spec/rtest_spec.rb:19:in `block (3 levels) in <top (required)>'
     # ./spec/rtest_spec.rb:18:in `each'
     # ./spec/rtest_spec.rb:18:in `block (2 levels) in <top (required)>'

Finished in 0.00212 seconds (files took 0.10228 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/rtest_spec.rb:14 # Rtest returns an Array
jasonroelofs commented

I wonder if there's something going on between the interplay of Animal and AnimalProxy. Why do you have the proxy object? Does this still crash if you return lists of Animal itself?

maxirmx commented

Hi, @jasonroelofs
Thank you for looking at this issue.

Let me explain a background. I am trying to stabilize expressir gem. It has native extension is C++ generated by antlr4-native gem that uses Rice gem to wrap code generated by antlr4 parser generator.
These are ~20K lines of generated C++ code that keeps crashing unpredictably.

I have AnimalProxy in the sample because it resembles the structure of production code. There Animal is a virtual base class with its own hierarchy on top and AnimalProxy is a decorator that adds some functionality.

If I remove AnimalProxy and return Animal itself it still crashes.

maxirmx commented

If I can make a suggestion regarding the crash, here it follows:

  template<typename From_Ruby_T, typename Function_T, bool IsMethod>
  void NativeFunction<From_Ruby_T, Function_T, IsMethod>::checkKeepAlive(VALUE self, VALUE returnValue, std::vector<VALUE>& rubyValues)
  {
    // Check function arguments
    Wrapper* selfWrapper = getWrapper(self);
    for (const Arg& arg : (*this->methodInfo_))
    {
      if (arg.isKeepAlive())
      {
        selfWrapper->addKeepAlive(rubyValues[arg.position]);
      }
    }

    // Check return value
    if (this->methodInfo_->returnInfo.isKeepAlive())
    {
      Wrapper* returnWrapper = getWrapper(returnValue);
      returnWrapper->addKeepAlive(self);
    }
  }

The function above calls getWrapper and getWrapper is just

  inline Wrapper* getWrapper(VALUE value)
  {
    return static_cast<Wrapper*>(RTYPEDDATA_DATA(value));
  }

However, in my sample returnValue is an Array (built-in type), not wrapped type so the cast above will have unpredictable results.

jasonroelofs commented

I've played around with your example here and looked more into what you're doing and the intended use of Return, and I don't think you need to use Return for either of these methods, particularly get_pets because every time it's called you are creating a new Array anyway. Leaving out Return, or making it more explicit with Return.takeOwnership() seem to work fine and do not cause memory leaks that I can find.

Granted, it probably shouldn't crash at all, but I would need to look further into the nitty gritty of what's being stored here to figure out what's getting messed up (I'm guessing a value isn't getting marked for GC and is getting prematurely freed).

Possibly something else is going on with the expressir gem? Do you have an example crash you're trying to fix?

maxirmx commented

Thank you, @jasonroelofs

  1. Every time I call get_pets it returns an Array that refers objects "embedded" into Zoo object. That's why I would like to mark Zoo object from the object retuned by get_pets. In other words I am not concerned that the Array would be prematurely garbade collected. I am concerned that Zoo is destroyed but there will be references to its internal in Ruby.
    It may look like overkill for this sample but I am trying to create simple case from a very large extension (~20 000 lines)

  2. It crashes because getWrapper function assumes that the return value is wrapped type and behaves incorrectly if return value is native type. Here is a PR that adresses this issue #194
    I cannot say that it is a real fix but at least it throws an exception and does not crash.

  3. You are right, there is something else happeninng in expressir gem.
    I was trying to keep Array alive but after looking at comments here b487179
    I think that there may be "entries in the array are getting GC'd"

jasonroelofs commented

Fixed with #194