ruby-changes:73965
From: Yusuke <ko1@a...>
Date: Wed, 12 Oct 2022 20:28:45 +0900 (JST)
Subject: [ruby-changes:73965] 7a9f865a1d (master): Do not read cached_id from callcache on stack
https://git.ruby-lang.org/ruby.git/commit/?id=7a9f865a1d From 7a9f865a1d855109c7990b5fee21f92cc951ce60 Mon Sep 17 00:00:00 2001 From: Yusuke Endoh <mame@r...> Date: Wed, 12 Oct 2022 19:02:11 +0900 Subject: Do not read cached_id from callcache on stack The inline cache is initialized by vm_cc_attr_index_set only when vm_cc_markable(cc). However, vm_getivar attempted to read the cache even if the cc is not vm_cc_markable. This caused a condition that depends on uninitialized value. Here is an output of valgrind: ``` ==10483== Conditional jump or move depends on uninitialised value(s) ==10483== at 0x4C1D60: vm_getivar (vm_insnhelper.c:1171) ==10483== by 0x4C1D60: vm_call_ivar (vm_insnhelper.c:3257) ==10483== by 0x4E8E48: vm_call_symbol (vm_insnhelper.c:3481) ==10483== by 0x4EAD8C: vm_sendish (vm_insnhelper.c:5035) ==10483== by 0x4C62B2: vm_exec_core (insns.def:820) ==10483== by 0x4DD519: rb_vm_exec (vm.c:0) ==10483== by 0x4F00B3: invoke_block (vm.c:1417) ==10483== by 0x4F00B3: invoke_iseq_block_from_c (vm.c:1473) ==10483== by 0x4F00B3: invoke_block_from_c_bh (vm.c:1491) ==10483== by 0x4D42B6: rb_yield (vm_eval.c:0) ==10483== by 0x259128: rb_ary_each (array.c:2733) ==10483== by 0x4E8730: vm_call_cfunc_with_frame (vm_insnhelper.c:3227) ==10483== by 0x4EAD8C: vm_sendish (vm_insnhelper.c:5035) ==10483== by 0x4C6254: vm_exec_core (insns.def:801) ==10483== by 0x4DD519: rb_vm_exec (vm.c:0) ==10483== ``` In fact, the CI on FreeBSD 12 started failing since ad63b668e22e21c352b852f3119ae98a7acf99f1. ``` gmake[1]: Entering directory '/usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby' /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:924:in `complete': undefined method `complete' for nil:NilClass (NoMethodError) from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1816:in `block in visit' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1815:in `reverse_each' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1815:in `visit' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1847:in `block in complete' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1846:in `catch' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1846:in `complete' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1640:in `block in parse_in_order' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1632:in `catch' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1632:in `parse_in_order' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1626:in `order!' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1732:in `permute!' from /usr/home/chkbuild/chkbuild/tmp/build/20221011T163003Z/ruby/lib/optparse.rb:1757:in `parse!' from ./ext/extmk.rb:359:in `parse_args' from ./ext/extmk.rb:396:in `<main>' ``` This change adds a guard to read the cache only when vm_cc_markable(cc). It might be better to initialize the cache as INVALID_SHAPE_ID when the cc is not vm_cc_markable. --- vm_callinfo.h | 1 + vm_insnhelper.c | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/vm_callinfo.h b/vm_callinfo.h index f10cd9a000..3fb0fa8ca7 100644 --- a/vm_callinfo.h +++ b/vm_callinfo.h @@ -385,6 +385,7 @@ vm_cc_attr_index_dest_shape_id(const struct rb_callcache *cc) https://github.com/ruby/ruby/blob/trunk/vm_callinfo.h#L385 static inline void vm_cc_atomic_shape_and_index(const struct rb_callcache *cc, shape_id_t * shape_id, attr_index_t * index) { + VM_ASSERT(vm_cc_markable(cc)); uintptr_t cache_value = cc->aux_.attr.value; // Atomically read 64 bits *shape_id = (shape_id_t)(cache_value >> SHAPE_FLAG_SHIFT); *index = (attr_index_t)(cache_value & SHAPE_FLAG_MASK) - 1; diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 2b4eda775a..9b9220a4e3 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -1160,7 +1160,13 @@ vm_getivar(VALUE obj, ID id, const rb_iseq_t *iseq, IVC ic, const struct rb_call https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1160 attr_index_t index; if (is_attr) { - vm_cc_atomic_shape_and_index(cc, &cached_id, &index); + if (vm_cc_markable(cc)) { + vm_cc_atomic_shape_and_index(cc, &cached_id, &index); + } + else { + cached_id = INVALID_SHAPE_ID; + index = ATTR_INDEX_NOT_SET; + } } else { vm_ic_atomic_shape_and_index(ic, &cached_id, &index); -- cgit v1.2.1 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/