diff --git a/bootstraptest/test_ractor.rb b/bootstraptest/test_ractor.rb index 3fb76118aea..09b9f56b1b5 100644 --- a/bootstraptest/test_ractor.rb +++ b/bootstraptest/test_ractor.rb @@ -869,8 +869,8 @@ assert_equal '99', %q{ Ractor.new { inner = 99; eval("inner").to_s }.value } -# ivar in shareable-objects are not allowed to access from non-main Ractor -assert_equal "can not get unshareable values from instance variables of classes/modules from non-main Ractors (@iv from C)", <<~'RUBY', frozen_string_literal: false +# ivar in shareable-objects are not allowed to access from non-owner Ractor +assert_equal "can not get unshareable values from instance variables of classes/modules created by another Ractor (@iv from C)", <<~'RUBY', frozen_string_literal: false class C @iv = 'str' end @@ -1175,8 +1175,97 @@ assert_equal 'true', %q{ r.value } +# A shareable object belongs to no single Ractor, so its singleton class is the +# main Ractor's rather than the one which materialized it +assert_equal 'true', %q{ + r = Ractor.new do + begin + Ractor.main.define_singleton_method(:zzz) { :sub } + :not_raised + rescue Ractor::IsolationError + :raised + end + end + raised = r.value + Ractor.main.define_singleton_method(:zzz) { :main } + (raised == :raised && Ractor.main.zzz == :main).to_s +} + +# The constant inline cache is keyed on the Ractor which filled it, so a non-owner +# can not be handed an unshareable constant through a cache the owner primed +assert_equal 'Ractor::IsolationError', %q{ + port = Ractor::Port.new + r = Ractor.new(port) do |port| + k = Class.new + k.const_set(:X, [1, 2, 3]) + m = Module.new + m.const_set(:K, k) + m.module_eval("def self.rd = K::X") + m.rd # the owner fills the cache + port << m + Ractor.receive + end + m = port.receive + res = begin + m.rd + 'no error' + rescue Ractor::IsolationError + 'Ractor::IsolationError' + end + r << nil + r.join + res +} + +# Class#initialize writes the superclass of an uninitialized class, so it is +# owner-only like any other modification +assert_equal 'can not modify K because it is created by another Ractor', %q{ + K = Class.allocate + + r = Ractor.new { K.send(:initialize, Struct) } + + begin + r.join + rescue Ractor::RemoteError => e + e.cause.message + end +} + +# Module#refine writes its refinement tables into the receiver, so the receiver +# must be owned too +assert_equal 'can not modify M because it is created by another Ractor', %q{ + module M; end + + r = Ractor.new do + M.send(:refine, Class.new) { def z = 1 } + end + + begin + r.join + rescue Ractor::RemoteError => e + e.cause.message + end +} + +# ... and so does Module#ruby2_keywords, which rewrites a method definition +assert_equal 'can not modify M because it is created by another Ractor', %q{ + module M + def m(*a) = a + end + + r = Ractor.new do + M.send(:ruby2_keywords, :m) + end + + begin + r.join + rescue Ractor::RemoteError => e + e.cause.message + end +} + # Getting non-shareable objects via constants by other Ractors is not allowed -assert_equal 'can not access non-shareable objects in constant C::CONST by non-main Ractor.', <<~'RUBY', frozen_string_literal: false +assert_equal 'can not access non-shareable objects in constant C::CONST of a class/module created by another Ractor.', <<~'RUBY', frozen_string_literal: false class C CONST = 'str' end @@ -1191,7 +1280,7 @@ assert_equal 'can not access non-shareable objects in constant C::CONST by non-m RUBY # Constant cache should care about non-shareable constants -assert_equal "can not access non-shareable objects in constant Object::STR by non-main Ractor.", <<~'RUBY', frozen_string_literal: false +assert_equal "can not access non-shareable objects in constant Object::STR of a class/module created by another Ractor.", <<~'RUBY', frozen_string_literal: false STR = "hello" def str; STR; end s = str() # fill const cache @@ -1203,7 +1292,7 @@ assert_equal "can not access non-shareable objects in constant Object::STR by no RUBY # The correct constant path shall be reported -assert_equal "can not access non-shareable objects in constant Object::STR by non-main Ractor.", <<~'RUBY', frozen_string_literal: false +assert_equal "can not access non-shareable objects in constant Object::STR of a class/module created by another Ractor.", <<~'RUBY', frozen_string_literal: false STR = "hello" module M def self.str; STR; end @@ -1216,8 +1305,8 @@ assert_equal "can not access non-shareable objects in constant Object::STR by no end RUBY -# Setting non-shareable objects into constants by other Ractors is not allowed -assert_equal 'can not set constants with non-shareable objects by non-main Ractors', <<~'RUBY', frozen_string_literal: false +# Setting constants of classes created by other Ractors is not allowed +assert_equal 'can not set constants of classes/modules created by another Ractor', <<~'RUBY', frozen_string_literal: false class C end r = Ractor.new do @@ -1733,19 +1822,12 @@ assert_equal 'true', %q{ } # check method cache invalidation +# (the owner Ractor redefines methods while another Ractor calls them) assert_equal 'true', %q{ class Foo def hello = nil end - r1 = Ractor.new do - 1000.times do - class Foo - def hello = nil - end - end - end - r2 = Ractor.new do 1000.times do o = Foo.new @@ -1753,7 +1835,12 @@ assert_equal 'true', %q{ end end - r1.value + 1000.times do + class Foo + def hello = nil + end + end + r2.value true @@ -2603,21 +2690,23 @@ RUBY assert_equal 'ok', <<~'RUBY' begin - CLASSES = 1000.times.map { Class.new }.freeze - + # Each Ractor creates its own class (it can only define bmethods on classes + # it owns) and returns it after defining the bmethod. # This would be better to run in parallel, but there's a bug with lambda # creation and YJIT causing crashes in dev mode - ractors = CLASSES.map do |klass| - Ractor.new(klass) do |klass| + ractors = 1000.times.map do + Ractor.new do + klass = Class.new Ractor.receive klass.define_method(:foo) {} + klass end end - ractors.each do |ractor| + CLASSES = ractors.map do |ractor| ractor << nil - ractor.join - end + ractor.value + end.freeze ractors.clear GC.start diff --git a/eval.c b/eval.c index ddefe300f5d..15a8cad2f4e 100644 --- a/eval.c +++ b/eval.c @@ -456,6 +456,7 @@ rb_class_modify_check(VALUE klass) } rb_error_frozen_object(klass); } + rb_class_owner_check(klass); } NORETURN(static void rb_longjmp(rb_execution_context_t *, enum ruby_tag_type, volatile VALUE, VALUE)); @@ -1636,6 +1637,11 @@ rb_mod_refine(VALUE module, VALUE klass) ensure_class_or_module(klass); + // refine installs refined method entries into the target's method table, and + // rb_refinement_setup writes the refinement tables into the receiver + rb_class_owner_check(module); + rb_class_owner_check(klass); + rb_refinement_setup(&data, module, klass); rb_yield_refine_block(data.refinement, data.refinements); diff --git a/object.c b/object.c index 6327f4aadc4..d76057640cf 100644 --- a/object.c +++ b/object.c @@ -2205,6 +2205,8 @@ rb_class_initialize(int argc, VALUE *argv, VALUE klass) { VALUE super; + // an uninitialized class is still somebody's, and this writes its superclass + rb_class_modify_check(klass); if (RCLASS_SUPER(klass) != 0 || klass == rb_cBasicObject) { rb_raise(rb_eTypeError, "already initialized class"); } diff --git a/test/ruby/test_iseq.rb b/test/ruby/test_iseq.rb index c01bb3dc90b..ffa0cbd5f39 100644 --- a/test/ruby/test_iseq.rb +++ b/test/ruby/test_iseq.rb @@ -878,17 +878,27 @@ class TestISeq < Test::Unit::TestCase def test_mandatory_only_redef assert_separately ['-W0'], <<~RUBY - r = Ractor.new{ - Float(10) - module Kernel - undef Float - def Float(n) - :new - end - end + port = Ractor::Port.new + r = Ractor.new(port){|port| + Float(10) # fill the mandatory only cache + port << :filled + Ractor.receive GC.start Float(30) } + + port.receive + + # Kernel is created by the main Ractor, so only the main Ractor can + # redefine it. The redefinition should be visible from the other Ractor. + module Kernel + undef Float + def Float(n) + :new + end + end + + r << :redefined assert_equal :new, r.value RUBY end diff --git a/variable.c b/variable.c index 2ff49546c8b..ee6e92a7062 100644 --- a/variable.c +++ b/variable.c @@ -1209,12 +1209,11 @@ rb_alias_variable(ID name1, ID name2) } static void -IVAR_ACCESSOR_SHOULD_BE_MAIN_RACTOR(ID id) +class_ivar_set_ractor_check(VALUE klass, ID id) { - if (UNLIKELY(!rb_ractor_main_p())) { - if (rb_is_instance_id(id)) { // check only normal ivars - rb_raise(rb_eRactorIsolationError, "can not set instance variables of classes/modules by non-main Ractors"); - } + if (rb_is_instance_id(id) && // check only normal ivars + UNLIKELY(!rb_class_owned_p(klass))) { + rb_raise(rb_eRactorIsolationError, "can not set instance variables of classes/modules created by another Ractor"); } } @@ -1244,6 +1243,10 @@ ivar_ractor_check(VALUE obj, ID id) 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"); } } @@ -1556,11 +1559,11 @@ rb_ivar_lookup(VALUE obj, ID id, VALUE undef) } if (is_class && val != undef && rb_is_instance_id(id)) { - if (UNLIKELY(!rb_ractor_main_p()) && !rb_ractor_shareable_p(val)) { + if (UNLIKELY(!rb_class_owned_p(obj)) && !rb_ractor_shareable_p(val)) { rb_raise( rb_eRactorIsolationError, - "can not get unshareable values from instance variables of classes/modules from " - "non-main Ractors (%"PRIsVALUE" from %"PRIsVALUE")", + "can not get unshareable values from instance variables of classes/modules " + "created by another Ractor (%"PRIsVALUE" from %"PRIsVALUE")", rb_id2str(id), obj ); @@ -1593,9 +1596,9 @@ rb_ivar_get_at(VALUE obj, attr_index_t index, ID id) VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj); VALUE val = rb_imemo_fields_ptr(fields_obj)[index]; - if (UNLIKELY(!rb_ractor_main_p()) && !rb_ractor_shareable_p(val)) { + if (UNLIKELY(!rb_class_owned_p(obj)) && !rb_ractor_shareable_p(val)) { rb_raise(rb_eRactorIsolationError, - "can not get unshareable values from instance variables of classes/modules from non-main Ractors"); + "can not get unshareable values from instance variables of classes/modules created by another Ractor"); } return val; @@ -1648,7 +1651,7 @@ rb_ivar_delete(VALUE obj, ID id, VALUE undef) int type = BUILTIN_TYPE(obj); if (type == T_CLASS || type == T_MODULE) { - IVAR_ACCESSOR_SHOULD_BE_MAIN_RACTOR(id); + class_ivar_set_ractor_check(obj, id); if (rb_multi_ractor_p()) { concurrent = true; @@ -2042,7 +2045,7 @@ ivar_set(VALUE obj, ID id, VALUE val) case T_CLASS: case T_MODULE: { - IVAR_ACCESSOR_SHOULD_BE_MAIN_RACTOR(id); + class_ivar_set_ractor_check(obj, id); bool dontcare; return class_ivar_set(obj, id, val, &dontcare); } @@ -2328,7 +2331,8 @@ rb_field_foreach(VALUE obj, rb_ivar_foreach_callback_func *func, st_data_t arg, case T_CLASS: case T_MODULE: { - IVAR_ACCESSOR_SHOULD_BE_MAIN_RACTOR(0); + // No owner check: every caller of this walk uses the names only for a + // class/module. Values are checked where they are read (rb_ivar_lookup). VALUE fields_obj = RCLASS_WRITABLE_FIELDS_OBJ(obj); if (fields_obj) { imemo_fields_each(fields_obj, func, arg, ivar_only); @@ -2891,6 +2895,8 @@ rb_autoload_str(VALUE module, ID name, VALUE feature) rb_raise(rb_eNameError, "autoload must be constant name: %"PRIsVALUE"", QUOTE_ID(name)); } + rb_class_owner_check(module); + Check_Type(feature, T_STRING); if (!RSTRING_LEN(feature)) { rb_raise(rb_eArgError, "empty feature name"); @@ -3332,9 +3338,9 @@ rb_const_get_0(VALUE klass, ID id, int exclude, int recurse, int visibility) VALUE found_in; VALUE c = rb_const_search(klass, id, exclude, recurse, visibility, &found_in); if (!UNDEF_P(c)) { - if (UNLIKELY(!rb_ractor_main_p())) { + if (UNLIKELY(!rb_class_owned_p(found_in))) { if (!rb_ractor_shareable_p(c)) { - rb_raise(rb_eRactorIsolationError, "can not access non-shareable objects in constant %"PRIsVALUE"::%"PRIsVALUE" by non-main Ractor.", rb_class_path(found_in), rb_id2str(id)); + rb_raise(rb_eRactorIsolationError, "can not access non-shareable objects in constant %"PRIsVALUE"::%"PRIsVALUE" of a class/module created by another Ractor.", rb_class_path(found_in), rb_id2str(id)); } } return c; @@ -3546,6 +3552,7 @@ rb_const_remove(VALUE mod, ID id) rb_const_entry_t *ce; rb_check_frozen(mod); + rb_class_owner_check(mod); ce = rb_const_lookup(mod, id); @@ -3842,8 +3849,8 @@ const_set(VALUE klass, ID id, VALUE val) QUOTE_ID(id)); } - if (!rb_ractor_main_p() && !rb_ractor_shareable_p(val)) { - rb_raise(rb_eRactorIsolationError, "can not set constants with non-shareable objects by non-main Ractors"); + if (UNLIKELY(!rb_class_owned_p(klass))) { + rb_raise(rb_eRactorIsolationError, "can not set constants of classes/modules created by another Ractor"); } check_before_mod_set(klass, id, val, "constant"); diff --git a/vm_core.h b/vm_core.h index fa407563b53..97b1bfb694a 100644 --- a/vm_core.h +++ b/vm_core.h @@ -263,10 +263,13 @@ struct iseq_inline_constant_cache_entry { VALUE value; const rb_cref_t *ic_cref; + /* Ractor that filled this entry. An unshareable value may be handed out again + * only to that Ractor: it is the one that passed the owner check. */ + rb_serial_t ractor_id; }; STATIC_ASSERT(sizeof_iseq_inline_constant_cache_entry, - (offsetof(struct iseq_inline_constant_cache_entry, ic_cref) + - sizeof(const rb_cref_t *)) <= RVALUE_SIZE); + (offsetof(struct iseq_inline_constant_cache_entry, ractor_id) + + sizeof(rb_serial_t)) <= RVALUE_SIZE); struct iseq_inline_constant_cache { struct iseq_inline_constant_cache_entry *entry; diff --git a/vm_insnhelper.c b/vm_insnhelper.c index fa9df3eb9df..3d0b4734406 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -1142,10 +1142,10 @@ vm_get_ev_const(rb_execution_context_t *ec, VALUE orig_klass, ID id, bool allow_ return 1; } else { - if (UNLIKELY(!rb_ractor_main_p())) { + if (UNLIKELY(!rb_class_owned_p(klass))) { if (!rb_ractor_shareable_p(val)) { rb_raise(rb_eRactorIsolationError, - "can not access non-shareable objects in constant %"PRIsVALUE"::%"PRIsVALUE" by non-main ractor.", rb_class_path(klass), rb_id2str(id)); + "can not access non-shareable objects in constant %"PRIsVALUE"::%"PRIsVALUE" of a class/module created by another Ractor.", rb_class_path(klass), rb_id2str(id)); } } return val; @@ -1255,10 +1255,10 @@ vm_getivar(VALUE obj, ID id, const rb_iseq_t *iseq, IVC ic, const struct rb_call case T_CLASS: case T_MODULE: { - if (UNLIKELY(!rb_ractor_main_p())) { - // For two reasons we can only use the fast path on the main - // ractor. - // First, only the main ractor is allowed to set ivars on classes + if (UNLIKELY(!rb_class_owned_p(obj))) { + // For two reasons we can only use the fast path on the Ractor + // that owns the class. + // First, only the owner Ractor is allowed to set ivars on classes // and modules. So we can skip locking. // Second, other ractors need to check the shareability of the // values returned from the class ivars. @@ -1428,7 +1428,7 @@ NOINLINE(static VALUE vm_setivar_class(VALUE obj, VALUE val, rb_setivar_cache ca static VALUE vm_setivar_class(VALUE obj, VALUE val, rb_setivar_cache cache) { - if (UNLIKELY(!rb_ractor_main_p())) { + if (UNLIKELY(!rb_class_owned_p(obj))) { return Qundef; } @@ -6708,9 +6708,11 @@ vm_ic_track_const_chain(rb_control_frame_t *cfp, IC ic, const ID *segments) // For JIT inlining static inline bool -vm_inlined_ic_hit_p(VALUE flags, VALUE value, const rb_cref_t *ic_cref, const VALUE *reg_ep) +vm_inlined_ic_hit_p(VALUE flags, VALUE value, const rb_cref_t *ic_cref, rb_serial_t ractor_id, const VALUE *reg_ep) { - if ((flags & IMEMO_CONST_CACHE_SHAREABLE) || rb_ractor_main_p()) { + // Not rb_ractor_main_p(): an owner Ractor may now cache an unshareable constant + // of its own class, and only it may be handed that value back. + if ((flags & IMEMO_CONST_CACHE_SHAREABLE) || ractor_id == rb_ractor_id(GET_RACTOR())) { VM_ASSERT(ractor_incidental_shareable_p(flags & IMEMO_CONST_CACHE_SHAREABLE, value)); return (ic_cref == NULL || // no need to check CREF @@ -6723,7 +6725,7 @@ static bool vm_ic_hit_p(const struct iseq_inline_constant_cache_entry *ice, const VALUE *reg_ep) { VM_ASSERT(IMEMO_TYPE_P(ice, imemo_constcache)); - return vm_inlined_ic_hit_p(ice->flags, ice->value, ice->ic_cref, reg_ep); + return vm_inlined_ic_hit_p(ice->flags, ice->value, ice->ic_cref, ice->ractor_id, reg_ep); } // YJIT needs this function to never allocate and never raise @@ -6745,6 +6747,7 @@ vm_ic_update(const rb_iseq_t *iseq, IC ic, VALUE val, const VALUE *reg_ep, const struct iseq_inline_constant_cache_entry *ice = SHAREABLE_IMEMO_NEW(struct iseq_inline_constant_cache_entry, imemo_constcache, 0); RB_OBJ_WRITE(ice, &ice->value, val); ice->ic_cref = vm_get_const_key_cref(reg_ep); + ice->ractor_id = rb_ractor_id(GET_RACTOR()); if (rb_ractor_shareable_p(val)) { RUBY_ASSERT((rb_gc_verify_shareable(val), 1)); diff --git a/vm_method.c b/vm_method.c index e0dc349e19c..3314f0449c7 100644 --- a/vm_method.c +++ b/vm_method.c @@ -3196,6 +3196,7 @@ rb_mod_ruby2_keywords(int argc, VALUE *argv, VALUE module) rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS); rb_check_frozen(module); + rb_class_owner_check(module); for (i = 0; i < argc; i++) { VALUE v = argv[i]; diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs index 2b688521c80..88d9c043364 100644 --- a/yjit/src/cruby_bindings.inc.rs +++ b/yjit/src/cruby_bindings.inc.rs @@ -453,6 +453,7 @@ pub struct iseq_inline_constant_cache_entry { pub flags: VALUE, pub value: VALUE, pub ic_cref: *const rb_cref_t, + pub ractor_id: rb_serial_t, } #[repr(C)] #[derive(Debug, Copy, Clone)] diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index a6a826514a6..e0161a42373 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -593,6 +593,7 @@ pub struct iseq_inline_constant_cache_entry { pub flags: VALUE, pub value: VALUE, pub ic_cref: *const rb_cref_t, + pub ractor_id: rb_serial_t, } #[repr(C)] #[derive(Debug, Copy, Clone)]