[前][次][番号順一覧][スレッド一覧]

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/

[前][次][番号順一覧][スレッド一覧]