From 73055acd572a458d0a2e90edeedc3027c86cc917 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 3 Sep 2026 10:14:59 -0700 Subject: [PATCH] Forbid setting ivars on Ractor shareable objects Previously we had a check that objects which were shareable, but not frozen on a non-main Ractor would raise an exception if instance variables were read. This prevented a non-main Ractor reading non-shareable objects. This changes the behaviour to forbid all writes to shareable objects (even when unfrozen). This is done by re-purposing the frozen flag on the object shape to reflect (FL_FROZEN || FL_SHAREABLE). With IV writes forbidden on all shareable objects whether or not they are frozen (with the exception of classes and modules), we can also remove the check against reading instance variables, since no unsafe instance variables can be set. [Bug #22291] --- bootstraptest/test_ractor.rb | 51 +++++++++++++++++------------------- eval.c | 1 + hash.c | 1 + inits.c | 1 - internal/inits.h | 3 +++ internal/variable.h | 1 + ractor.c | 23 ++++++++++++---- ractor_sync.c | 3 +-- shape.h | 3 ++- test/ruby/test_env.rb | 31 +++++++++------------- variable.c | 49 +++++++++++++++++++++++----------- vm_insnhelper.c | 2 +- 12 files changed, 98 insertions(+), 71 deletions(-) diff --git a/bootstraptest/test_ractor.rb b/bootstraptest/test_ractor.rb index 4158556c06b..5bead4ba9f6 100644 --- a/bootstraptest/test_ractor.rb +++ b/bootstraptest/test_ractor.rb @@ -888,50 +888,47 @@ assert_equal "can not get unshareable values from instance variables of classes/ end RUBY -# ivar in shareable-objects are not allowed to access from non-main Ractor -assert_equal 'can not access instance variables of shareable objects from non-main Ractors', %q{ +# setting an ivar on a shareable but unfrozen object is not allowed, by instance_variable_set +assert_equal "can't modify instance variables of a shareable Ractor", %q{ shared = Ractor.new{} - shared.instance_variable_set(:@iv, 'str') - - r = Ractor.new shared do |shared| - p shared.instance_variable_get(:@iv) - end begin - r.value - rescue Ractor::RemoteError => e - e.cause.message + shared.instance_variable_set(:@iv, 'str') + rescue Ractor::IsolationError => e + e.message end } -# ivar in shareable-objects are not allowed to access from non-main Ractor, by @iv (get) -assert_equal 'can not access instance variables of shareable objects from non-main Ractors', %q{ +# setting an ivar on a shareable but unfrozen object is not allowed, by @iv = ... +assert_equal "can't modify instance variables of a shareable Ractor", %q{ class Ractor def setup @foo = '' end - - def foo - @foo - end end shared = Ractor.new{} - shared.setup - - r = Ractor.new shared do |shared| - p shared.foo - end begin - r.value - rescue Ractor::RemoteError => e - e.cause.message + shared.setup + rescue Ractor::IsolationError => e + e.message end } -# ivar in shareable-objects are not allowed to access from non-main Ractor, by @iv (set) -assert_equal 'can not access instance variables of shareable objects from non-main Ractors', %q{ +# ivars of a shareable object are frozen, so they can be read from a non-main Ractor +assert_equal 'nil', %q{ + shared = Ractor.new{} + + r = Ractor.new shared do |shared| + shared.instance_variable_get(:@iv) + end + + r.value.inspect +} + +# setting an ivar on a shareable object is not allowed from a non-main Ractor either +assert_equal "can't modify instance variables of a shareable Ractor", %q{ class Ractor def setup @foo = '' @@ -941,7 +938,7 @@ assert_equal 'can not access instance variables of shareable objects from non-ma shared = Ractor.new{} r = Ractor.new shared do |shared| - p shared.setup + shared.setup end begin diff --git a/eval.c b/eval.c index 17a3f2743ff..89460c55e67 100644 --- a/eval.c +++ b/eval.c @@ -82,6 +82,7 @@ ruby_setup(void) rb_w32_init_long_paths(); #endif Init_BareVM(); + Init_default_shapes(); rb_vm_encoded_insn_data_table_init(); Init_enable_box(); Init_vm_objects(); diff --git a/hash.c b/hash.c index bc7bd5b4a36..279f540d379 100644 --- a/hash.c +++ b/hash.c @@ -36,6 +36,7 @@ #include "internal/hash.h" #include "internal/object.h" #include "internal/proc.h" +#include "internal/ractor.h" #include "internal/st.h" #include "internal/symbol.h" #include "internal/thread.h" diff --git a/inits.c b/inits.c index e4323cc4a84..95ef7402edf 100644 --- a/inits.c +++ b/inits.c @@ -20,7 +20,6 @@ static void Init_builtin_prelude(void); void rb_call_inits(void) { - CALL(default_shapes); CALL(Thread_Mutex); CALL(RandomSeedCore); CALL(encodings); diff --git a/internal/inits.h b/internal/inits.h index be73dac1dcb..4fcced17426 100644 --- a/internal/inits.h +++ b/internal/inits.h @@ -29,6 +29,9 @@ int Init_enc_set_filesystem_encoding(void); /* newline.c */ void Init_newline(void); +/* shape.c */ +void Init_default_shapes(void); + /* vm.c */ void Init_BareVM(void); void Init_vm_objects(void); diff --git a/internal/variable.h b/internal/variable.h index 47d4c86090f..2c832819671 100644 --- a/internal/variable.h +++ b/internal/variable.h @@ -50,6 +50,7 @@ void rb_obj_replace_fields(VALUE obj, VALUE fields_obj); VALUE rb_obj_complex_fields_build(VALUE obj); VALUE rb_obj_field_get(VALUE obj, shape_id_t target_shape_id); void rb_ivar_set_internal(VALUE obj, ID id, VALUE val); +void rb_check_ivar_modifiable(VALUE obj); void rb_ivar_foreach_buffered(VALUE obj, int (*func)(ID name, VALUE val, st_data_t arg), st_data_t arg); attr_index_t rb_ivar_set_index(VALUE obj, ID id, VALUE val); attr_index_t rb_obj_field_set(VALUE obj, shape_id_t target_shape_id, ID field_name, VALUE val); diff --git a/ractor.c b/ractor.c index 3c2b54425ce..110294a94a3 100644 --- a/ractor.c +++ b/ractor.c @@ -715,8 +715,7 @@ ractor_alloc(VALUE klass) { rb_ractor_t *r; VALUE rv = TypedData_Make_Struct(klass, rb_ractor_t, &ractor_data_type, r); - FL_SET_RAW(rv, RUBY_FL_SHAREABLE); - rb_gc_obj_became_shareable(rv); + RB_OBJ_SET_SHAREABLE(rv); r->pub.self = rv; r->next_ec_serial = 1; VM_ASSERT(ractor_status_p(r, ractor_created)); @@ -831,8 +830,7 @@ void rb_ractor_main_setup(rb_vm_t *vm, rb_ractor_t *r, rb_thread_t *th) { VALUE rv = r->pub.self = TypedData_Wrap_Struct(rb_cRactor, &ractor_data_type, r); - FL_SET_RAW(r->pub.self, RUBY_FL_SHAREABLE); - rb_gc_obj_became_shareable(r->pub.self); + RB_OBJ_SET_SHAREABLE(r->pub.self); ractor_init(r, Qnil, Qnil); r->threads.main = th; rb_ractor_living_threads_insert(r, th); @@ -1469,6 +1467,16 @@ rb_obj_set_shareable_no_assert(VALUE obj) FL_SET_RAW(obj, FL_SHAREABLE); rb_gc_obj_became_shareable(obj); + /* Ivars on a shareable object would be mutable shared state, so freeze them + * (not obj itself). A T_IMEMO has no shape id to transition. */ + bool froze_ivars = false; + if (!RB_OBJ_FROZEN_RAW(obj) && !RB_TYPE_P(obj, T_IMEMO) && + !RB_TYPE_P(obj, T_CLASS) && !RB_TYPE_P(obj, T_MODULE) && !RB_TYPE_P(obj, T_ICLASS)) { + + RBASIC_SET_SHAPE_ID(obj, rb_shape_transition_frozen(RBASIC_SHAPE_ID(obj))); + froze_ivars = true; + } + /* A T_OBJECT can have a fields imemo too (too_complex and friends), and an imemo * born while its owner was unshareable stays unshareable * (imemo_fields_complex_from_obj), so align it here. */ @@ -1481,6 +1489,8 @@ rb_obj_set_shareable_no_assert(VALUE obj) // no recursive mark FL_SET_RAW(fields, FL_SHAREABLE); rb_gc_obj_became_shareable(fields); + // the imemo carries its owner's shape id, frozen bit included + if (froze_ivars) RBASIC_SET_SHAPE_ID(fields, RBASIC_SHAPE_ID(obj)); // Field values the traversal never reaches (hidden internal ivars, say) // can stay unshareable, so record their shrefs to keep the shareable // fields imemo's edges correct. @@ -3422,9 +3432,12 @@ ractor_native_shallow_copy(VALUE obj) } /* The traversal rewrites the children inside the copy with raw stores, so the frozen - * bit can be set now: by the time leave runs the original is out of sight. */ + * bit can be set now: by the time leave runs the original is out of sight. The shape + * has to be transitioned along with the flag, because field writes are refused based + * on the shape (see rb_check_ivar_modifiable). */ if (OBJ_FROZEN(obj)) { RB_FL_SET_RAW(copy, RUBY_FL_FREEZE); + RBASIC_SET_SHAPE_ID(copy, rb_obj_shape_transition_frozen(copy)); } return copy; } diff --git a/ractor_sync.c b/ractor_sync.c index 3eb83a6550b..047ea540b53 100644 --- a/ractor_sync.c +++ b/ractor_sync.c @@ -1016,8 +1016,7 @@ rb_ractor_setup_default_port(rb_ractor_t *r) { VM_ASSERT(r->sync.default_port_value == Qfalse); r->sync.default_port_value = ractor_port_new(r); - FL_SET_RAW(r->sync.default_port_value, RUBY_FL_SHAREABLE); // only default ports are shareable - rb_gc_obj_became_shareable(r->sync.default_port_value); + RB_OBJ_SET_SHAREABLE(r->sync.default_port_value); } // Ractor#value diff --git a/shape.h b/shape.h index 2a416b1b126..de83d06b876 100644 --- a/shape.h +++ b/shape.h @@ -29,7 +29,8 @@ STATIC_ASSERT(shape_id_num_bits, SHAPE_ID_NUM_BITS == sizeof(shape_id_t) * CHAR_ // 26 SHAPE_ID_FL_COMPLEX // The object is backed by a `st_table`. // 27 SHAPE_ID_FL_FROZEN -// Whether the object is frozen or not. +// Whether field writes are refused: either the object is frozen, or it +// became shareable while unfrozen (see rb_obj_set_shareable in ractor.c). // 28 SHAPE_ID_FL_HAS_OBJECT_ID // Whether the object has an `SHAPE_OBJ_ID` transition. // 29-30 SHAPE_ID_LAYOUT_MASK diff --git a/test/ruby/test_env.rb b/test/ruby/test_env.rb index 48409f84356..3e928a5b52b 100644 --- a/test/ruby/test_env.rb +++ b/test/ruby/test_env.rb @@ -1474,25 +1474,13 @@ class TestEnv < Test::Unit::TestCase end; end - def test_ivar_in_env_should_not_be_access_from_non_main_ractors + def test_ivar_in_env_is_not_allowed + # ENV is shareable but can never be frozen, so it may not carry instance + # variables at all: they would be unshareable values reachable from any ractor. assert_ractor <<~RUBY - ENV.instance_eval{ @a = "hello" } - assert_equal "hello", ENV.instance_variable_get(:@a) - - r_get = Ractor.new do - ENV.instance_variable_get(:@a) - rescue Ractor::IsolationError => e - e - end - assert_equal Ractor::IsolationError, r_get.value.class - - r_get = Ractor.new do - ENV.instance_eval{ @a } - rescue Ractor::IsolationError => e - e - end - - assert_equal Ractor::IsolationError, r_get.value.class + assert_raise(Ractor::IsolationError) { ENV.instance_eval{ @a = "hello" } } + assert_nil ENV.instance_variable_get(:@a) + assert_equal [], ENV.instance_variables r_set = Ractor.new do ENV.instance_eval{ @b = "hello" } @@ -1501,6 +1489,13 @@ class TestEnv < Test::Unit::TestCase end assert_equal Ractor::IsolationError, r_set.value.class + + # Reads are allowed: since writes are forbidden, there is nothing + # unshareable to read. + r_get = Ractor.new do + ENV.instance_variable_get(:@a) + end + assert_nil r_get.value RUBY end diff --git a/variable.c b/variable.c index 9eab776a554..a5e5d465fc5 100644 --- a/variable.c +++ b/variable.c @@ -30,6 +30,7 @@ #include "internal/object.h" #include "internal/gc.h" #include "internal/re.h" +#include "internal/string.h" #include "internal/struct.h" #include "internal/symbol.h" #include "internal/thread.h" @@ -1280,19 +1281,16 @@ cvar_read_ractor_check(VALUE klass, ID id, VALUE val) } static inline void -ivar_ractor_check(VALUE obj, ID id) +ivar_ractor_assert(VALUE obj, ID id) { - if (LIKELY(rb_is_instance_id(id)) /* not internal ID */ && - !RB_OBJ_FROZEN_RAW(obj) && - UNLIKELY(!rb_ractor_main_p()) && - UNLIKELY(rb_ractor_shareable_p(obj))) { - - if (RB_TYPE_P(obj, T_CLASS) || RB_TYPE_P(obj, T_MODULE)) { - // classes/modules are owner-checked at each read/write site instead - return; - } - rb_raise(rb_eRactorIsolationError, "can not access instance variables of shareable objects from non-main Ractors"); - } + RUBY_ASSERT(!rb_is_instance_id(id) /* internal ID */ || + SPECIAL_CONST_P(obj) || + !rb_ractor_shareable_p(obj) || + RB_OBJ_FROZEN_RAW(obj) || + RB_TYPE_P(obj, T_CLASS) || RB_TYPE_P(obj, T_MODULE) || + RB_TYPE_P(obj, T_ICLASS) || RB_TYPE_P(obj, T_IMEMO) || + rb_shape_frozen_p(RBASIC_SHAPE_ID(obj)), + "shareable object must not have writable instance variables"); } struct st_table * @@ -1378,7 +1376,7 @@ obj_use_generic_fields_tbl_p(VALUE obj) VALUE rb_obj_fields(VALUE obj, ID field_name) { - ivar_ractor_check(obj, field_name); + ivar_ractor_assert(obj, field_name); switch (BUILTIN_TYPE(obj)) { case T_IMEMO: @@ -1467,7 +1465,7 @@ rb_free_generic_ivar(VALUE obj) static void rb_obj_set_fields(VALUE obj, VALUE fields_obj, ID field_name, VALUE original_fields_obj) { - ivar_ractor_check(obj, field_name); + ivar_ractor_assert(obj, field_name); if (!fields_obj) { RUBY_ASSERT(original_fields_obj); @@ -2035,6 +2033,25 @@ obj_ivar_set(VALUE obj, ID id, VALUE val) return obj_field_set(obj, target_shape_id, id, val); } +void +rb_check_ivar_modifiable(VALUE obj) +{ + if (UNLIKELY(!RB_FL_ABLE(obj) || rb_shape_frozen_p(RBASIC_SHAPE_ID(obj)))) { + rb_check_frozen(obj); + + RUBY_ASSERT(RB_OBJ_SHAREABLE_P(obj), "unfrozen object with a frozen shape must be shareable"); + + rb_raise(rb_eRactorIsolationError, + "can't modify instance variables of a shareable %"PRIsVALUE, + rb_obj_class(obj)); + } + else if (UNLIKELY(CHILLED_STRING_P(obj))) { + CHILLED_STRING_MUTATED(obj); + } + + RUBY_ASSERT(!RB_OBJ_FROZEN_RAW(obj), "frozen object with an unfrozen shape"); +} + /* Set the instance variable +val+ on object +obj+ at ivar name +id+. * This function only works with T_OBJECT objects, so make sure * +obj+ is of type T_OBJECT before using this function. @@ -2042,7 +2059,7 @@ obj_ivar_set(VALUE obj, ID id, VALUE val) VALUE rb_vm_set_ivar_id(VALUE obj, ID id, VALUE val) { - rb_check_frozen(obj); + rb_check_ivar_modifiable(obj); obj_ivar_set(obj, id, val); return val; } @@ -2101,7 +2118,7 @@ ivar_set(VALUE obj, ID id, VALUE val) VALUE rb_ivar_set(VALUE obj, ID id, VALUE val) { - rb_check_frozen(obj); + rb_check_ivar_modifiable(obj); ivar_set(obj, id, val); return val; } diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 1f9d8acc805..026b93a6f5a 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -1394,7 +1394,7 @@ vm_setivar_slowpath(VALUE obj, ID id, VALUE val, const rb_iseq_t *iseq, IVC ic, #if OPT_IC_FOR_IVAR RB_DEBUG_COUNTER_INC(ivar_set_ic_miss); - rb_check_frozen(obj); + rb_check_ivar_modifiable(obj); shape_id_t previous_shape_id = RBASIC_SHAPE_ID(obj); attr_index_t index = rb_ivar_set_index(obj, id, val);