Backport #7825

VM/envのマーク漏れによるSEGV

Added by Kazuki Tsujimoto about 1 year ago. Updated 3 months ago.

[ruby-dev:46970]
Status:Closed
Priority:Normal
Assignee:Usaku NAKAMURA

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扱いになるのではないかと思いますが、いかがでしょうか。

backtrace.txt Magnifier (4.24 KB) Kazuki Tsujimoto, 02/11/2013 10:24 AM

Associated revisions

Revision 39276
Added by Koichi Sasada about 1 year ago

  • vm.c (rbthreadmark): 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 markme's only if `me' was free'ed. This is future work after Ruby 2.0.0.
  • test/ruby/test_method.rb: add a test.

Revision 44736
Added by Usaku NAKAMURA 3 months ago

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.

History

#1 Updated by Yusuke Endoh about 1 year ago

  • Assignee changed from Yusuke Endoh to Koichi Sasada

showstopper扱いになるのではないかと思いますが、いかがでしょうか。

そうですね。
ktsj さんのパッチなので大丈夫だとは思いますが、一応 ko1 さんに確認してもらって、よさそうならコミットしてください。

Yusuke Endoh mame@tsg.ne.jp

#2 Updated by Koichi Sasada about 1 year 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 (rbthreadmark): 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 markme's only if `me' was free'ed. This is future work after Ruby 2.0.0.
  • test/ruby/test_method.rb: add a test.

#3 Updated by Koichi Sasada about 1 year ago

  • Assignee changed from Koichi Sasada to Yusuke Endoh

これは SEGV 起こせるバグなので、 r39276 を(検証の上)
バックポート頂けると助かります。

原因を簡単に説明すると、
- define_method で定義したメソッド foo を実行中に
- foo を別のメソッドで上書きすると
- foo の実体の Proc が解放されて大惨事
(正確には、Proc から参照される Env が解放されて大惨事)

という話でした。
最初のパッチでは Env を mark しようとしていますが、
最終的なパッチは method_entry から辿れる Proc を mark するようになっています。

PS.
ただし、この辺もっと綺麗になると思うので、2.0.0 が出た後でなんとかしたいと思っています。

#4 Updated by Koichi Sasada about 1 year ago

  • Status changed from Closed to Assigned

#5 Updated by Yusuke Endoh about 1 year ago

  • Assignee changed from Yusuke Endoh to Kazuki Tsujimoto

ktsj さんに確認してもらって、よさそうならバックポートしてもらうというのでどうでしょうか。

Yusuke Endoh mame@tsg.ne.jp

#6 Updated by Kazuki Tsujimoto about 1 year ago

  • Status changed from Assigned to Closed

> ささださん
レビューありがとうございました。

問題なさそうなので、 r39279 でバックポートしました。

#7 Updated by Kazuki Tsujimoto 3 months ago

  • Tracker changed from Bug to Backport
  • Project changed from ruby-trunk to Backport93
  • Category deleted (core)
  • Status changed from Closed to Assigned
  • Assignee changed from Kazuki Tsujimoto to Usaku NAKAMURA
  • Priority changed from High to Normal
  • Target version deleted (2.0.0)

1.9.3にも同じ問題があるので、バックポートをお願いします。

#8 Updated by Kazuki Tsujimoto 3 months ago

  • Description updated (diff)

#9 Updated by Usaku NAKAMURA 3 months 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.

Also available in: Atom PDF