Merge pull request #387 from ruby-rice/dev

Dev

Author: cfis

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+Incompatible Changes:
+* `InstanceRegistry.isEnabled` (boolean) has been replaced by an `InstanceRegistry.isEnabled` which is an enum (`Off`, `Owned`, `All`).
+* `InstanceRegistry` now defaults to `Owned` - previously it was disabled. The goal of this change is to ensure C++ objects owned by Ruby are only wrapped once to avoid double free errors.
+
 ## 4.10.0 (2026-02-07)
 
 Enhancements:

docs/bindings/constructors.md

@@ -83,6 +83,10 @@ class Mat
 end
 ```
 
+### KeepAlive
+
+When an object is cloned or duped, Rice automatically copies its [keepAlive](keep_alive.md) references to the new object. This ensures that the clone directly protects the same Ruby objects as the original. For example, if a `std::vector<MyClass*>` holds pointers to Ruby-wrapped objects, cloning the vector will ensure those objects are kept alive by both the original and the clone independently.
+
 ## Move Constructors
 
 Rice does not support move constructors because there is no clean mapping of them to Ruby.

docs/bindings/instance_registry.md

@@ -4,17 +4,20 @@ Rice 4.1 added an instance registry which tracks which C++ objects have been wra
 
 ## Enabled
 
-When the instance registry is enabled, Rice will check if a C++ instance has been wrapped by a Ruby instance. If it has, then the existing Ruby instance is returned.
+When instance tracking is enabled (that is, mode is not `Off`), Rice will check if a C++ instance has already been wrapped by Ruby. If it has, then the existing Ruby instance is returned.
 
-By default, instance tracking is disabled. To turn it on:
+Instance tracking is configured only via mode:
 
 ```cpp
