Bug #4765: signal が正しくマスクされておらず main thread でシグナルハンドラが動いている

• Status: Closed
• Assignee: kosaki (Motohiro KOSAKI)
• Target version: 2.0.0
• ruby -v: ruby 1.9.3dev (2011-05-21 trunk 31654) [x86_64-linux]

[kosaki (Motohiro KOSAKI)]

Bug#4027 から派生させます

しかしメインスレッドその他の Thread に対応するスレッドのシグナルマスクは SIGSEGV と SIGVTALRM のみ
外されているはずなのに、全て外されていました。
このため sighandler() がタイマースレッドでなくメインスレッドで実行されています。
これは以下のような流れでおきています。

• init_sigchld で初期状態(空マスク)の sigmask が trap_last_sigmask に保存される
• タイマースレッドの起動(rb_thread_create_timer_thread)でメインスレッドはマスクがセットされる
• その後ファイルロード中に例外が生成された時に rb_trap_restore_mask() が呼ばれて trap_last_sigmask に
  保存された空マスクがセットされるためシグナルマスクが外れる

また、Signal.trap でハンドラをセットした時に、そのシグナルのシグナルマスクが外されています。
sigaction によるシグナルハンドラの実行はタイマースレッドにまかせているはずなのでこれは不要だと思います。
また例外発生時に rb_trap_restore_mask を読んでいるのも、trap() で例外が発生しても trap_ensure() で
シグナルマスクを戻す処理は行なわれているので不要なような気がします。元々どういう理由で呼ばれているのか
わからなかったので自信ないですけど。

レビューした結果、rb_trap_restore_mask()がまったく不要だという意見に賛成します。

というわけで添付のようなパッチを作成してみました。

ところがこれを当てると make test-all で Failure が 2つ増えます。
いずれも ruby のプロセスをシグナルで止めた時に終了ステータスが $?.signaled? == true でなく
$?.exited? == true になるためです。
これはどう直せばいいのかわかりません。

1. Failure:
   test_should_propagate_signaled(TestBeginEndBlock) [/Users/nagachika/opt/ruby-trunk/src/ruby-trunk/test/ruby/test_beginendblock.rb:108]:
   Expected 0 to be nil.

2. Failure:
   test_status_kill(TestProcess) [/Users/nagachika/opt/ruby-trunk/src/ruby-trunk/test/ruby/test_process.rb:1073]:
   [s.exited?, s.signaled?, s.stopped?].
   <[false, true, false]> expected but was
   <[true, false, false]>.

原因は rb_syswait()にあります。現状 Process.wait()中は なぜか SIGHUP, SIGQUIT, SIGINTを SIG_IGNに設定してしまうため
この間に別スレッドが SIGQUITを送るようなテストは正しく動きません。
そもそも、signal handlerはプロセスグローバルなので、処理の途中で一時的に SIG_IGNにしてはいけません。他のスレッドが
迷惑します。pthread_sigmask()に差し替えることを検討したのですが、そもそもシグナルがタイマースレッドにしか配送されない
という設計を貫く限り、いかなる処理も必要ないという結論になりました。

まつもとさん、この処理の実装はまつもとさんのように見えます。10年以上前なので無理かもしれませんが、覚えていたら
SIG_IGNを設定していた理由を教えてください。

また、sighandler() がメインスレッドから呼ばれるのは Process.spawn を実行するとまだおきてしまいます。
process.c の before_fork/after_fork で fork の時に一時的にタイマースレッドを止めてシグナルマスクを外すためです。
こちらもどうしていいものかわからないです。
fork してそのまま動き続けるようなものはどうしようもないような気がしますが、spawn するものは
fork 後に sigprogmask で外すようにするなどでなんとかならないものでしょうか。
あーでもタイマースレッドの再起動で結局一時的に外してしまいますね……

これも、一時的にシグナルマスクを一時的に外す処理自体が不要だと思います。

また、現状、chikanagaさんの報告よりも、さらに状況は悪化しており、シグナル処理を直すと
test_signal.rb#test_kill_immediately_before_termination でテストがハングします。
これは、くだんのテストが子プロセスにSIGINT送って送出されてくる例外を確認しているのですが、
test/unit/parallel.rb#run() に Signal.trap(:INT,"IGNORE") という極悪な文があるため、
テストでSIGINTが動かなくなる仕様変更がこっそり行われているためです。

soraさん、この行の意図はなんですか？代替案を考える必要があると思いますが、このような
子プロセスに伝搬する設定をこっそり行うのは許容できないと思います。テストに支障が出るし
第一 Ctrl-C がなかなか効かなくて test-all するときにイライラします。

ついでに以下の変更を行いました
・SIGPIPEのハンドラを空関数からSIG_IGNに変更。空関数にするぐらいならユーザ空間に
処理を戻すだけ無駄。それに、ざっと見たところrubyの中でEPIPEをハンドリングしてない場所はない
・処理の最後で、ruby_default_signal() でシグナルハンドラをSIG_DFLに戻している箇所は
シグナルマスクも解除するよう変更

添付のパッチで test-all が全て通ることを確認出来ています。

これは当面コミットせずに1.9.4に回そうと思っています。理由は
・これを直さないと致命的な状況になるような bug report はきてない
・長年放置されていたせいで、すでに複数箇所いまの挙動に依存するコードが見つかっており
安定化に時間がかかる可能性がある

と考えているため

[sorah (Sorah Fukumori)]

Motohiro KOSAKI wrote:

これは、くだんのテストが子プロセスにSIGINT送って送出されてくる例外を確認しているのですが、
test/unit/parallel.rb#run() に Signal.trap(:INT,"IGNORE") という極悪な文があるため、
テストでSIGINTが動かなくなる仕様変更がこっそり行われているためです。

soraさん、この行の意図はなんですか？代替案を考える必要があると思いますが、このような
子プロセスに伝搬する設定をこっそり行うのは許容できないと思います。テストに支障が出るし
第一 Ctrl-C がなかなか効かなくて test-all するときにイライラします。

実行するテストでのシグナル周りを失念していました．修正します…

[kosaki (Motohiro KOSAKI)]

• File remove-signalmask-op.patch remove-signalmask-op.patch added

ささださんと議論した結果、逆にシグナルマスク操作を全部削除してしまって全スレッドでシグナルを受けれるようにしたほうが
よいという結論にしました。改訂版パッチを添付します。
rb_syswait() の件が経緯不明のため、このパッチは1.9.4 送りの方向で考えています。

[kosaki (Motohiro KOSAKI)]

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

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

---

• thread_pthread.c (rb_thread_create_timer_thread): removed
  rb_disable_interrupt()/rb_enable_interrupt().

• vm_core.h: ditto.

• process.c (static void before_exec): ditto.

• process.c (static void after_exec): ditto.
  [Bug #4765] [ruby-dev:43571]

• eval_intern.h: removed rb_trap_restore_mask().

• vm_eval.c (rb_throw_obj): ditto.

• eval.c (setup_exception): ditto.

• signal.c: removed trap_last_mask.

• signal.c (trap_restore_mask): removed.

• signal.c (init_sigchld): comment clarification why signal block
  is needed. and removed trap_last_mask operation.

• signal.c (trap_ensure): removed trap_last_mask operation.

• signal.c (rb_disable_interrupt, rb_enable_interrupt): made
  static and removed sigdelset(SIGVTALARM) and sigdelset(SIGSEGV).

• process.c (rb_syswait): removed implicit signal handler change.