Bug #2777
Invalid read of size 4 by redefining load
| Status: | Closed | Start date: | ||
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: |  Koichi Sasada |
% Done: | 100% |
|
| Category: | - | |||
| Target version: | 1.9.2 | |||
| ruby -v: | - |
Description
なかだです。 At Mon, 22 Feb 2010 01:20:07 +0900, Tanaka Akira wrote in [ruby-dev:40452]: > 以下のように load 中に load を再定義すると、変なところをアクセスするのが > valgrind で観測されます。 rb_method_entry_tにもrefcountを入れますかねぇ。 -- --- 僕の前にBugはない。 --- 僕の後ろにBugはできる。 中田 伸悦
Associated revisions
* vm_method.c (rb_unlink_method_entry, rb_sweep_method_entry):
added. Unlinked method entries are collected to
vm->unlinked_method_entry_list. On the GC timing, mark all method
entries which are on all living threads. Only non-marked method
entries are collected. This hack prevents releasing living method
entry.
[Performance Consideration] Since this Method Entry GC (MEGC)
doesn't occuer frequently, MEGC will not be a performance bottleneck.
However, to traverse living method entries, every control frame push
needs to clear cfp->me field. This will be a performance issue
(because pushing control frame is occurred frequently).
Bug #2777 [ruby-dev:40457]
* cont.c (fiber_init): init cfp->me.
* gc.c (garbage_collect): kick rb_sweep_method_entry().
* method.h (rb_method_entry_t): add a mark field.
* vm.c (invoke_block_from_c): set passed me.
* vm.c (rb_thread_mark): mark cfp->me.
* vm_core.h (rb_thread_t): add a field passed_me.
* vm_core.h (rb_vm_t): add a field unlinked_method_entry_list.
* vm_insnhelper.c (vm_push_frame): clear cfp->me at all times.
* vm_insnhelper.c (vm_call_bmethod): pass me.
* bootstraptest/test_method.rb: add a test.
* vm_method.c (rb_unlink_method_entry, rb_sweep_method_entry):
added. Unlinked method entries are collected to
vm->unlinked_method_entry_list. On the GC timing, mark all method
entries which are on all living threads. Only non-marked method
entries are collected. This hack prevents releasing living method
entry.
[Performance Consideration] Since this Method Entry GC (MEGC)
doesn't occuer frequently, MEGC will not be a performance bottleneck.
However, to traverse living method entries, every control frame push
needs to clear cfp->me field. This will be a performance issue
(because pushing control frame is occurred frequently).
Bug #2777 [ruby-dev:40457]
* cont.c (fiber_init): init cfp->me.
* gc.c (garbage_collect): kick rb_sweep_method_entry().
* method.h (rb_method_entry_t): add a mark field.
* vm.c (invoke_block_from_c): set passed me.
* vm.c (rb_thread_mark): mark cfp->me.
* vm_core.h (rb_thread_t): add a field passed_me.
* vm_core.h (rb_vm_t): add a field unlinked_method_entry_list.
* vm_insnhelper.c (vm_push_frame): clear cfp->me at all times.
* vm_insnhelper.c (vm_call_bmethod): pass me.
* bootstraptest/test_method.rb: add a test.
History
Updated by Koichi Sasada almost 2 years ago
(2010/02/22 15:03), Nobuyoshi Nakada wrote:: > rb_method_entry_tにもrefcountを入れますかねぇ。 性能の観点から反対します． さっきの IRC に書いた内容ですが，こんな感じでどうでしょうか．大仰な気 もしますが． (1) unlink された me を保存しておく (2) 一定のタイミング（多分，GCタイミング）でフレームを走査して 保存した me に mark (3) mark がついてないのを消す（sweep） NODE に戻すってのも一案ですが...． -- // SASADA Koichi at atdot dot net
Updated by Yusuke Endoh almost 2 years ago
- Assignee set to Koichi Sasada
- Priority changed from Low to Normal
- Target version set to 1.9.2
- ruby -v set to -
遠藤です。
> % cat tst.rb
> module Kernel
> def load(*args)
> end
> end
> raise
> % valgrind ./ruby -ve 'load "tst.rb"'
*snip*
というオリジナルの問題は r27393 で巧妙に回避されましたが、本質的には
解決しておらず、以下で SEGV します。
class C
define_method(:foo) do
C.class_eval { remove_method(:foo) }
super()
end
end
C.new.foo
あと、r27393 だと rb_method_definition_t の参照を減らしていないので
メモリリークするような気がします。
diff --git a/vm_method.c b/vm_method.c
index 04b62f2..c9d99db 100644
--- a/vm_method.c
+++ b/vm_method.c
@@ -215,6 +215,14 @@ rb_add_method_def(VALUE klass, ID mid, rb_method_type_t type, rb_method_definiti
* another problem when the usage is changed.
*/
me = old_me;
+
+ if (me->def) {
+ if (me->def->alias_count == 0)
+ xfree(me->def);
+ else if (me->def->alias_count > 0)
+ me->def->alias_count--;
+ me->def = 0;
+ }
}
else {
me = ALLOC(rb_method_entry_t);
--
Yusuke Endoh <mame@tsg.ne.jp>
Updated by Koichi Sasada almost 2 years ago
- Status changed from Open to Closed
- % Done changed from 0 to 100
This issue was solved with changeset r27634. Nobuyoshi, thank you for reporting this issue. Your contribution to Ruby is greatly appreciated. May Ruby be with you.