-detail::Internal::intance.instances.isEnabled = true;
+detail::Registries::instance.instances.mode = detail::InstanceRegistry::Mode::Owned; // default
+// Off   -> disable tracking
+// Owned -> track only Ruby-owned wrapped instances
+// All   -> track both Ruby-owned and borrowed instances
 ```
 
 ## Disabled
 
-When the instance registry is disabled, Rice will wrap a C++ instance in a new Ruby instance regardless of whether it is already wrapped by a Ruby instance. Therefore if you make multiple calls to a C++ method that returns the same C++ object each time via a reference or pointer, multiple wrapping Ruby objects will be created. By default having multiple Ruby objects wrap a C++ object is fine since the Ruby objects do not own the C++ object. For more information please carefully read the [C++ to Ruby](memory_management.md#c-to-ruby) topic.
+When mode is `Off`, Rice will wrap a C++ instance in a new Ruby instance regardless of whether it is already wrapped by a Ruby instance. Therefore if you make multiple calls to a C++ method that returns the same C++ object each time via a reference or pointer, multiple wrapping Ruby objects will be created. By default having multiple Ruby objects wrap a C++ object is fine since the Ruby objects do not own the C++ object. For more information please carefully read the [C++ to Ruby](memory_management.md#c-to-ruby) topic.
 
 There is one exception to this rule, which happens when a C++ method returns back itself. Rice recognizes that the C++ object is wrapped by the Ruby object making the call, and thus it is returning self (see [return self](methods.md#return-self)).
 
@@ -29,3 +32,11 @@ First, the implementation is not thread-safe. Due to Ruby's GIL, this is not con
 Second, pairs in the global map are removed when a Ruby object is freed by the garbage collector. There could be a window where a Ruby object is marked for deletion but the underlying C++ object is returned back to Ruby. Then the Ruby object would be freed resulting in a crash. It is unknown if this really happens, it has never been observed.
 
 Third, a C++ instance wrapped by Ruby instance may be freed on the C++ side. As long as ownership rules have been correctly setup, this is fine. However, if a new C++ instance is created that has the same address as the deleted C++ object and then is passed to Ruby the instance tracker will return the old deleted object. This has been observed in the Rice tests. It is unknown if this is due to how the tests are written or is a more general problem.
+
+## Rule for Ruby-Owned Objects
+
+A useful practical rule is: if Ruby owns a C++ object, there should be only one Ruby object wrapping that C++ instance. Multiple Ruby wrappers for the same Ruby-owned C++ object can create lifetime bugs where one wrapper frees the underlying C++ object while another wrapper still exists and later dereferences a dangling pointer.
+
+The instance registry addresses this by mapping C++ instance addresses to existing Ruby wrappers and reusing the same Ruby object instead of creating duplicates. In other words, when ownership is on the Ruby side, enabling instance tracking helps enforce a single-wrapper identity and avoids this class of aliasing/lifetime errors.
+
+Value-return wrappers do not participate in instance tracking. When C++ returns values, Rice copies or moves them into wrapper-owned storage, so there is no stable shared native address identity to reuse. Instance tracking is therefore only meaningful for pointer/reference-style aliasing cases, not value copies.

rice/Constructor.ipp

@@ -45,11 +45,14 @@ namespace Rice
       detail::wrapConstructed<T>(self, Data_Type<T>::ruby_data_type(), data);
     }
 
-    static void initialize_copy(VALUE self, const T& other)
+    // Takes VALUE (via Arg.setValue()) instead of const T& so we have access to
+    // both Ruby wrappers and can copy the keepAlive list from the original to the clone.
+    static VALUE initialize_copy(VALUE self, VALUE other)
     {
-      // Call C++ copy constructor
-      T* data = new T(other);
-      detail::wrapConstructed<T>(self, Data_Type<T>::ruby_data_type(), data);
+      T* otherData = detail::unwrap<T>(other, Data_Type<T>::ruby_data_type(), false);
+      T* data = new T(*otherData);
+      detail::wrapConstructed<T>(self, Data_Type<T>::ruby_data_type(), data, other);
+      return self;
     }
 
   };

rice/Data_Type.ipp

@@ -192,8 +192,23 @@ namespace Rice
 
     if constexpr (Constructor_T::isCopyConstructor())
     {
-      // Define initialize_copy that will copy the C++ object
-      this->define_method("initialize_copy", &Constructor_T::initialize_copy, args...);
+      // Define initialize_copy that will copy the C++ object and its keepAlive references.
+      // We use setValue() so Rice passes the raw VALUE without conversion - this gives
+      // initialize_copy access to both wrappers so it can copy the keepAlive list.
+      using Rice_Arg_Tuple = std::tuple<Rice_Arg_Ts...>;
+      constexpr std::size_t arg_index = detail::tuple_element_index_v<Rice_Arg_Tuple, Arg>;
+
+      if constexpr (arg_index < sizeof...(Rice_Arg_Ts))
+      {
+        // User provided an Arg - extract it and ensure setValue is set
+        Arg arg = std::get<arg_index>(std::forward_as_tuple(args...));
+        arg.setValue();
+        this->define_method("initialize_copy", &Constructor_T::initialize_copy, arg);
+      }
+      else
+      {
+        this->define_method("initialize_copy", &Constructor_T::initialize_copy, Arg("other").setValue());
+      }
     }
     else if constexpr (Constructor_T::isMoveConstructor())
     {

rice/detail/InstanceRegistry.hpp

@@ -1,29 +1,37 @@
 #ifndef Rice__detail__InstanceRegistry__hpp_
 #define Rice__detail__InstanceRegistry__hpp_
 
+#include <type_traits>
+
 namespace Rice::detail
 {
   class InstanceRegistry
   {
   public:
+    enum class Mode
+    {
+      Off,
+      Owned,
+      All
+    };
+
     template <typename T>
-    VALUE lookup(T& cppInstance);
+    VALUE lookup(T* cppInstance, bool isOwner);
 
     template <typename T>
-    VALUE lookup(T* cppInstance);
-    VALUE lookup(void* cppInstance);
+    void add(T* cppInstance, VALUE rubyInstance, bool isOwner);
 
-    void add(void* cppInstance, VALUE rubyInstance);
     void remove(void* cppInstance);
     void clear();
 
   public:
-    bool isEnabled = false;
+    Mode mode = Mode::Owned;
 
   private:
+    bool shouldTrack(bool isOwner) const;
+
     std::map<void*, VALUE> objectMap_;
   };
 } // namespace Rice::detail
 
 #endif // Rice__detail__InstanceRegistry__hpp_
-

rice/detail/InstanceRegistry.ipp

@@ -3,39 +3,26 @@
 namespace Rice::detail
 {
   template <typename T>
-  inline VALUE InstanceRegistry::lookup(T& cppInstance)
+  inline VALUE InstanceRegistry::lookup(T* cppInstance, bool isOwner)
   {
-    return this->lookup((void*)&cppInstance);
-  }
-
-  template <typename T>
-  inline VALUE InstanceRegistry::lookup(T* cppInstance)
-  {
-    return this->lookup((void*)cppInstance);
-  }
-
-  inline VALUE InstanceRegistry::lookup(void* cppInstance)
-  {
-    if (!this->isEnabled)
-      return Qnil;
-
-    auto it = this->objectMap_.find(cppInstance);
-    if (it != this->objectMap_.end())
-    {
-      return it->second;
-    }
-    else
+    if (!this->shouldTrack(isOwner))
     {
       return Qnil;
     }
+
+    auto it = this->objectMap_.find((void*)cppInstance);
+    return it != this->objectMap_.end() ? it->second : Qnil;
   }
 
-  inline void InstanceRegistry::add(void* cppInstance, VALUE rubyInstance)
+  template <typename T>
+  inline void InstanceRegistry::add(T* cppInstance, VALUE rubyInstance, bool isOwner)
   {
-    if (this->isEnabled)
+    if (!this->shouldTrack(isOwner))
     {
-      this->objectMap_[cppInstance] = rubyInstance;
+      return;
     }
+
+    this->objectMap_[(void*)cppInstance] = rubyInstance;
   }
 
   inline void InstanceRegistry::remove(void* cppInstance)
@@ -47,4 +34,19 @@ namespace Rice::detail
   {
     this->objectMap_.clear();
   }
-} // namespace
+
+  inline bool InstanceRegistry::shouldTrack(bool isOwner) const
+  {
+    switch (this->mode)
+    {
+      case Mode::Off:
+        return false;
+      case Mode::Owned:
+        return isOwner;
+      case Mode::All:
+        return true;
+      default:
+        return false;
+    }
+  }
+}

rice/detail/Wrapper.hpp

@@ -17,6 +17,8 @@ namespace Rice::detail
 
     void ruby_mark();
     void addKeepAlive(VALUE value);
+    const std::vector<VALUE>& getKeepAlive() const;
+    void setKeepAlive(const std::vector<VALUE>& keepAlive);
     void setOwner(bool value);
 
   protected:
@@ -37,7 +39,7 @@ namespace Rice::detail
   public:
     Wrapper(rb_data_type_t* rb_data_type, T& data);
     Wrapper(rb_data_type_t* rb_data_type, T&& data);
-    ~Wrapper();
+    ~Wrapper() = default;
     void* get(rb_data_type_t* requestedType) override;
 
   private:
@@ -82,7 +84,7 @@ namespace Rice::detail
 
   // ---- Helper Functions ---------
   template <typename T>
-  Wrapper<T*>* wrapConstructed(VALUE value, rb_data_type_t* rb_data_type, T* data);
+  Wrapper<T*>* wrapConstructed(VALUE value, rb_data_type_t* rb_data_type, T* data, VALUE source = Qnil);
 
   template <typename T>
   VALUE wrap(VALUE klass, rb_data_type_t* rb_data_type, T& data, bool isOwner);
@@ -99,4 +101,4 @@ namespace Rice::detail
   WrapperBase* getWrapper(VALUE value);
 }
 #endif // Rice__detail__Wrapper__hpp_
- 
+