From 57213d44ce7b1a31fc9648e9cd5eb0c4507f4a49 Mon Sep 17 00:00:00 2001 From: Randy Stauner Date: Thu, 24 Sep 2026 16:26:10 -0700 Subject: [PATCH 1/5] [Bug #22383] Fix inverted `STR_SHARED` check in `rb_str_tmp_frozen_release` 45a2c95d0f7184c9cd64ddd26699af31bea8675d mistakenly rewrote FL_TEST_RAW(orig, STR_SHARED) && !FL_TEST_RAW(orig, STR_TMPLOCK|RUBY_FL_FREEZE) as FL_TEST_RAW(orig, STR_SHARED | STR_TMPLOCK) == STR_TMPLOCK requiring orig to not be shared, the opposite of the original condition. orig always shares the buffer with tmp here, so the buffer was never given back and the string stayed shared until its next modification copied it. The code then read aux.shared from strings that are not shared, where the union holds aux.capa. This should be backported to 4.0 --- ext/-test-/string/capacity.c | 9 +++++++++ string.c | 2 +- test/-ext-/string/test_capacity.rb | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/ext/-test-/string/capacity.c b/ext/-test-/string/capacity.c index 26eb6f4ff14b47..51f1713d21a3da 100644 --- a/ext/-test-/string/capacity.c +++ b/ext/-test-/string/capacity.c @@ -17,9 +17,18 @@ bug_str_new_shared(VALUE klass, VALUE str) return rb_str_new_shared(str); } +static VALUE +bug_str_tmp_frozen_acquire_release(VALUE klass, VALUE str) +{ + VALUE tmp = rb_str_tmp_frozen_acquire(str); + rb_str_tmp_frozen_release(str, tmp); + return str; +} + void Init_string_capacity(VALUE klass) { rb_define_singleton_method(klass, "capacity", bug_str_capacity, 1); rb_define_singleton_method(klass, "rb_str_new_shared", bug_str_new_shared, 1); + rb_define_singleton_method(klass, "tmp_frozen_acquire_release", bug_str_tmp_frozen_acquire_release, 1); } diff --git a/string.c b/string.c index 4c99f8d4a548ac..391764a14cd063 100644 --- a/string.c +++ b/string.c @@ -1629,7 +1629,7 @@ rb_str_tmp_frozen_release(VALUE orig, VALUE tmp) if (STR_EMBED_P(tmp)) { RUBY_ASSERT(OBJ_FROZEN_RAW(tmp)); } - else if (FL_TEST_RAW(orig, STR_SHARED | STR_TMPLOCK) == STR_TMPLOCK && + else if (FL_TEST_RAW(orig, STR_SHARED | STR_TMPLOCK) == STR_SHARED && !OBJ_FROZEN_RAW(orig)) { VALUE shared = RSTRING(orig)->as.heap.aux.shared; diff --git a/test/-ext-/string/test_capacity.rb b/test/-ext-/string/test_capacity.rb index ca7ba23ce2680e..dc02490aefa228 100644 --- a/test/-ext-/string/test_capacity.rb +++ b/test/-ext-/string/test_capacity.rb @@ -60,12 +60,31 @@ def test_capacity_fstring assert_equal(s.length, capa(s)) end + # Temporarily handing a string's buffer over to a frozen shared root + # (rb_str_tmp_frozen_acquire) uses an internal ASCII-8BIT string. Use + # UTF-16LE so that the terminator length of the root differs from the + # terminator length of the string to verify that releasing the root restores + # the capacity in the string's own encoding. + def test_frozen_root_capacity_with_multibyte_terminator + s = multibyte_terminator_string + capacity = capa(s) + assert_operator(capacity, :>=, s.bytesize) + + Bug::String.tmp_frozen_acquire_release(s) + + assert_equal(capacity, capa(s)) + end + private def capa(str) Bug::String.capacity(str) end + def multibyte_terminator_string + ("\u{30AF}\u{30FC}\u{30DD}\u{30F3}\u{1F381}" * 100).encode("UTF-16LE") + end + def embed_header_size GC::INTERNAL_CONSTANTS[:RBASIC_SIZE] + RbConfig::SIZEOF['void*'] end From 3ebb1b2cab13d1c24d71e80bc19f77ed82515a96 Mon Sep 17 00:00:00 2001 From: Matt Valentine-House Date: Tue, 22 Sep 2026 10:53:05 +0100 Subject: [PATCH 2/5] dir.c: keep GC-registered globals shareable or unregistered last_cwd is set on init in the main Ractor, but Dir.pwd attempts to store into it. When this is done on a child Ractor we violate the invariant that the owning and registering Ractor must be the same. On a normal build the use-after-free is invisible, because we only zero out the flags, so the cache check compares the slot bytes as usual and just thinks it's a cache miss. It gets flagged on ASAN builds --- bootstraptest/test_ractor.rb | 14 ++++++++++++++ dir.c | 24 ++++++++++++++++-------- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/bootstraptest/test_ractor.rb b/bootstraptest/test_ractor.rb index 578360a507cf01..a72d6b34970281 100644 --- a/bootstraptest/test_ractor.rb +++ b/bootstraptest/test_ractor.rb @@ -3303,3 +3303,17 @@ def st.m; :strct end [untimed_result, th2.value] } + +# last_cwd is set on init in the main Ractor, but Dir.pwd attempts to store into +# it. When this is done on a child Ractor we violate the invariant that the +# owning and registering Ractor must be the same. On a normal build the +# use-after-free is invisible, because we only zero out the flags, so the cache +# check compares the slot bytes as usual and just thinks it's a cache miss. +# +# This test exists because it will hit a use-after-poison on ASAN builds +assert_equal 'ok', %q{ + Dir.chdir("..") + Ractor.new { Dir.pwd; 500_000.times { "y" * 300 } }.join + Dir.pwd + 'ok' +} diff --git a/dir.c b/dir.c index 474769d67b8f1d..d5e24223c6e448 100644 --- a/dir.c +++ b/dir.c @@ -115,7 +115,10 @@ char *strchr(char*,char); #include "internal/object.h" #include "internal/imemo.h" #include "internal/vm.h" +#include "vm_core.h" #include "ruby/encoding.h" +#include "ruby/ractor.h" + #include "ruby/ruby.h" #include "ruby/thread.h" #include "ruby/util.h" @@ -1270,12 +1273,12 @@ dir_chdir0(VALUE path) } static struct { - VALUE thread; + rb_thread_t *thread; /* only ever compared, never dereferenced */ VALUE path; int line; int blocking; } chdir_lock = { - .blocking = 0, .thread = Qnil, + .blocking = 0, .thread = NULL, .path = Qnil, .line = 0, }; @@ -1283,11 +1286,16 @@ static void chdir_enter(void) { if (chdir_lock.blocking == 0) { - chdir_lock.path = rb_source_location(&chdir_lock.line); + VALUE path = rb_source_location(&chdir_lock.line); + /* chdir_lock.path is registered on the main Ractor, but the source + * location string belongs to the calling Ractor. To avoid the dangling + * reference on local GC, this needs to be shareable + */ + chdir_lock.path = NIL_P(path) ? Qnil : RB_OBJ_SET_FROZEN_SHAREABLE(rb_str_dup(path)); } chdir_lock.blocking++; - if (NIL_P(chdir_lock.thread)) { - chdir_lock.thread = rb_thread_current(); + if (chdir_lock.thread == NULL) { + chdir_lock.thread = rb_thread_ptr(rb_thread_current()); } } @@ -1296,7 +1304,7 @@ chdir_leave(void) { chdir_lock.blocking--; if (chdir_lock.blocking == 0) { - chdir_lock.thread = Qnil; + chdir_lock.thread = NULL; chdir_lock.path = Qnil; chdir_lock.line = 0; } @@ -1307,7 +1315,7 @@ chdir_alone_block_p(void) { int block_given = rb_block_given_p(); if (chdir_lock.blocking > 0) { - if (rb_thread_current() != chdir_lock.thread) + if (rb_thread_ptr(rb_thread_current()) != chdir_lock.thread) rb_raise(rb_eRuntimeError, "conflicting chdir during another chdir block"); if (!block_given) { if (!NIL_P(chdir_lock.path)) { @@ -1658,6 +1666,7 @@ rb_dir_getwd_ospath(void) cached_cwd = rb_str_new(path, (long)len); #endif rb_str_freeze(cached_cwd); + RB_OBJ_SET_SHAREABLE(cached_cwd); RUBY_ATOMIC_VALUE_SET(last_cwd, cached_cwd); } return cached_cwd; @@ -4146,7 +4155,6 @@ Init_Dir(void) #endif rb_gc_register_address(&chdir_lock.path); - rb_gc_register_address(&chdir_lock.thread); rb_gc_register_address(&last_cwd); rb_cDir = rb_define_class("Dir", rb_cObject); From 43b1e17d08dab39b347e30379c5fc24a3dedad84 Mon Sep 17 00:00:00 2001 From: Matt Valentine-House Date: Tue, 22 Sep 2026 10:53:21 +0100 Subject: [PATCH 3/5] json: reject non-shareable default sort_keys proc from non-main Ractors JSON::State.default_sort_keys_proc is registered at extension load time so it's owned by the main Ractor. When we implement the registration table on each Ractor storing another Ractor's unshareable Proc into that slot leaves a dangling reference when the owning Ractor runs a local GC. This commit ensures that we won't use a shareable proc by raising a Ractor::IsolationError. --- ext/json/lib/json/common.rb | 18 ++++++++++++++++++ test/json/json_generator_test.rb | 12 ++++++++++++ test/json/ractor_test.rb | 29 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/ext/json/lib/json/common.rb b/ext/json/lib/json/common.rb index 82d334061e29d5..1c5d4c22e53300 100644 --- a/ext/json/lib/json/common.rb +++ b/ext/json/lib/json/common.rb @@ -80,6 +80,24 @@ def parser=(parser) # :nodoc: def generator=(generator) # :nodoc: old, $VERBOSE = $VERBOSE, nil + unless generator::State.respond_to?(:default_sort_keys_proc_unchecked=, true) + generator::State.singleton_class.class_eval do + alias_method :default_sort_keys_proc_unchecked=, :default_sort_keys_proc= + private :default_sort_keys_proc_unchecked= + + def default_sort_keys_proc=(proc) + unless ::Proc === proc + raise ::TypeError, "sort_key_proc must be a Proc" + end + if defined?(::Ractor) && !::Ractor.shareable?(proc) && !::Ractor.current.equal?(::Ractor.main) + raise ::Ractor::IsolationError, + "can not set a non-shareable Proc as the default sort_keys proc from a non-main Ractor" + end + self.default_sort_keys_proc_unchecked = proc + end + end + end + # The default proc used when the +sort_keys+ generation option is +true+. # It returns a new hash with the entries sorted by their keys. sort_keys_proc = ->(hash) { hash.sort.to_h } diff --git a/test/json/json_generator_test.rb b/test/json/json_generator_test.rb index 1b2197735dec4c..e86fe6a2e5c03b 100755 --- a/test/json/json_generator_test.rb +++ b/test/json/json_generator_test.rb @@ -248,6 +248,18 @@ def test_generate_sort_keys_with_proc assert_instance_of Proc, state.sort_keys end + def test_default_sort_keys_proc_setter_survives_generator_reassignment + omit "fork not supported" unless Process.respond_to?(:fork) + pid = fork do + JSON.generator = JSON::Ext::Generator + JSON.generator = JSON::Ext::Generator + JSON::State.default_sort_keys_proc = ->(hash) { hash.sort.to_h } + exit!(JSON.generate({b: 1, a: 2}, sort_keys: true) == '{"a":2,"b":1}' ? 0 : 1) + end + _, status = Process.wait2(pid) + assert_predicate status, :success? + end + def test_generate_custom state = State.new(space_before: " ", space: " ", indent: "", object_nl: "\n", array_nl: "") json = generate({1=>{2=>3,4=>[5,6]}}, state) diff --git a/test/json/ractor_test.rb b/test/json/ractor_test.rb index 9e28f13c129c86..a501344fa7a093 100644 --- a/test/json/ractor_test.rb +++ b/test/json/ractor_test.rb @@ -123,4 +123,33 @@ def test_coder_proc _, status = Process.waitpid2(pid) assert_predicate status, :success? end if Ractor.respond_to?(:shareable_proc) + + def test_default_sort_keys_proc_ractor_safety + pid = fork do + Warning[:experimental] = false + results = Ractor.new do + outcomes = [] + + begin + JSON::State.default_sort_keys_proc = ->(hash) { hash.sort.to_h } + outcomes << :accepted + rescue Ractor::IsolationError + outcomes << :rejected + end + + # A shareable Proc is allowed from any Ractor. + JSON::State.default_sort_keys_proc = Ractor.shareable_lambda { |hash| hash.sort.reverse.to_h } + outcomes << JSON.generate({b: 1, a: 2}, sort_keys: true) + outcomes + end.value + + # The main Ractor owns the slot, so it may install any Proc. + JSON::State.default_sort_keys_proc = ->(hash) { hash.sort.to_h } + main_result = JSON.generate({b: 1, a: 2}, sort_keys: true) + + exit(results == [:rejected, '{"b":1,"a":2}'] && main_result == '{"a":2,"b":1}' ? 0 : 1) + end + _, status = Process.waitpid2(pid) + assert_predicate status, :success? + end if Ractor.respond_to?(:shareable_lambda) end if defined?(Ractor) && Process.respond_to?(:fork) From 2ad6b99a98058ce25de2d1d536f291b93d893834 Mon Sep 17 00:00:00 2001 From: Matt Valentine-House Date: Tue, 22 Sep 2026 10:54:31 +0100 Subject: [PATCH 4/5] GC: scope rb_gc_register_address to the calling Ractor The registered_globals list made every Ractor's GC scan every slot under a shared lock. This commit splits it into per-Ractor lists. We also maintain a global registry that references every Ractor that has ever registered so that unregistering from a Ractor other than the one that registered can look up the owning list. When a Ractor is joined, the dead Ractor's slots are moved to the joining Ractor's heap before the objspace merge happens. This change tightens up the requirements for values stored in a registered address. Previously, Every Ractor's local GC scanned the shared list. So if a slot held an unshareable object living in a different Ractor's heap, that object was always kept alive regardless. After this change, only the registering Ractor's GC scans its list. So if a slot holds another Ractor's unshareable object, the owning Ractor's local GC cannot see that reference, so it can collect the object while the registered slot still points at it. Storing such an object was already a Ractor containment violation. It was just hidden by the shared scan. [Feature #22277] --- NEWS.md | 19 +++ doc/extension.rdoc | 19 +++ ext/-test-/gc/register/register.c | 68 +++++++++++ gc.c | 185 ++++++++++++++++++++++++------ gc/default/default.c | 6 +- include/ruby/internal/gc.h | 25 +++- internal/gc.h | 4 + ractor.c | 53 ++++++++- ractor_core.h | 10 ++ ractor_sync.c | 1 + test/-ext-/gc/test_register.rb | 157 +++++++++++++++++++++++++ thread.c | 6 +- vm.c | 6 +- vm_core.h | 9 +- 14 files changed, 513 insertions(+), 55 deletions(-) diff --git a/NEWS.md b/NEWS.md index e64c4dbc066528..d97f66619b12ac 100644 --- a/NEWS.md +++ b/NEWS.md @@ -380,6 +380,25 @@ Ruby 4.0 bundled RubyGems and Bundler version 4. see the following links for det [[Feature #21861]] +### Ractor-scoped GC address registration + +* `rb_gc_register_address` and `rb_global_variable` now register the address + with the calling Ractor, and only that Ractor's GC marks the stored object. + + Any Ractor may register an address, but the value stored through it must be a + special constant, a shareable object, or an unshareable object owned by the + registering Ractor. Storing another Ractor's unshareable object can result in + the object being freed while the address still refers to it (use-after-free). + + If the address has process lifetime (a static VALUE), register it from the + main Ractor or keep the stored values shareable. + + When the registering Ractor is joined with `Ractor#value`, remaining + registrations move to the joining Ractor; otherwise they move to the main + Ractor once the dead Ractor is collected. + + [[Feature #22277]] + ### Removed APIs The following APIs, which have been deprecated for many years, are removed. diff --git a/doc/extension.rdoc b/doc/extension.rdoc index 10d1b482867242..b3cda9458e9bd6 100644 --- a/doc/extension.rdoc +++ b/doc/extension.rdoc @@ -1092,6 +1092,22 @@ or the objects themselves by void rb_gc_register_mark_object(VALUE object) +Registration is scoped to the calling Ractor. Any Ractor may register an +address. The stored value must be a special constant, a shareable object, +or an unshareable object owned by the registering Ractor. If the address +holds another Ractor's unshareable object, the owning Ractor's GC cannot +see the registration and can free the stored object while the registered +address still refers to it. This can cause a use-after-free if the +registering Ractor attempts to use the Object stored. + +If the address has process lifetime (a +static VALUE+), either register it +from the main Ractor or keep the stored values shareable. + +Calling +Ractor#value+ on a Ractor moves its registrations to the calling +Ractor; otherwise they move to the main Ractor once the dead Ractor is +collected. If you rely on a Ractor inheriting another Ractor's registered +globals, call +Ractor#value+. + === Prepare extconf.rb If the file named extconf.rb exists, it will be executed to generate @@ -1552,6 +1568,9 @@ golf_prelude.rb :: goruby specific libraries. void rb_global_variable(VALUE *var) :: Tells GC to protect C global variable, which holds Ruby value to be marked. + The registration belongs to the calling Ractor: the stored value must be + a special constant, a shareable object, or an unshareable object owned by + the registering Ractor. void rb_gc_register_mark_object(VALUE object) :: diff --git a/ext/-test-/gc/register/register.c b/ext/-test-/gc/register/register.c index 903b8f4074f44d..7f9b1e9a17d83e 100644 --- a/ext/-test-/gc/register/register.c +++ b/ext/-test-/gc/register/register.c @@ -1,4 +1,5 @@ #include "ruby.h" +#include "ruby/internal/has/feature.h" /* * Regression test for a heap-use-after-free in rb_gc_unregister_address(). @@ -52,11 +53,78 @@ gc_unregister_address_keeps_siblings(VALUE self) return result; } +static VALUE static_slot; + +static VALUE +gc_register_static(VALUE self, VALUE v) +{ + rb_gc_register_address(&static_slot); + static_slot = v; + return Qtrue; +} + +static VALUE +gc_unregister_static(VALUE self) +{ + rb_gc_unregister_address(&static_slot); + return Qnil; +} + +static VALUE +gc_static_slot_value(VALUE self) +{ + return static_slot; +} + +static VALUE +gc_static_slot_eq(VALUE self, VALUE v) +{ + if (!RB_TYPE_P(static_slot, T_STRING) || !RB_TYPE_P(v, T_STRING)) return Qfalse; + return rb_str_equal(static_slot, v); +} + +static VALUE +gc_assign_static(VALUE self, VALUE v) +{ + static_slot = v; + return v; +} + +static VALUE +gc_register_current_static(VALUE self) +{ + rb_gc_register_address(&static_slot); + return Qnil; +} + +/* Mirrors the VM-side RB_GC_REGISTERED_ADDR_CHECK definition without including a + * private GC header: debug and ASAN builds record the registration-time value. */ +static VALUE +gc_registered_address_check_enabled_p(VALUE self) +{ +#if RUBY_DEBUG || defined(__SANITIZE_ADDRESS__) || RBIMPL_HAS_FEATURE(address_sanitizer) + return Qtrue; +#else + return Qfalse; +#endif +} + void Init_register(void) { + rb_ext_ractor_safe(true); + VALUE mBug = rb_define_module("Bug"); VALUE mGC = rb_define_module_under(mBug, "GC"); rb_define_singleton_method(mGC, "unregister_address_keeps_siblings?", gc_unregister_address_keeps_siblings, 0); + rb_define_singleton_method(mGC, "register_static", gc_register_static, 1); + rb_define_singleton_method(mGC, "unregister_static", gc_unregister_static, 0); + rb_define_singleton_method(mGC, "static_slot_value", gc_static_slot_value, 0); + rb_define_singleton_method(mGC, "assign_static", gc_assign_static, 1); + rb_define_singleton_method(mGC, "static_slot_eq?", gc_static_slot_eq, 1); + rb_define_singleton_method(mGC, "register_current_static", + gc_register_current_static, 0); + rb_define_singleton_method(mGC, "registered_address_check_enabled?", + gc_registered_address_check_enabled_p, 0); } diff --git a/gc.c b/gc.c index d1bf86e2bb1b35..340d8dc38ec84c 100644 --- a/gc.c +++ b/gc.c @@ -636,14 +636,14 @@ rb_gc_guarded_ptr_val(volatile VALUE *ptr, VALUE val) static const char *obj_type_name(VALUE obj); -/* A forking parent can hold registered_globals.lock (every Ractor's root scan takes - * it); inheriting it locked would make the child's first GC wait forever, so rebuild - * it, like the generic_fields lock. */ +/* A forking parent can hold registered_addrs.lock; inheriting it locked would make + * the child's first register wait forever, so rebuild it, like the generic_fields + * lock. */ void rb_gc_atfork_global_locks(void) { rb_vm_t *vm = GET_VM(); - rb_native_mutex_initialize(&vm->gc.registered_globals.lock); + rb_native_mutex_initialize(&vm->gc.registered_addrs.lock); } #include "gc/default/default.c" @@ -3345,6 +3345,54 @@ rb_gc_get_ec(void) } } +/* need_lock is false when the caller already holds vm->gc.registered_addrs.lock (the + * terminated_set walk in rb_gc_mark_roots); the lock is not recursive, so both the + * marking path and the traversal-API snapshot path below must honour it. */ +void +rb_gc_mark_registered_addrs(rb_ractor_t *r, bool need_lock) +{ + if (r->registered_addrs_cnt == 0) return; + + if (rb_gc_impl_during_gc_p(rb_gc_get_objspace())) { + rb_vm_t *vm = GET_VM(); + if (need_lock) rb_native_mutex_lock(&vm->gc.registered_addrs.lock); + for (size_t i = 0; i < r->registered_addrs_cnt; i++) { + rb_gc_mark_maybe(*r->registered_addrs[i].addr); + } + if (need_lock) rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); + return; + } + + VALUE stack_snap[16]; + VALUE *snap = stack_snap; + size_t capa = numberof(stack_snap); + + rb_vm_t *vm = GET_VM(); + if (need_lock) rb_native_mutex_lock(&vm->gc.registered_addrs.lock); + size_t cnt = r->registered_addrs_cnt; + if (RB_UNLIKELY(cnt > capa)) { + if (need_lock) rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); + rb_ractor_t *cr = rb_current_ractor_raw(false); + bool saved_malloc_gc_disabled = cr ? cr->malloc_gc_disabled : false; + if (cr) cr->malloc_gc_disabled = true; + snap = ALLOC_N(VALUE, cnt); + if (cr) cr->malloc_gc_disabled = saved_malloc_gc_disabled; + capa = cnt; + if (need_lock) rb_native_mutex_lock(&vm->gc.registered_addrs.lock); + cnt = r->registered_addrs_cnt; + VM_ASSERT(cnt <= capa); + } + for (size_t i = 0; i < cnt; i++) { + snap[i] = *r->registered_addrs[i].addr; + } + if (need_lock) rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); + + for (size_t i = 0; i < cnt; i++) { + rb_gc_mark_maybe(snap[i]); + } + if (snap != stack_snap) xfree(snap); +} + void rb_gc_mark_roots(void *objspace, const char **categoryp) { @@ -3367,12 +3415,18 @@ rb_gc_mark_roots(void *objspace, const char **categoryp) rb_ractor_t *r; ccan_list_for_each(&vm->ractor.set, r, vmlr_node) { rb_ractor_mark_local_roots(r); + MARK_CHECKPOINT("registered_addrs"); + rb_gc_mark_registered_addrs(r, true); + MARK_CHECKPOINT("ractor"); } /* Early in boot (before rb_ractor_main_setup) main is not in vm->ractor.set * yet; do not drop its registered_marks in a single-objspace boot GC. */ if (vm->ractor.cnt == 0 && vm->ractor.main_ractor) { rb_ractor_mark_local_roots(vm->ractor.main_ractor); + MARK_CHECKPOINT("registered_addrs"); + rb_gc_mark_registered_addrs(vm->ractor.main_ractor, true); + MARK_CHECKPOINT("ractor"); } /* A Ractor that terminated (left vm->ractor.set) but whose struct is not freed * still owns rb_gc_register_mark_object pins. Keep them alive until @@ -3383,6 +3437,9 @@ rb_gc_mark_roots(void *objspace, const char **categoryp) if (owner) { rb_gc_mark_vm_stack_values((long)owner->registered_marks_cnt, owner->registered_marks); + MARK_CHECKPOINT("registered_addrs"); + rb_gc_mark_registered_addrs(owner, true); + MARK_CHECKPOINT("ractor"); } } @@ -3391,27 +3448,23 @@ rb_gc_mark_roots(void *objspace, const char **categoryp) * reachability. With multiple objspaces zombie_objspaces covers this. */ if (!rb_gc_impl_multi_objspace_p()) { rb_ractor_t *tr; - rb_native_mutex_lock(&vm->gc.registered_globals.lock); + rb_native_mutex_lock(&vm->gc.registered_addrs.lock); ccan_list_for_each(&vm->ractor.terminated_set, tr, vmlr_node) { rb_gc_mark_vm_stack_values((long)tr->registered_marks_cnt, tr->registered_marks); + MARK_CHECKPOINT("registered_addrs"); + rb_gc_mark_registered_addrs(tr, false); + MARK_CHECKPOINT("ractor"); } - rb_native_mutex_unlock(&vm->gc.registered_globals.lock); + rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); } } else { - rb_ractor_mark_local_roots(rb_ec_ractor_ptr(ec)); - } - - /* rb_gc_register_address slots live in one VM-wide list: *addr can later hold - * another objspace's value, so every Ractor's GC scans all slots conservatively, - * marking only its own residents. */ - MARK_CHECKPOINT("registered_globals"); - rb_native_mutex_lock(&vm->gc.registered_globals.lock); - for (size_t i = 0; i < vm->gc.registered_globals.addrs_cnt; i++) { - rb_gc_mark_maybe(*vm->gc.registered_globals.addrs[i]); + rb_ractor_t *cr = rb_ec_ractor_ptr(ec); + rb_ractor_mark_local_roots(cr); + MARK_CHECKPOINT("registered_addrs"); + rb_gc_mark_registered_addrs(cr, true); } - rb_native_mutex_unlock(&vm->gc.registered_globals.lock); /* Trap handlers live in the VM-global vm->trap_list.cmd[], a fixed array of aligned * VALUEs (signal.c uses ACCESS_ONCE): a racing walk reads either the old or the new @@ -3915,21 +3968,83 @@ rb_gc_register_mark_object(VALUE obj) } } +static rb_ractor_t * +gc_registered_addrs_owner(rb_vm_t *vm) +{ + rb_ractor_t *cr = rb_current_ractor_raw(false); + if (cr) return cr; + RUBY_ASSERT(vm->ractor.main_ractor != NULL); + return vm->ractor.main_ractor; +} + +/* Keep track of every ractor that owns registered addresses, so that a + * rb_gc_unregister_address() call from a non-owning ractor can find the list + * holding the address, and so GC can iterate every live registration list + * without walking the whole ractor set. */ +void +rb_gc_registered_addrs_enroll_without_gc(rb_vm_t *vm, rb_ractor_t *r) +{ + if (r->registered_addrs_listed) return; + if (vm->gc.registered_addrs.registry_cnt == vm->gc.registered_addrs.registry_capa) { + size_t nc = vm->gc.registered_addrs.registry_capa ? vm->gc.registered_addrs.registry_capa * 2 : 16; + struct rb_ractor_struct **p = realloc(vm->gc.registered_addrs.registry, + nc * sizeof(struct rb_ractor_struct *)); + if (!p) rb_bug("rb_gc_registered_addrs_enroll_without_gc: out of memory"); + vm->gc.registered_addrs.registry = p; + vm->gc.registered_addrs.registry_capa = nc; + } + vm->gc.registered_addrs.registry[vm->gc.registered_addrs.registry_cnt++] = r; + r->registered_addrs_listed = true; +} + +void +rb_gc_registered_addrs_unenroll_without_gc(rb_vm_t *vm, rb_ractor_t *r) +{ + if (!r->registered_addrs_listed) return; + for (size_t i = 0; i < vm->gc.registered_addrs.registry_cnt; i++) { + if (vm->gc.registered_addrs.registry[i] == r) { + vm->gc.registered_addrs.registry[i] = + vm->gc.registered_addrs.registry[--vm->gc.registered_addrs.registry_cnt]; + break; + } + } + r->registered_addrs_listed = false; +} + +static bool +gc_registered_addrs_remove(rb_ractor_t *r, VALUE *addr) +{ + for (size_t i = 0; i < r->registered_addrs_cnt; i++) { + if (r->registered_addrs[i].addr == addr) { + MEMMOVE(&r->registered_addrs[i], &r->registered_addrs[i + 1], + struct rb_ractor_registered_addr, r->registered_addrs_cnt - i - 1); + r->registered_addrs_cnt--; + return true; + } + } + return false; +} + void rb_gc_register_address(VALUE *addr) { rb_vm_t *vm = GET_VM(); - rb_native_mutex_lock(&vm->gc.registered_globals.lock); - if (vm->gc.registered_globals.addrs_cnt == vm->gc.registered_globals.addrs_capa) { - size_t nc = vm->gc.registered_globals.addrs_capa ? vm->gc.registered_globals.addrs_capa * 2 : 64; - VALUE **p = realloc(vm->gc.registered_globals.addrs, nc * sizeof(VALUE *)); + rb_native_mutex_lock(&vm->gc.registered_addrs.lock); + rb_ractor_t *owner = gc_registered_addrs_owner(vm); + if (owner->registered_addrs_cnt == owner->registered_addrs_capa) { + size_t nc = owner->registered_addrs_capa ? owner->registered_addrs_capa * 2 : 64; + struct rb_ractor_registered_addr *p = + realloc(owner->registered_addrs, nc * sizeof(struct rb_ractor_registered_addr)); if (!p) rb_bug("rb_gc_register_address: out of memory"); - vm->gc.registered_globals.addrs = p; - vm->gc.registered_globals.addrs_capa = nc; + owner->registered_addrs = p; + owner->registered_addrs_capa = nc; } - vm->gc.registered_globals.addrs[vm->gc.registered_globals.addrs_cnt++] = addr; - rb_native_mutex_unlock(&vm->gc.registered_globals.lock); + struct rb_ractor_registered_addr *entry = &owner->registered_addrs[owner->registered_addrs_cnt]; + entry->addr = addr; + owner->registered_addrs_cnt++; + rb_gc_registered_addrs_enroll_without_gc(vm, owner); + rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); /* Some C extensions register before assigning, so protect obj from GC here. */ RB_GC_GUARD(*addr); @@ -3940,19 +4055,18 @@ rb_gc_unregister_address(VALUE *addr) { rb_vm_t *vm = GET_VM(); - /* One VM-wide list, so a register and unregister from different Ractors (Init on - * main, dfree elsewhere) still pair up. Silently a no-op when not found: upstream - * tolerates a double unregister too. */ - rb_native_mutex_lock(&vm->gc.registered_globals.lock); - for (size_t i = 0; i < vm->gc.registered_globals.addrs_cnt; i++) { - if (vm->gc.registered_globals.addrs[i] == addr) { - MEMMOVE(&vm->gc.registered_globals.addrs[i], &vm->gc.registered_globals.addrs[i + 1], - VALUE *, vm->gc.registered_globals.addrs_cnt - i - 1); - vm->gc.registered_globals.addrs_cnt--; + rb_native_mutex_lock(&vm->gc.registered_addrs.lock); + rb_ractor_t *cr = rb_current_ractor_raw(false); + if (cr == NULL) cr = vm->ractor.main_ractor; + if (cr && gc_registered_addrs_remove(cr, addr)) goto done; + for (size_t i = 0; i < vm->gc.registered_addrs.registry_cnt; i++) { + if (vm->gc.registered_addrs.registry[i] != cr && + gc_registered_addrs_remove(vm->gc.registered_addrs.registry[i], addr)) { break; } } - rb_native_mutex_unlock(&vm->gc.registered_globals.lock); + done: + rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); } void @@ -4423,6 +4537,7 @@ rb_gc_objspace_absorb_all_zombies(void) rb_ractor_t *owner = vm->gc.zombie_objspaces[0].owner; if (owner) { rb_ractor_absorb_registered_marks(GET_RACTOR(), owner); + rb_ractor_absorb_registered_addrs_without_gc(GET_RACTOR(), owner); } rb_gc_objspace_absorb_into_current(vm->gc.zombie_objspaces[0].owner_slot); if (vm->gc.zombie_objspaces_count >= before) { diff --git a/gc/default/default.c b/gc/default/default.c index 0bc27677a7959a..9aa1c046f9d1fa 100644 --- a/gc/default/default.c +++ b/gc/default/default.c @@ -7069,11 +7069,7 @@ root_scope_check_i(const char *category, VALUE obj, void *ptr) if (strcmp(category, "machine_context") == 0 || strcmp(category, "vm_registered_objects") == 0 || strcmp(category, "end_proc") == 0 || - strcmp(category, "trap_list") == 0 || - /* Every Ractor's root scan walks the one VM-wide registered-globals list (a slot - * can hold another objspace's value); rb_gc_mark_maybe filters to its own - * objspace, so a foreign entry here is by design, not a leak. */ - strcmp(category, "registered_globals") == 0) { + strcmp(category, "trap_list") == 0) { return; } diff --git a/include/ruby/internal/gc.h b/include/ruby/internal/gc.h index 8986efd0e90e8d..fa04a27bb71c6f 100644 --- a/include/ruby/internal/gc.h +++ b/include/ruby/internal/gc.h @@ -406,17 +406,40 @@ void rb_gc_adjust_memory_usage(ssize_t diff); * Because this registration itself has a possibility to trigger a GC, this * function must be called before any GC-able objects is assigned to the * address pointed by `valptr`. + * + * Registration is scoped to the calling Ractor. The calling Ractor owns the + * registered address, and only that Ractor's GC marks the object stored in it. + * Any Ractor may call this function to register an address but the value at + * that address must be a special constant, a shareable object, or an + * unshareable object owned by the registering Ractor. + * + * The owning Ractor's GC cannot see the registration, so if the object at the + * registered address is unshareable, and owned by another ractor, the owning + * Ractor could free it while the address still refers to it, which results in a + * use-after-free crash, when the address is next accessed. If the address has + * process lifetime (a static VALUE), register it from the main Ractor or keep + * the stored values shareable. + * + * Calling `Ractor#value` on the registering Ractor moves its remaining + * registrations to the calling Ractor; otherwise they move to the main + * Ractor once the dead Ractor is collected. */ void rb_gc_register_address(VALUE *valptr); /** - * An alias for `rb_gc_register_address()`. + * An alias for `rb_gc_register_address()`. The same Ractor-ownership + * rule applies to the value stored in the variable: a special constant, + * a shareable object, or an unshareable object owned by the registering + * Ractor. */ void rb_global_variable(VALUE *); /** * Inform the garbage collector that a pointer previously passed to * `rb_gc_register_address()` no longer points to a live Ruby object. + * + * Any Ractor may call this function; the address is removed from the + * registering Ractor's list, which need not be the calling Ractor. */ void rb_gc_unregister_address(VALUE *valptr); diff --git a/internal/gc.h b/internal/gc.h index 0453162f4b81b2..8b63301046aeb6 100644 --- a/internal/gc.h +++ b/internal/gc.h @@ -271,6 +271,10 @@ size_t rb_obj_memsize_of(VALUE); struct rb_gc_object_metadata_entry *rb_gc_object_metadata(VALUE obj); void rb_gc_mark_values(long n, const VALUE *values); void rb_gc_mark_vm_stack_values(long n, const VALUE *values); +struct rb_vm_struct; +void rb_gc_mark_registered_addrs(struct rb_ractor_struct *r, bool need_lock); +void rb_gc_registered_addrs_enroll_without_gc(struct rb_vm_struct *vm, struct rb_ractor_struct *r); +void rb_gc_registered_addrs_unenroll_without_gc(struct rb_vm_struct *vm, struct rb_ractor_struct *r); void rb_gc_update_values(long n, VALUE *values); void rb_gc_mark_set_no_pin(st_table *); /* Exercised by the bundled -test-/gc/writebarrier extension, so it must be visible diff --git a/ractor.c b/ractor.c index fc7d8ac1004416..c68b5f9d0e9664 100644 --- a/ractor.c +++ b/ractor.c @@ -356,6 +356,7 @@ rb_ractor_mark_local_roots(rb_ractor_t *r) VM_ASSERT(RUBY_ATOMIC_PTR_LOAD(r->threads.dying_th) == NULL); rb_ractor_mark_terminated_join_value(r); rb_gc_mark_vm_stack_values((long)r->registered_marks_cnt, r->registered_marks); + rb_gc_mark_registered_addrs(r, true); return; } @@ -372,7 +373,6 @@ rb_ractor_mark_local_roots(rb_ractor_t *r) * marks only its own residents and leaves foreign or shareable entries to their * owner or to the global GC. */ rb_gc_mark_vm_stack_values((long)r->registered_marks_cnt, r->registered_marks); - } /* Mark and pin a terminated, unfreed Ractor's return value (legacy); the global GC @@ -409,6 +409,33 @@ rb_ractor_absorb_registered_marks(rb_ractor_t *dst, rb_ractor_t *src) src->registered_marks_cnt = 0; } +void +rb_ractor_absorb_registered_addrs_without_gc(rb_ractor_t *dst, rb_ractor_t *src) +{ + rb_vm_t *vm = GET_VM(); + + rb_native_mutex_lock(&vm->gc.registered_addrs.lock); + if (src->registered_addrs_cnt > 0) { + size_t need = dst->registered_addrs_cnt + src->registered_addrs_cnt; + if (need > dst->registered_addrs_capa) { + size_t nc = dst->registered_addrs_capa ? dst->registered_addrs_capa : 64; + while (nc < need) nc *= 2; + struct rb_ractor_registered_addr *p = + realloc(dst->registered_addrs, nc * sizeof(struct rb_ractor_registered_addr)); + if (!p) rb_bug("rb_ractor_absorb_registered_addrs_without_gc: out of memory"); + dst->registered_addrs = p; + dst->registered_addrs_capa = nc; + } + MEMCPY(dst->registered_addrs + dst->registered_addrs_cnt, + src->registered_addrs, struct rb_ractor_registered_addr, src->registered_addrs_cnt); + dst->registered_addrs_cnt = need; + src->registered_addrs_cnt = 0; + rb_gc_registered_addrs_enroll_without_gc(vm, dst); + } + rb_gc_registered_addrs_unenroll_without_gc(vm, src); + rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); +} + static int free_targeted_hook_lists(st_data_t key, st_data_t val, st_data_t _arg) { @@ -457,10 +484,10 @@ ractor_free(void *ptr) ractor_sync_free(r); if (r->in_terminated_set) { - rb_native_mutex_lock(&GET_VM()->gc.registered_globals.lock); + rb_native_mutex_lock(&GET_VM()->gc.registered_addrs.lock); ccan_list_del(&r->vmlr_node); r->in_terminated_set = false; - rb_native_mutex_unlock(&GET_VM()->gc.registered_globals.lock); + rb_native_mutex_unlock(&GET_VM()->gc.registered_addrs.lock); } /* An orphan (unjoined) Ractor hands its rb_gc_register_mark_object pins to main @@ -468,11 +495,21 @@ ractor_free(void *ptr) * Both happen before the objspace merge, so no window has unmoved registrations. */ if (!r->main_ractor) { rb_ractor_absorb_registered_marks(GET_VM()->ractor.main_ractor, r); + rb_ractor_absorb_registered_addrs_without_gc(GET_VM()->ractor.main_ractor, r); + } + else { + rb_native_mutex_lock(&GET_VM()->gc.registered_addrs.lock); + rb_gc_registered_addrs_unenroll_without_gc(GET_VM(), r); + rb_native_mutex_unlock(&GET_VM()->gc.registered_addrs.lock); } free(r->registered_marks); r->registered_marks = NULL; r->registered_marks_cnt = r->registered_marks_capa = 0; + free(r->registered_addrs); + r->registered_addrs = NULL; + r->registered_addrs_cnt = r->registered_addrs_capa = 0; + if (!r->main_ractor) { SIZED_FREE(r); } @@ -641,10 +678,10 @@ vm_remove_ractor(rb_vm_t *vm, rb_ractor_t *cr) * registered_marks of a Ractor that left the set; track it in a separate list * until ractor_free. */ if (!rb_gc_multi_objspace_p()) { - rb_native_mutex_lock(&vm->gc.registered_globals.lock); + rb_native_mutex_lock(&vm->gc.registered_addrs.lock); ccan_list_add(&vm->ractor.terminated_set, &cr->vmlr_node); cr->in_terminated_set = true; - rb_native_mutex_unlock(&vm->gc.registered_globals.lock); + rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); } if (vm->ractor.cnt <= 2 && vm->ractor.sync.terminate_waiting) { @@ -781,6 +818,12 @@ rb_ractor_terminate_atfork(rb_vm_t *vm, rb_ractor_t *r) r->status_ = ractor_terminated; // a termination epilogue in the parent did not survive the fork r->threads.dying_th = NULL; + if (!rb_gc_multi_objspace_p()) { + ccan_list_del(&r->vmlr_node); + ccan_list_add(&vm->ractor.terminated_set, &r->vmlr_node); + r->in_terminated_set = true; + } + /* In a forked child every other Ractor is terminated-unjoined, so keep its objspace * enumerable until a join or a global GC merges it. */ if (r->objspace) { diff --git a/ractor_core.h b/ractor_core.h index 5829dc9aca7cbe..b3aa9df08483e6 100644 --- a/ractor_core.h +++ b/ractor_core.h @@ -75,6 +75,10 @@ enum ractor_status { ractor_terminated, }; +struct rb_ractor_registered_addr { + VALUE *addr; +}; + struct rb_ractor_struct { struct rb_ractor_pub pub; struct rb_ractor_sync sync; @@ -85,6 +89,10 @@ struct rb_ractor_struct { VALUE *registered_marks; size_t registered_marks_cnt, registered_marks_capa; + struct rb_ractor_registered_addr *registered_addrs; + size_t registered_addrs_cnt, registered_addrs_capa; + bool registered_addrs_listed; + /* traversal-API mark redirect (NULL outside a traversal). Per Ractor so a * concurrent traversal on another Ractor is never observed. A modular GC's * Ractor-less marking worker threads read vm->gc.mark_func_data instead. */ @@ -171,6 +179,8 @@ void rb_ractor_reap_dead_ports(rb_ractor_t *r); * realloc (ractor.c). */ void rb_ractor_absorb_registered_marks(rb_ractor_t *dst, rb_ractor_t *src); +void rb_ractor_absorb_registered_addrs_without_gc(rb_ractor_t *dst, rb_ractor_t *src); + enum ractor_wakeup_status { wakeup_none, wakeup_by_send, diff --git a/ractor_sync.c b/ractor_sync.c index b02e00dd65d958..b2fad46f8835f9 100644 --- a/ractor_sync.c +++ b/ractor_sync.c @@ -1054,6 +1054,7 @@ ractor_value(rb_execution_context_t *ec, VALUE self) /* Move r's rb_gc_register_mark_object pins to the joiner before the merge * below sweeps r's objspace, or the objects pinned there lose their root. */ rb_ractor_absorb_registered_marks(GET_RACTOR(), r); + rb_ractor_absorb_registered_addrs_without_gc(GET_RACTOR(), r); rb_gc_objspace_absorb_into_current(&r->objspace); diff --git a/test/-ext-/gc/test_register.rb b/test/-ext-/gc/test_register.rb index cd5fa6a5907b55..7c9da0ae292680 100644 --- a/test/-ext-/gc/test_register.rb +++ b/test/-ext-/gc/test_register.rb @@ -1,5 +1,6 @@ # frozen_string_literal: false require 'test/unit' +require 'weakref' require '-test-/gc/register' class Test_GCRegisterAddress < Test::Unit::TestCase @@ -9,4 +10,160 @@ class Test_GCRegisterAddress < Test::Unit::TestCase def test_unregister_address_keeps_other_registered_addresses assert_equal(true, Bug::GC.unregister_address_keeps_siblings?) end + + def test_registered_value_survives_ractor_value + # assert_separately: keep the test-all process Ractor-free + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require '-test-/gc/register' + + r = Ractor.new { Bug::GC.register_static("registered in child".dup) } + assert_equal(true, r.value) + + 2.times { GC.start(full_mark: true) } + assert_equal("registered in child", Bug::GC.static_slot_value) + RUBY + end + + def test_unregister_address_from_another_ractor + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require 'weakref' + require '-test-/gc/register' + + alive = 5.times.count do + ref = Thread.new { + v = "main owns this".dup + Bug::GC.register_static(v) + WeakRef.new(v) + }.value + assert_predicate(ref, :weakref_alive?) + + assert_equal(true, Ractor.new { Bug::GC.unregister_static; true }.value) + + 3.times do + GC.start(full_mark: true) + break unless ref.weakref_alive? + end + ref.weakref_alive? + end + assert_operator(alive, :<, 5, "value stayed alive after unregister in every trial") + RUBY + end + + def test_registered_value_survives_fork + omit "fork not supported" unless Process.respond_to?(:fork) + Bug::GC.register_static("pre-fork".dup) + + begin + pid = fork do + GC.start(full_mark: true) + exit!(Bug::GC.static_slot_value == "pre-fork" ? 0 : 1) + end + _, status = Process.wait2(pid) + assert_predicate(status, :success?) + ensure + Bug::GC.unregister_static + end + end + + def test_ractor_registered_value_survives_fork + omit "fork not supported" unless Process.respond_to?(:fork) + assert_separately([], <<~'RUBY') + Warning[:experimental] = false + require '-test-/gc/register' + + port = Ractor::Port.new + # Not joined: the registration must stay with the dead Ractor until + # ractor_free, and keeping r referenced prevents an early ractor_free. + r = Ractor.new(port) { |port| + Bug::GC.register_static("MARKER" * 10) + port.send(:ok) + } + port.receive + + err = "#{ENV['TMPDIR'] || '/tmp'}/reg_fork_#{Process.pid}.log" + pid = fork do + $stderr.reopen(err, "w") + GC.start + 100_000.times { "x" * 100 } + exit!(Bug::GC.static_slot_eq?("MARKER" * 10) ? 0 : 1) + end + _, status = Process.wait2(pid) + unless status.success? + detail = File.exist?(err) ? File.read(err) : "" + flunk(detail.empty? ? "registered value lost after fork+GC (#{status})" : detail) + end + File.unlink(err) + r.value # joins only after the scenario ran; keeps the wrapper alive until then + RUBY + end + + def test_reachable_objects_from_root_with_terminated_ractor_registrations + # A joined (not value'd) Ractor keeps its registrations until ractor_free, so it + # sits in vm->ractor.terminated_set with a non-empty list. A single-objspace GC + # (mmtk) walks that set under registered_addrs.lock; the traversal API runs that + # walk outside a GC and must not relock the same mutex. + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require '-test-/gc/register' + require 'objspace' + + r = Ractor.new { Bug::GC.register_static("registered in child".dup) } + r.join + Thread.pass until r.inspect.include?("terminated") + + assert_kind_of(Hash, ObjectSpace.reachable_objects_from_root) + # r is still referenced here, so the Ractor was unfreed (in terminated_set) + # throughout the walk above. + assert_include(r.inspect, "terminated") + RUBY + end + + def test_verify_internal_consistency_with_ractor_stored_values + omit "needs GC.verify_internal_consistency" unless GC.respond_to?(:verify_internal_consistency) + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require '-test-/gc/register' + + Bug::GC.register_static(0) + + port = Ractor::Port.new + r = Ractor.new(port) do |port| + Bug::GC.assign_static(Ractor.make_shareable("shareable".dup)) + port.send(:stored) + Ractor.receive + end + port.receive + GC.verify_internal_consistency + + Bug::GC.assign_static("main owns this".dup) + GC.verify_internal_consistency + + r.send(:done) + r.value + RUBY + end + + def test_verify_internal_consistency_with_many_registered_addresses_and_gc_stress + omit "needs GC.verify_internal_consistency" unless GC.respond_to?(:verify_internal_consistency) + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require '-test-/gc/register' + port = Ractor::Port.new + r = Ractor.new(port) { |port| port.send(:ready); Ractor.receive } + 17.times { Bug::GC.register_static(0) } + port.receive + begin + GC.stress = true + GC.verify_internal_consistency + ensure + GC.stress = false + 17.times { Bug::GC.unregister_static } + end + + r.send(:done) + r.value + RUBY + end end diff --git a/thread.c b/thread.c index ffee3a1bae3a83..41f15f7f726e42 100644 --- a/thread.c +++ b/thread.c @@ -5318,7 +5318,8 @@ rb_thread_atfork_internal(rb_thread_t *th, void (*atfork)(rb_thread_t *, const r rb_signal_atfork(); // OK. Only this thread accesses: - ccan_list_for_each(&vm->ractor.set, r, vmlr_node) { + rb_ractor_t *r_next; + ccan_list_for_each_safe(&vm->ractor.set, r, r_next, vmlr_node) { if (r != vm->ractor.main_ractor) { rb_ractor_terminate_atfork(vm, r); } @@ -5326,7 +5327,8 @@ rb_thread_atfork_internal(rb_thread_t *th, void (*atfork)(rb_thread_t *, const r atfork(i, th); } } - rb_vm_living_threads_init(vm); + + ccan_list_head_init(&vm->ractor.set); rb_ractor_atfork(vm, th); rb_vm_postponed_job_atfork(); diff --git a/vm.c b/vm.c index 4dd49f0db47fdb..726479a49f3ac9 100644 --- a/vm.c +++ b/vm.c @@ -3647,6 +3647,10 @@ ruby_vm_destruct(rb_vm_t *vm) } rb_objspace_free(objspace); } + + if (rb_free_at_exit) { + free(vm->gc.registered_addrs.registry); + } rb_native_mutex_destroy(&vm->once_lock); rb_native_cond_destroy(&vm->once_cond); /* after freeing objspace, you *can't* use ruby_xfree() */ @@ -4945,6 +4949,7 @@ Init_BareVM(void) /* The boot objspace belongs to the main Ractor, so the main Ractor has to exist * before rb_gc_init_objspaces allocates it. */ vm->ractor.main_ractor = rb_ractor_main_alloc(); + rb_native_mutex_initialize(&vm->gc.registered_addrs.lock); rb_gc_init_objspaces(); vm->ractor.main_ractor->newobj_cache = rb_gc_ractor_cache_alloc(vm->ractor.main_ractor); rb_id_table_init(&vm->negative_cme_table, 16); @@ -4969,7 +4974,6 @@ Init_BareVM(void) rb_native_mutex_initialize(&vm->ractor.sync.lock); rb_native_cond_initialize(&vm->ractor.sync.terminate_cond); rb_native_mutex_initialize(&vm->ractor.generic_fields_lock); - rb_native_mutex_initialize(&vm->gc.registered_globals.lock); vm->gc.orphan_merge_pjob = POSTPONED_JOB_HANDLE_INVALID; vm_opt_method_def_table = st_init_numtable(); diff --git a/vm_core.h b/vm_core.h index 9723d1151ff139..0048cc7a47ac5b 100644 --- a/vm_core.h +++ b/vm_core.h @@ -818,14 +818,11 @@ typedef struct rb_vm_struct { #if USE_MODULAR_GC struct gc_mark_func_data_struct *mark_func_data; #endif - /* One VM-wide list for rb_gc_register_address: a slot can later hold another - * objspace's value, so it is not split per Ractor and every Ractor's GC scans it - * conservatively. Leaf lock; register/unregister are cold paths. */ struct { rb_nativethread_lock_t lock; - VALUE **addrs; /* rb_gc_register_address: mark_maybe on *addr */ - size_t addrs_cnt, addrs_capa; - } registered_globals; + struct rb_ractor_struct **registry; + size_t registry_cnt, registry_capa; + } registered_addrs; /* Holders keeping GC disabled (atomic): Ractors that called GC.disable (at * most one hold each) plus short internal critical sections. One holder stops From bca1314ef7c12819778d1822d6b5cc2b781e8b12 Mon Sep 17 00:00:00 2001 From: Matt Valentine-House Date: Tue, 22 Sep 2026 10:54:58 +0100 Subject: [PATCH 5/5] GC: verify consistency of Ractor local registered addresses. Now that registrations are Ractor-scoped, a store of another Ractor's unshareable object through a registered address is a use-after-free waiting for the owner's next GC. This commit introduces a method to catch it in debug and ASAN builds by recording each VALUE at registration time so that when gc_verify_internal_consistency walks the registration list it can flag live, non-shareable values that are owned by a Ractor other than the registrant. --- gc.c | 53 +++++++++++++ gc/default/default.c | 40 ++++++++++ gc/gc.h | 6 ++ ractor_core.h | 16 ++++ test/-ext-/gc/test_register.rb | 77 +++++++++++++++++++ test/.excludes-mmtk/Test_GCRegisterAddress.rb | 1 + 6 files changed, 193 insertions(+) create mode 100644 test/.excludes-mmtk/Test_GCRegisterAddress.rb diff --git a/gc.c b/gc.c index 340d8dc38ec84c..f618f4569c452f 100644 --- a/gc.c +++ b/gc.c @@ -4011,6 +4011,56 @@ rb_gc_registered_addrs_unenroll_without_gc(rb_vm_t *vm, rb_ractor_t *r) r->registered_addrs_listed = false; } +void +rb_gc_each_registered_addr(rb_gc_registered_addr_cb func, void *data) +{ +#if !RB_GC_REGISTERED_ADDR_CHECK + /* Production entries carry no provenance, so the verifier has nothing to check. */ + (void)func; + (void)data; + return; +#else + rb_vm_t *vm = GET_VM(); + for (size_t i = 0; i < vm->gc.registered_addrs.registry_cnt; i++) { + rb_ractor_t *r = vm->gc.registered_addrs.registry[i]; + for (size_t j = 0; j < r->registered_addrs_cnt; j++) { + struct rb_ractor_registered_addr *entry = &r->registered_addrs[j]; + func(entry->addr, entry->initial_value, (void *)r->objspace, data); + } + } +#endif +} + +/* True if the same address is registered by a Ractor whose objspace is the given one; + * that registrant's local GC can root the value, so the verifier accepts it. Called + * from the consistency verifier with the world stopped, so no lock is taken here. */ +bool +rb_gc_registered_addr_owned_by_registrant_p(VALUE *addr, void *objspace) +{ + rb_vm_t *vm = GET_VM(); + for (size_t i = 0; i < vm->gc.registered_addrs.registry_cnt; i++) { + rb_ractor_t *r = vm->gc.registered_addrs.registry[i]; + if ((void *)r->objspace != objspace) continue; + for (size_t j = 0; j < r->registered_addrs_cnt; j++) { + if (r->registered_addrs[j].addr == addr) return true; + } + } + return false; +} + +/* True while the objspace is a zombie pending merge. Registration ownership moves to + * the inheritor before the zombie merge completes, so a value still owned by a zombie + * is a safe transient for the verifier. */ +bool +rb_gc_vm_zombie_objspace_p(void *objspace) +{ + rb_vm_t *vm = GET_VM(); + for (size_t i = 0; i < vm->gc.zombie_objspaces_count; i++) { + if (vm->gc.zombie_objspaces[i].objspace == objspace) return true; + } + return false; +} + static bool gc_registered_addrs_remove(rb_ractor_t *r, VALUE *addr) { @@ -4042,6 +4092,9 @@ rb_gc_register_address(VALUE *addr) } struct rb_ractor_registered_addr *entry = &owner->registered_addrs[owner->registered_addrs_cnt]; entry->addr = addr; +#if RB_GC_REGISTERED_ADDR_CHECK + entry->initial_value = *addr; +#endif owner->registered_addrs_cnt++; rb_gc_registered_addrs_enroll_without_gc(vm, owner); rb_native_mutex_unlock(&vm->gc.registered_addrs.lock); diff --git a/gc/default/default.c b/gc/default/default.c index 9aa1c046f9d1fa..8c57effedf7135 100644 --- a/gc/default/default.c +++ b/gc/default/default.c @@ -7286,6 +7286,42 @@ gc_verify_heap_pages(rb_objspace_t *objspace) return remembered_old_objects; } +static void +verify_registered_addr(VALUE *slot, VALUE initial_value, void *owner_objspace, void *d) +{ + struct verify_internal_consistency_struct *data = d; + VALUE v = *slot; + + /* Conservative registration permits uninitialized data and pre-registration + * values; only a store made after registration is a violation. */ + if (v == initial_value) return; + if (SPECIAL_CONST_P(v)) return; + if (!verify_pointer_in_any_heap_p((void *)v)) return; + + bool live = false; + asan_unpoisoning_object(v) { + live = BUILTIN_TYPE(v) != T_NONE && BUILTIN_TYPE(v) != T_ZOMBIE; + } + if (!live) return; + + rb_objspace_t *value_objspace = GET_HEAP_OBJSPACE(v); + if (value_objspace == (rb_objspace_t *)owner_objspace) return; + /* Join and orphan handling move a registration to the inheritor before the + * source objspace merge; a global GC scans every registry while the zombie + * exists, so this is a safe transient exemption. */ + if (rb_gc_vm_zombie_objspace_p(value_objspace)) return; + if (value_objspace->flags.during_postmortem) return; + if (MARKED_IN_BITMAP(GET_HEAP_SHAREABLE_BITS(v), v)) return; + if (MARKED_IN_BITMAP(GET_HEAP_SHREF_BITS(v), v)) return; + /* When multiple Ractors register one address, ownership by any registrant is + * enough to root the value. */ + if (rb_gc_registered_addr_owned_by_registrant_p(slot, value_objspace)) return; + + fprintf(stderr, "registered address %p changed since registration to an unshareable object owned by another Ractor: %s\n", + (void *)slot, rb_obj_info(v)); + data->err_count++; +} + static void gc_verify_internal_consistency_(rb_objspace_t *objspace, bool world_stopped) { @@ -7314,6 +7350,10 @@ gc_verify_internal_consistency_(rb_objspace_t *objspace, bool world_stopped) rb_objspace_reachable_objects_from_root(root_scope_check_i, &data); } + if (data.world_stopped && !global_objspace->during_absorb) { + rb_gc_each_registered_addr(verify_registered_addr, &data); + } + if (data.err_count != 0) { #if RGENGC_CHECK_MODE >= 5 objspace->rgengc.error_count = data.err_count; diff --git a/gc/gc.h b/gc/gc.h index 3360744bb02ab4..c6b92dfd7f2955 100644 --- a/gc/gc.h +++ b/gc/gc.h @@ -68,12 +68,18 @@ bool ruby_free_at_exit_p(void); void rb_objspace_reachable_objects_from_root(void (func)(const char *category, VALUE, void *), void *passing_data); void rb_gc_verify_shareable(VALUE); +typedef void (*rb_gc_registered_addr_cb)(VALUE *slot, VALUE initial_value, void *owner_objspace, void *data); + MODULAR_GC_FN unsigned int rb_gc_vm_lock(const char *file, int line); MODULAR_GC_FN void rb_gc_vm_unlock(unsigned int lev, const char *file, int line); MODULAR_GC_FN unsigned int rb_gc_vm_lock_no_barrier(const char *file, int line); MODULAR_GC_FN void rb_gc_vm_unlock_no_barrier(unsigned int lev, const char *file, int line); MODULAR_GC_FN void rb_gc_vm_barrier(void); MODULAR_GC_FN void rb_gc_vm_each_objspace(void (*func)(void *objspace, void *data), void *data); +MODULAR_GC_FN void rb_gc_each_registered_addr(rb_gc_registered_addr_cb func, void *data); +/* Verifier support, valid only with the world stopped (no extra locking inside). */ +MODULAR_GC_FN bool rb_gc_registered_addr_owned_by_registrant_p(VALUE *addr, void *objspace); +MODULAR_GC_FN bool rb_gc_vm_zombie_objspace_p(void *objspace); MODULAR_GC_FN size_t rb_gc_vm_zombie_total_pages(void); MODULAR_GC_FN unsigned int rb_gc_vm_ractor_count(void); MODULAR_GC_FN void rb_gc_vm_refresh_zombie_pages(void); diff --git a/ractor_core.h b/ractor_core.h index b3aa9df08483e6..ecf28f1827279f 100644 --- a/ractor_core.h +++ b/ractor_core.h @@ -1,6 +1,7 @@ #ifndef RUBY_RACTOR_CORE_H #define RUBY_RACTOR_CORE_H #include "internal/gc.h" +#include "internal/sanitizers.h" #include "ruby/ruby.h" #include "ruby/ractor.h" #include "vm_core.h" @@ -12,6 +13,17 @@ #define RACTOR_CHECK_MODE (VM_CHECK_MODE || RUBY_DEBUG) && (SIZEOF_UINT64_T == SIZEOF_VALUE) #endif +/* Record the registration-time VALUE in each rb_gc_register_address entry so the + * verifier can distinguish a transitioned cross-Ractor store from conservative + * garbage. Debug and ASAN builds only; production keeps one-word entries. */ +#ifndef RB_GC_REGISTERED_ADDR_CHECK +# if RUBY_DEBUG || defined(RUBY_ASAN_ENABLED) +# define RB_GC_REGISTERED_ADDR_CHECK 1 +# else +# define RB_GC_REGISTERED_ADDR_CHECK 0 +# endif +#endif + // experimental flag because it is not sure it is the common pattern #define RUBY_TYPED_FROZEN_SHAREABLE_NO_REC RUBY_FL_FINALIZE @@ -77,6 +89,10 @@ enum ractor_status { struct rb_ractor_registered_addr { VALUE *addr; +#if RB_GC_REGISTERED_ADDR_CHECK + /* A raw provenance snapshot for verification only. Never mark or dereference it. */ + VALUE initial_value; +#endif }; struct rb_ractor_struct { diff --git a/test/-ext-/gc/test_register.rb b/test/-ext-/gc/test_register.rb index 7c9da0ae292680..84a069fcad51cd 100644 --- a/test/-ext-/gc/test_register.rb +++ b/test/-ext-/gc/test_register.rb @@ -145,6 +145,83 @@ def test_verify_internal_consistency_with_ractor_stored_values RUBY end + def test_verify_internal_consistency_fails_on_foreign_unshareable_store + omit "needs GC.verify_internal_consistency with the registered-address check" unless + GC.respond_to?(:verify_internal_consistency) && Bug::GC.registered_address_check_enabled? + assert_in_out_err([], <<~RUBY, [], /registered address .* changed since registration to an unshareable object owned by another Ractor/, success: false) + require '-test-/gc/register' + Bug::GC.register_static(0) + port = Ractor::Port.new + Ractor.new(port) do |port| + child_string = "foreign".dup + Bug::GC.assign_static(child_string) + port.send(:stored) + Ractor.receive + child_string + end + port.receive + GC.verify_internal_consistency + RUBY + end + + def test_verify_internal_consistency_ignores_registration_time_value + omit "needs the registered-address check" unless Bug::GC.registered_address_check_enabled? + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require '-test-/gc/register' + + port = Ractor::Port.new + r = Ractor.new(port) do |port| + child_string = "registered in child".dup + Bug::GC.assign_static(child_string) + port.send(:stored) + Ractor.receive + child_string + end + port.receive + + # The slot already holds the child's string at registration time, so the + # registration-time snapshot matches and the verifier must not fail. + Bug::GC.register_current_static + GC.verify_internal_consistency + Bug::GC.unregister_static + + r.send(:done) + r.value + RUBY + end + + def test_verify_internal_consistency_ignores_cross_ractor_duplicate_registration + omit "needs the registered-address check" unless Bug::GC.registered_address_check_enabled? + assert_separately([], <<~RUBY) + Warning[:experimental] = false + require '-test-/gc/register' + + Bug::GC.register_static(0) + + port = Ractor::Port.new + r = Ractor.new(port) do |port| + child_string = "registered in child".dup + Bug::GC.register_static(child_string) + port.send(:stored) + Ractor.receive + Bug::GC.unregister_static + port.send(:unregistered) + child_string + end + port.receive + + # Main's entry is stale (initial value 0, current value owned by the child), + # but the child registrant owns the value, so the verifier must not fail. + GC.verify_internal_consistency + + r.send(:done) + port.receive + Bug::GC.unregister_static + r.value + RUBY + end + def test_verify_internal_consistency_with_many_registered_addresses_and_gc_stress omit "needs GC.verify_internal_consistency" unless GC.respond_to?(:verify_internal_consistency) assert_separately([], <<~RUBY) diff --git a/test/.excludes-mmtk/Test_GCRegisterAddress.rb b/test/.excludes-mmtk/Test_GCRegisterAddress.rb new file mode 100644 index 00000000000000..369c1f005a19a2 --- /dev/null +++ b/test/.excludes-mmtk/Test_GCRegisterAddress.rb @@ -0,0 +1 @@ +exclude(:test_verify_internal_consistency_fails_on_foreign_unshareable_store, "the registered-address check lives in the default GC implementation")