From e22fc73c66b478a19930788b7d23c6ea48b4bdec Mon Sep 17 00:00:00 2001
From: Jean Boussier <jean.boussier@gmail.com>
Date: Fri, 13 Jun 2025 14:25:42 +0200
Subject: [PATCH] Fix a race condition in object_id for shareable objects

If an object is shareable and has no capacity left, it isn't
safe to store the object ID in fields as it requires an object
resize which can't be done unless all field reads are synchronized.

In this very specific case we create the object_id in advance,
before the object is made shareable.
---
 ractor.c                    | 23 +++++++++--
 test/ruby/test_object_id.rb | 77 +++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/ractor.c b/ractor.c
index 2b9d5b3d5b..bd26c7739d 100644
--- a/ractor.c
+++ b/ractor.c
@@ -1357,9 +1357,26 @@ make_shareable_check_shareable(VALUE obj)
         }
     }
 
-    if (RB_TYPE_P(obj, T_IMEMO)) {
-        return traverse_skip;
-    }
+    switch (TYPE(obj)) {
+        case T_IMEMO:
+          return traverse_skip;
+        case T_OBJECT:
+          {
+              // If a T_OBJECT is shared and has no free capacity, we can't safely store the object_id inline,
+              // as it would require to move the object content into an external buffer.
+              // This is only a problem for T_OBJECT, given other types have external fields and can do RCU.
+              // To avoid this issue, we proactively create the object_id.
+              shape_id_t shape_id = RBASIC_SHAPE_ID(obj);
+              attr_index_t capacity = RSHAPE_CAPACITY(shape_id);
+              attr_index_t free_capacity = capacity - RSHAPE_LEN(shape_id);
+              if (!rb_shape_has_object_id(shape_id) && capacity && !free_capacity) {
+                  rb_obj_id(obj);
+              }
+          }
+          break;
+        default:
+          break;
+    } 
 
     if (!RB_OBJ_FROZEN_RAW(obj)) {
         rb_funcall(obj, idFreeze, 0);
diff --git a/test/ruby/test_object_id.rb b/test/ruby/test_object_id.rb
index 44421ea256..018cc81496 100644
--- a/test/ruby/test_object_id.rb
+++ b/test/ruby/test_object_id.rb
@@ -198,3 +198,80 @@ class TestObjectIdTooComplexGeneric < TestObjectId
     end
   end
 end
+
+class TestObjectIdRactor < Test::Unit::TestCase
+  def test_object_id_race_free
+    assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
+    begin;
+      Warning[:experimental] = false
+      class MyClass
+        attr_reader :a, :b, :c
+        def initialize
+          @a = @b = @c = nil
+        end
+      end
+      N = 10_000
+      objs = Ractor.make_shareable(N.times.map { MyClass.new })
+      results = 4.times.map{
+        Ractor.new(objs) { |objs|
+          vars = []
+          ids = []
+          objs.each do |obj|
+            vars << obj.a << obj.b << obj.c
+            ids << obj.object_id
+          end
+          [vars, ids]
+        }
+      }.map(&:value)
+      assert_equal 1, results.uniq.size
+    end;
+  end
+
+  def test_external_object_id_ractor_move
+    assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
+    begin;
+      Warning[:experimental] = false
+      class MyClass
+        attr_reader :a, :b, :c
+        def initialize
+          @a = @b = @c = nil
+        end
+      end
+      obj = Ractor.make_shareable(MyClass.new)
+      object_id = obj.object_id
+      obj = Ractor.new { Ractor.receive }.send(obj, move: true).value
+      assert_equal object_id, obj.object_id
+    end;
+  end
+
+  def test_object_id_race_free_with_stress_compact
+    assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
+    begin;
+      Warning[:experimental] = false
+      class MyClass
+        attr_reader :a, :b, :c
+        def initialize
+          @a = @b = @c = nil
+        end
+      end
+      N = 20
+      objs = Ractor.make_shareable(N.times.map { MyClass.new })
+
+      GC.stress = true
+      GC.auto_compact = true if GC.respond_to?(:auto_compact=)
+
+      results = 4.times.map{
+        Ractor.new(objs) { |objs|
+          vars = []
+          ids = []
+          objs.each do |obj|
+            vars << obj.a << obj.b << obj.c
+            ids << obj.object_id
+          end
+          [vars, ids]
+        }
+      }.map(&:value)
+      assert_equal 1, results.uniq.size
+    end;
+  end
+end