Bug #4911

timer_thread_function() が thead unsafe

Added by Motohiro KOSAKI almost 3 years ago. Updated over 1 year ago.

[ruby-dev:43859]
Status:Closed
Priority:Normal
Assignee:Motohiro KOSAKI
Category:core
Target version:2.0.0
ruby -v:trunk Backport:

Description

どうして、これを先月気づかなかったのだろう。という罪悪感があるのですが、
target 1.9.x で起票します。

timerthreadfunction()で vm->running_thread にアクセスするのは
thread unsafe な気がします

0.スレッドAがrunningthreadである
1.タイマースレッドがvm->running
threadをレジスタにのせる
(レジスタにスレッドAのアドレスがのっかる)
2.コンテキストスイッチ
3.スレッドAが終了。スレッドBがrunningthreadになる
4.スレッドAの rb
theadt がfreeされる
5.コンテキストスイッチ
6.そんなkとはつゆしらず、タイマースレッドはスレッドAの
アドレスに対して th->interrupt
flag |= 1; するのでメモリ破壊


static void
timerthreadfunction(void arg)
{
rbvmt *vm = GET_VM(); /
TODO: fix me for Multi-VM */

/* for time slice */
RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);      ※ここ

/* check signal */
rb_threadptr_check_signal(vm->main_thread);

}

Associated revisions

Revision 38063
Added by Motohiro KOSAKI over 1 year ago

  • vmcore.h (rbvmstruct): add threaddestruct_lock field.
  • thread.c (Init_Thread): ditto.
  • thread.c (rbvmgvl_destroy): ditto.

  • thread.c (threadstartfunc2): make sure vm->runningthread
    don't point to dead thread.

  • thread.c (timerthreadfunction): close a race against thead
    destruction. [Bug #4911]

  • vmcore.h (rbthreadsetcurrent): reset running time of
    current thread instead of previous thread. We no longer
    assume previous running thread still live.

History

#1 Updated by Shyouhei Urabe about 2 years ago

  • Status changed from Open to Assigned

#2 Updated by Motohiro KOSAKI over 1 year ago

  • Assignee changed from Motohiro KOSAKI to Koichi Sasada

I've made a patch.

https://github.com/ruby/ruby/pull/182

ko1, could you please review it?

#3 Updated by Koichi Sasada over 1 year ago

問題はわかったのですが,
これがどうして解決するのかわかりません.
(結局,+ if (!vm->runningthread->yielding) の部分で runningthread を触っているように見える)

解説してもらえないでしょうか.


では,どうやって解決するかというと難しいですね.タイマスレッドが動いている間は free しない,とか,そういうのになりそうな感じ.

#4 Updated by Koichi Sasada over 1 year ago

(2012/09/21 15:10), ko1 (Koichi Sasada) wrote:

では,どうやって解決するかというと難しいですね.タイマスレッドが動いている間は free しない,とか,そういうのになりそうな感じ.

勘で書いてみたんですが,こんな感じでしょうか.再現が出来なさそうなので,
テストの書きようが無さそう.

Index: vmcore.h
===================================================================
--- vm
core.h (revision 37007)
+++ vmcore.h (working copy)
@@ -312,6 +312,7 @@ typedef struct rb
vmstruct {
int thread
abortonexception;
unsigned long traceflag;
volatile int sleeper;
+ volatile struct rb
threadstruct *timerthread_target;

  /* object management */
  VALUE mark_object_ary;

Index: thread.c
===================================================================
--- thread.c (revision 37007)
+++ thread.c (working copy)
@@ -3388,9 +3388,13 @@ timerthreadfunction(void arg)
rbvmt *vm = GET_VM(); /
TODO: fix me for Multi-VM */

  /* for time slice */
  • vm->timerthreadtarget = vm->running_thread;
  • { if (!vm->runningthread->yielding) { RUBYVMSETTIMERINTERRUPT(vm->runningthread); }
  • }
  • vm->timerthreadtarget = 0;

    /* check signal */
    rbthreadptrchecksignal(vm->mainthread);

    Index: vm.c

    --- vm.c (revision 37007)
    +++ vm.c (working copy)
    @@ -1724,6 +1724,9 @@ thread_free(void *ptr)
    free(th->altstack);
    }
    #endif

  •  while (GET_VM()->timer_thread_target == ptr) {
    
  •  rb_thread_schedule(); /* Timer thread seeing my thread */
    
  •  }
    ruby_xfree(ptr);
    

    }
    if (rubycurrentthread == th)

    // SASADA Koichi at atdot dot net

#5 Updated by Motohiro KOSAKI over 1 year ago

問題はわかったのですが,
これがどうして解決するのかわかりません.
(結局,+ if (!vm->runningthread->yielding) の部分で runningthread を触っているように見える)
解説してもらえないでしょうか.

あ、しまった。放置しすぎていてコンテキストスイッチだけを救うパッチを書いってしまった。本当に問題なのはスイッチ元のスレッドがfreeされていてメモリ破壊に至るケースなのでおっしゃるとおり考慮不足です

#6 Updated by Motohiro KOSAKI over 1 year ago

で、Comment#4のパッチだと残念なことにタイマースレッドとスレッド解放ルートに共通のロックがないため、グローバル変数に代入してもメモリバリア的な意味でthread_free() からは可視にならなさそう

#7 Updated by Koichi Sasada over 1 year ago

(2012/09/21 17:44), kosaki (Motohiro KOSAKI) wrote:

で、Comment#4のパッチだと残念なことにタイマースレッドとスレッド解放ルートに共通のロックがないため、グローバル変数に代入してもメモリバリア的な意味でthread_free() からは可視にならなさそう

うーん,なるほど.
真面目に mutex で囲いますかねぇ.それ以外の冴えたやり方はないかなぁ.
別に,ここでオーバヘッドになることもないだろうから,いっか.

--
// SASADA Koichi at atdot dot net

#8 Updated by Koichi Sasada over 1 year ago

  • Assignee changed from Koichi Sasada to Motohiro KOSAKI

これは忘れてはいけなかった気がする.
小崎先生お願いします.

#9 Updated by Motohiro KOSAKI over 1 year ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r38063.
Motohiro, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • vmcore.h (rbvmstruct): add threaddestruct_lock field.
  • thread.c (Init_Thread): ditto.
  • thread.c (rbvmgvl_destroy): ditto.

  • thread.c (threadstartfunc2): make sure vm->runningthread
    don't point to dead thread.

  • thread.c (timerthreadfunction): close a race against thead
    destruction. [Bug #4911]

  • vmcore.h (rbthreadsetcurrent): reset running time of
    current thread instead of previous thread. We no longer
    assume previous running thread still live.

Also available in: Atom PDF