Backport #7825
closedVM/envのマーク漏れによるSEGV
Description
辻本です。
trunk/rc2で次のコードでSEGVします(Ubuntu 12.04 x86_64)。
require 'irb'
IRB::Irb.module_eval do
define_method(:eval_input) do
IRB::Irb.module_eval { alias_method :eval_input, :to_s }
# (A)
GC.start
Kernel
end
end
IRB.start
以下がGC前(A)のフレームの構造です。
(0x555555e078c8は2行目のmodule_evalのBLOCKフレームに対応するep)
c:0006 (0x7ffff6b08e30) p:0046 s:0018 e:000017 LAMBDA test.rb:5 [FINISH]
-- prev ep(s) --
ep: 0x7ffff6a09098
ep: 0x555555e078c8
ep: 0x555555b74360
ep: 0x555555b26a88
c:0002 (0x7ffff6b08f70) p:0044 s:0005 E:0012d0 EVAL test.rb:10 [FINISH]
-- prev ep(s) --
ep: 0x555555b74360
ep: 0x555555b26a88
c:0001 (0x7ffff6b08fc0) p:0000 s:0002 E:001bf8 TOP [FINISH]
-- prev ep(s) --
ep: 0x555555b26a88
これがGC後に次のようになり、epが辿れなくなっています。
c:0006 (0x7ffff6b08e30) p:0062 s:0018 e:000017 LAMBDA test.rb:7 [FINISH]
-- prev ep(s) --
ep: 0x7ffff6a09098
ep: 0x555555e078c8
ep: 0x555555e0da20
ep: (nil)
epがスタックを指している場合に、上位(PREV)のepがヒープにあると
そのennvalがマークされずにGCされるということのようです。
ちょっと自信がありませんが、以下のパッチで直ります。
diff --git a/vm.c b/vm.c
index 36def2c..fea4a57 100644
--- a/vm.c
+++ b/vm.c
@@ -1775,10 +1775,22 @@ rb_thread_mark(void *ptr)
rb_gc_mark_locations(p, p + th->mark_stack_len);
while (cfp != limit_cfp) {
+ VALUE *ep = cfp->ep;
+ VALUE *lep = VM_CF_LEP(cfp);
rb_iseq_t *iseq = cfp->iseq;
rb_gc_mark(cfp->proc);
rb_gc_mark(cfp->self);
rb_gc_mark(cfp->klass);
+ while (1) {
+ if (ENV_IN_HEAP_P(th, ep)) {
+ rb_gc_mark(ep[1]); /* envval */
+ break;
+ }
+ if (ep == lep) {
+ break;
+ }
+ ep = VM_EP_PREV_EP(ep);
+ }
if (iseq) {
rb_gc_mark(RUBY_VM_NORMAL_ISEQ_P(iseq) ? iseq->self : (VALUE)iseq);
}
showstopper扱いになるのではないかと思いますが、いかがでしょうか。
Files
Updated by mame (Yusuke Endoh) over 11 years ago
- Assignee changed from mame (Yusuke Endoh) to ko1 (Koichi Sasada)
showstopper扱いになるのではないかと思いますが、いかがでしょうか。
そうですね。
ktsj さんのパッチなので大丈夫だとは思いますが、一応 ko1 さんに確認してもらって、よさそうならコミットしてください。
--
Yusuke Endoh mame@tsg.ne.jp
Updated by ko1 (Koichi Sasada) over 11 years ago
- Status changed from Assigned to Closed
- % Done changed from 0 to 100
This issue was solved with changeset r39276.
Kazuki, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.
- vm.c (rb_thread_mark): mark a working Proc of bmethod
(a method defined by define_method) even if the method was removed.
We could not trace working Proc object which represents the body
of bmethod if the method was removed (alias/undef/overridden).
Simply, it was mark miss.
This patch by Kazuki Tsujimoto. [Bug #7825]
NOTE: We can brush up this marking because we do not need to mark
me' on each living control frame. We need to mark
me's
only if `me' was free'ed. This is future work after Ruby 2.0.0. - test/ruby/test_method.rb: add a test.
Updated by ko1 (Koichi Sasada) over 11 years ago
- Assignee changed from ko1 (Koichi Sasada) to mame (Yusuke Endoh)
これは SEGV 起こせるバグなので、 r39276 を(検証の上)
バックポート頂けると助かります。
原因を簡単に説明すると、
- define_method で定義したメソッド foo を実行中に
- foo を別のメソッドで上書きすると
- foo の実体の Proc が解放されて大惨事
(正確には、Proc から参照される Env が解放されて大惨事)
という話でした。
最初のパッチでは Env を mark しようとしていますが、
最終的なパッチは method_entry から辿れる Proc を mark するようになっています。
PS.
ただし、この辺もっと綺麗になると思うので、2.0.0 が出た後でなんとかしたいと思っています。
Updated by ko1 (Koichi Sasada) over 11 years ago
- Status changed from Closed to Assigned
Updated by mame (Yusuke Endoh) over 11 years ago
- Assignee changed from mame (Yusuke Endoh) to ktsj (Kazuki Tsujimoto)
ktsj さんに確認してもらって、よさそうならバックポートしてもらうというのでどうでしょうか。
--
Yusuke Endoh mame@tsg.ne.jp
Updated by ktsj (Kazuki Tsujimoto) over 11 years ago
- Status changed from Assigned to Closed
> ささださん
レビューありがとうございました。
問題なさそうなので、 r39279 でバックポートしました。
Updated by ktsj (Kazuki Tsujimoto) over 10 years ago
- Tracker changed from Bug to Backport
- Project changed from Ruby master to Backport193
- Category deleted (
core) - Status changed from Closed to Assigned
- Assignee changed from ktsj (Kazuki Tsujimoto) to usa (Usaku NAKAMURA)
- Priority changed from 5 to Normal
- Target version deleted (
2.0.0)
1.9.3にも同じ問題があるので、バックポートをお願いします。
Updated by usa (Usaku NAKAMURA) over 10 years ago
- Status changed from Assigned to Closed
Applied in changeset r44736.
merge revision(s) 39276: [Backport #7825]
* vm.c (rb_thread_mark): mark a working Proc of bmethod
(a method defined by define_method) even if the method was removed.
We could not trace working Proc object which represents the body
of bmethod if the method was removed (alias/undef/overridden).
Simply, it was mark miss.
This patch by Kazuki Tsujimoto. [Bug #7825]
NOTE: We can brush up this marking because we do not need to mark
`me' on each living control frame. We need to mark `me's
only if `me' was free'ed. This is future work after Ruby 2.0.0.
* test/ruby/test_method.rb: add a test.