Merge pull request #398 from ruby-rice/dev

Fix memory leak with abstract classes. Fixes #397

Author: cfis

rice/detail/Wrapper.ipp

@@ -122,8 +122,9 @@ namespace Rice::detail
     if constexpr (is_complete_v<T>)
     {
       // is_abstract_v requires a complete type, so nest inside is_complete_v.
-      // Deleting an abstract class through a non-virtual destructor is UB.
-      if constexpr (std::is_destructible_v<T> && !std::is_abstract_v<T>)
+      // Deleting an abstract class through a non-virtual destructor is UB,
+      // but it is safe if the destructor is virtual.
+      if constexpr (std::is_destructible_v<T> && (!std::is_abstract_v<T> || std::has_virtual_destructor_v<T>))
       {
         if (this->isOwner_)
         {

test/test_Ownership.cpp

@@ -188,6 +188,36 @@ namespace
     static inline MyCopyableClass* instance_ = nullptr;
   };
 
+  // Abstract class with virtual destructor - deletion through base pointer is safe
+  class AbstractVirtualDtor
+  {
+  public:
+    static inline int destructorCalls = 0;
+
+    static void reset()
+    {
+      destructorCalls = 0;
+    }
+
+    virtual ~AbstractVirtualDtor()
+    {
+      destructorCalls++;
+    }
+
+    virtual int compute() const = 0;
+  };
+
+  class ConcreteChild : public AbstractVirtualDtor
+  {
+  public:
+    int compute() const override { return 42; }
+  };
+
+  AbstractVirtualDtor* createChild()
+  {
+    return new ConcreteChild();
+  }
+
   class OwnerBox;
 
   class AliasBox
@@ -327,6 +357,14 @@ SETUP(Ownership)
     .define_constructor(Constructor<SharedFactory>())
     .define_method("create_owned", &SharedFactory::createOwned, Return().takeOwnership())
     .define_method("get_borrowed", &SharedFactory::getBorrowed);
+
+  define_class<AbstractVirtualDtor>("AbstractVirtualDtor")
+    .define_method("compute", &AbstractVirtualDtor::compute);
+
+  define_class<ConcreteChild, AbstractVirtualDtor>("ConcreteChild");
+
+  define_module("AbstractVirtualDtorTest")
+    .define_module_function("create_child", &createChild, Return().takeOwnership());
 }
 
 TEARDOWN(Ownership)
@@ -541,3 +579,23 @@ TESTCASE(MultipleOwnerReferences)
   result = m.module_eval(code);
   ASSERT_EQUAL(Qfalse, result.value());
 }
+
+TESTCASE(AbstractVirtualDestructor)
+{
+  AbstractVirtualDtor::reset();
+
+  Module m = define_module("TestingModule");
+
+  std::string code = R"(child = AbstractVirtualDtorTest.create_child
+                        child.compute)";
+
+  Object result = m.module_eval(code);
+  ASSERT_EQUAL(detail::To_Ruby<int>().convert(42), result.value());
+
+  // Force GC to clean up the object - destructor should be called
+  // since AbstractVirtualDtor has a virtual destructor
+  rb_gc_start();
+  rb_gc_start();
+
+  ASSERT_EQUAL(1, AbstractVirtualDtor::destructorCalls);
+}