Ruby » ruby-trunk

遠藤です。

2010年2月5日10:50 Tanaka Akira <akr@fsij.org>:
> 2010年2月5日0:18 Yusuke ENDOH <mame@tsg.ne.jp>:
>
>> _WIN32 以外では呼ばないようにするパッチを作ってみました。
>
> 環境によっては問題が残るという話でしょうか。


そういうことになると思います。
windows でも誰かが直せるなら直せばいいですし、重大な問題ではない
ので best effort な環境では無視してもいいかと思います。

「重大でないならどの環境でも無視すればいい」というなら、「これは
仕様 (implementation-defined or dependent) であり、rubyspec に
書くべきではない」という権威あるお言葉さえ頂ければ、私はそれで
構いません。rubyspec から当該テストを消す根拠になりますので。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

遠藤です。

2010年2月5日14:05 Tanaka Akira <akr@fsij.org>:
> 2010年2月5日12:21 Yusuke ENDOH <mame@tsg.ne.jp>:
>
>>> 環境によっては問題が残るという話でしょうか。
>>
>>
>> そういうことになると思います。
>> windows でも誰かが直せるなら直せばいいですし、重大な問題ではない
>> ので best effort な環境では無視してもいいかと思います。
>
> 1.8 はどうします?


ああ、そんなものもありましたねえ。全く見てません。

windows と同様に、それはそれで考えればいいと思います。
1.8 と 1.9 が同じアプローチで解決できるならそれがよさそうですが、
1.8 でダメだからといって 1.9 でも解決しないという理由もないと
思います。

それでもやはり「1.8 で解決できないからダメだ」ということなら、
「これが仕様であり rubyspec に書くべきでない」と言ってください。
お願いします。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

遠藤です。

2010年2月5日0:18 Yusuke ENDOH <mame@tsg.ne.jp>:
> 単一スレッドの状態で TCPServer#gets を呼ぶと Errno::ENOTCONN が
> 投げられますが、複数のスレッドがいる状態だと接続があるまでブロック
> します。
>
>  Thread.new { sleep }
>  TCPServer.new(0).gets
>
> 原因は gets が読み込みを行う前に (rb_thread_wait_fd 経由で) select
> を行うことです。
> io.c の READ_CHECK の中で rb_thread_wait_fd を呼ぶ必要はあるので
> しょうか。「windows で必要」との akr さんのお告げがあったので、
> _WIN32 以外では呼ばないようにするパッチを作ってみました。


直接的な反対はないようなので、一旦コミットします。

といってもこのパッチで正しいかどうかはかなり自信ないので、IO
周りで変とか固まるとかになったら、この件を疑ってください。

akr さんの指摘する問題 (windows と 1.8) は残るので、よりよい
解決策の提案を待っています。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

遠藤です。

2010年2月11日16:46 Tanaka Akira <akr@fsij.org>:
> 2010年2月9日22:23 Yusuke ENDOH <mame@tsg.ne.jp>:
>>
>> といってもこのパッチで正しいかどうかはかなり自信ないので、IO
>> 周りで変とか固まるとかになったら、この件を疑ってください。
>
> test-all で失敗が増えていますね。
>
> % ./ruby -v test/ruby/test_io.rb
> ruby 1.9.2dev (2010-02-11 trunk 26640) [i686-linux]
> Loaded suite test/ruby/test_io
> Started
> ........................................................F.......F....................
> Finished in 1.087307 seconds.
>
>  1) Failure:
> test_read_error(TestIO) [test/ruby/test_io.rb:869]:
> RuntimeError expected but nothing was raised.
>
>  2) Failure:
> test_readpartial_error(TestIO) [test/ruby/test_io.rb:834]:
> RuntimeError expected but nothing was raised.
>
> 85 tests, 336 assertions, 2 failures, 0 errors, 0 skips


ああそうだ、忘れてました。それが「瑣末な問題」といったやつです。
もともとは

  $ ruby-1.9.1-p378 -e '
  r, w = IO.pipe
  s = ""
  t = Thread.new { r.read(5, s) }
  0 until s.size == 5
  s.clear
  w.write "foobarbaz"
  w.close
  t.join
  '
  -e:4:in `read': buffer string modified (RuntimeError)
          from -e:4:in `block in <main>'

のように、

- read にバッファを明示して待ち状態にし、
- バッファを modify した後で
- write したら例外が上がる、

という挙動だったのですが、以下のようにすると、このチェックは
抜けられることがあります (タイミングに依存しますが) 。

  $ ruby-1.9.1-p378 -e '
  r, w = IO.pipe
  s = ""
  w.write "foo"
  t = Thread.new { r.read(5, s) }
  0 until s.size >= 5
  p s
  s.clear
  w.write "barbaz"
  w.close
  t.join
  p s
  '
  "\x00ooba"

つまり、

- read にバッファを明示して待ち状態にし、
- read 分に満たないだけ write して (これでチェックをかわせる)
- バッファを modify した後で
- write しても例外が上がらず変なことになる (ことがある)

というわけで、このチェックには今のところ意味がないと思います。

やるとしたら、io_read の中の READ_CHECK の直後だけでチェックしても
だめで、buffer string に書こうとする直前で毎回チェックする必要が
あると思いますが、IO のパフォーマンスが下がりそうなのでそこまでする
必要があるかどうかはわかりません。


と言うわけで、反対がなければこのチェックとテストは外してしまおうと
思っています。


> まぁ、これは落ちなければいいという類の話で、いまのところ落ちていないので
> いいといえばいいのですが、危険に近づいている気はします。

なんとかできるといいんですけどねえ。

何度も言っていますが、「安全側に倒して TCPServer#gets が固まることが
あるのは仕様とする」と言ってくれるのでも私は構いません。

確かに、rubyspec の人たちも 1.8 とかで rubinius とかで固まってないの
か不思議ですよね。今度聞いときます。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

遠藤です。

連投すみません。

2010年2月11日17:14 Yusuke ENDOH <mame@tsg.ne.jp>:
> - read にバッファを明示して待ち状態にし、
> - read 分に満たないだけ write して (これでチェックをかわせる)
> - バッファを modify した後で
> - write しても例外が上がらず変なことになる (ことがある)
>
> というわけで、このチェックには今のところ意味がないと思います。


このチェック回避のアプローチで、めでたく今回の変更前でも落とせる
ことが確認できました。


  $ cat t.rb
  r, w = IO.pipe
  s = ""
  n = 10000
  w.write("foo")
  t = Thread.new { r.read(n, s) }
  (sleep 1; p [s[0, 5], s.size]) until s.size == n
  s.clear
  w.write("barbaz" * 1000)
  w.close
  t.join

  $ ruby-1.9.1-p378 t.rb
  ["foo\x00\x00", 10000]
  t.rb:9: [BUG] Segmentation fault
  ruby 1.9.1p378 (2010-01-10 revision 26273) [i686-linux]

  -- control frame ----------
  c:0003 p:0182 s:0012 b:0011 l:001784 d:002150 EVAL   t.rb:9
  c:0002 p:---- s:0004 b:0004 l:000003 d:000003 FINISH
  c:0001 p:0000 s:0002 b:0002 l:001784 d:001784 TOP
  ---------------------------
  セグメンテーション違反です


再現性はあまり高くないようです (20 回に 1 回くらい？) 。


> やるとしたら、io_read の中の READ_CHECK の直後だけでチェックしても
> だめで、buffer string に書こうとする直前で毎回チェックする必要が
> あると思いますが、IO のパフォーマンスが下がりそうなのでそこまでする
> 必要があるかどうかはわかりません。


のように、今回の変更とは関係なく、io_fread などの実行中に文字列がいじられて
いないかチェックする必要がありそうです。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

遠藤です。

2010年2月11日17:35 Tanaka Akira <akr@fsij.org>:
> 2010年2月11日17:14 Yusuke ENDOH <mame@tsg.ne.jp>:
>
>> という挙動だったのですが、以下のようにすると、このチェックは
>> 抜けられることがあります (タイミングに依存しますが) 。
>
> べつに落ちなければいいんですが。
> テストがあるのは、以前、落ちたからのような気がします。

いえ、問題のテストは単にカバレッジのために r16683 で私が入れたものです。


>> やるとしたら、io_read の中の READ_CHECK の直後だけでチェックしても
>> だめで、buffer string に書こうとする直前で毎回チェックする必要が
>> あると思いますが、IO のパフォーマンスが下がりそうなのでそこまでする
>> 必要があるかどうかはわかりません。
>
> EFAULT のケースだと、buffer string に書くのは read システムコールなので、
> カーネルが EFAULT で済ましてくれるというのを期待することになりますね。
> システムコールを発行する前にはメモリはちゃんとあって検査のしようがないし。
> それでは済まない OS が無いことを期待する、ということになりますかね。

そうでした。io_fread の中でチェックしてもダメですね。

EFAULT ってどのくらいあてになるんでしょうね。read 発行直後はちゃんと
バッファがあったけど、ブロック中に別スレッドが free/realloc したなんて
のも助けてくれるのかなあ。
実際 [ruby-dev:40391] で落ちているのがそのせいかどうかは調べてません。


> STR_TMPLOCK が使えるかなぁ。

そういえばそんなものがありましたね。それが良さそう。


>> 確かに、rubyspec の人たちも 1.8 とかで rubinius とかで固まってないの
>> か不思議ですよね。今度聞いときます。
>
> 確認してませんが、他にスレッドが無いのでは?
>
> まぁ、1.9 の場合に存在するスレッドは何かというのも確認してませんが。

TCPServer#gets の spec を単体で実行しても固まらなくて、Net::HTTP の
spec とあわせて実行すると固まってました。なので、その辺のスレッドが
たまたま 1.9 では生き残っているんですかね (もちろん Net::HTTP 以外の
スレッドもいるかもしれない) 。

# そもそも生き残るのがおかしい？
# スレッドプールみたいなことするんでしたっけ。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

kosakiです。

>>> やるとしたら、io_read の中の READ_CHECK の直後だけでチェックしても
>>> だめで、buffer string に書こうとする直前で毎回チェックする必要が
>>> あると思いますが、IO のパフォーマンスが下がりそうなのでそこまでする
>>> 必要があるかどうかはわかりません。
>>
>> EFAULT のケースだと、buffer string に書くのは read システムコールなので、
>> カーネルが EFAULT で済ましてくれるというのを期待することになりますね。
>> システムコールを発行する前にはメモリはちゃんとあって検査のしようがないし。
>> それでは済まない OS が無いことを期待する、ということになりますかね。
>
> そうでした。io_fread の中でチェックしてもダメですね。
>
> EFAULT ってどのくらいあてになるんでしょうね。read 発行直後はちゃんと
> バッファがあったけど、ブロック中に別スレッドが free/realloc したなんて
> のも助けてくれるのかなあ。

ちゃんと調べずに書いていますが、カーネル屋さんの基本思想としては、あるプロセスがアホの子の時に
そのプロセスが死ぬのを防ぐ必要はないと考えています。memcpyの引数間違えただけでSIGSEGVする
こんな世の中じゃカーネルだけ頑張ってチェックしてもあんまり価値ないし。
もちろん、ミスるとカーネルとか他プロセスに被害が出そうな場合は厳密にやりますが。

そういうわけで、広い世の中きっとSEGVするOSはあるんじゃないかなぁ

おはようございます。

In message <e0b1e5701002110133r2f7599c8u1c931f70169675c7@mail.gmail.com>
	on Thu, 11 Feb 2010 18:36:14 +0900,
	Yusuke ENDOH <mame@tsg.ne.jp> wrote:
> EFAULT ってどのくらいあてになるんでしょうね。read 発行直後はちゃんと
> バッファがあったけど、ブロック中に別スレッドが free/realloc したなんて
> のも助けてくれるのかなあ。
free(3)/realloc(3)したとしても、カーネルから見てプロセスに割り当てられ
たメモリに変わりがなければ、EFALUTにはならないでしょう。

これを期待する方が、無理な気がします。

-- 
神戸 隆博 (かんべ たかひろ)		at 仕事場 

遠藤です。

2010年2月12日9:39 Takahiro Kambe <taca@back-street.net>:
>> EFAULT ってどのくらいあてになるんでしょうね。read 発行直後はちゃんと
>> バッファがあったけど、ブロック中に別スレッドが free/realloc したなんて
>> のも助けてくれるのかなあ。
> free(3)/realloc(3)したとしても、カーネルから見てプロセスに割り当てられ
> たメモリに変わりがなければ、EFALUTにはならないでしょう。
>
> これを期待する方が、無理な気がします。

まあ、そりゃそうですよね。OS が libc レベルの情報を知る訳ない。


後からログを読むひとのために問題を整理しておくと、


- EFAULT や SEGV が起きる問題は元からあった (1.9.1 でも観測され
  ている)

- SEGV を起きにくくするチェックがあったが、タイミングをずらせば
  簡単に抜けられるので、本質的には解決されていない

- [ruby-dev:40317] のパッチはタイミング的にこのチェックの効果を
  弱めるため、EFAULT が観測しやすくなった

- 元の問題は STR_TMPLOCK を使えばたぶんちゃんと解決できる


というわけで、[ruby-dev:40317] のパッチに問題があるわけではない
と考えています。今のところ。

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

遠藤です。

2010年2月12日12:24 Yusuke ENDOH <mame@tsg.ne.jp>:
> - 元の問題は STR_TMPLOCK を使えばたぶんちゃんと解決できる


やってみました。make check と make test-rubyspec は完走していますが、
どうでしょう。反対がなければコミットします。


diff --git a/io.c b/io.c
index 9983e17..fd7fbbf 100644
--- a/io.c
+++ b/io.c
@@ -1742,7 +1742,9 @@ read_all(rb_io_t *fptr, long siz, VALUE str)
     }
     for (;;) {
 	READ_CHECK(fptr);
+	rb_str_locktmp(str);
 	n = io_fread(str, bytes, fptr);
+	rb_str_unlocktmp(str);
 	if (n == 0 && bytes == 0) {
 	    break;
 	}
@@ -1810,18 +1812,15 @@ io_getpartial(int argc, VALUE *argv, VALUE io,
int nonblock)

     if (!nonblock)
         READ_CHECK(fptr);
-    if (RSTRING_LEN(str) != len) {
-      modified:
-	rb_raise(rb_eRuntimeError, "buffer string modified");
-    }
     n = read_buffered_data(RSTRING_PTR(str), len, fptr);
     if (n <= 0) {
       again:
-	if (RSTRING_LEN(str) != len) goto modified;
         if (nonblock) {
             rb_io_set_nonblock(fptr);
         }
+	rb_str_locktmp(str);
 	n = rb_read_internal(fptr->fd, RSTRING_PTR(str), len);
+	rb_str_unlocktmp(str);
         if (n < 0) {
             if (!nonblock && rb_io_wait_readable(fptr->fd))
                 goto again;
@@ -2136,10 +2135,9 @@ io_read(int argc, VALUE *argv, VALUE io)
     if (len == 0) return str;

     READ_CHECK(fptr);
-    if (RSTRING_LEN(str) != len) {
-	rb_raise(rb_eRuntimeError, "buffer string modified");
-    }
+    rb_str_locktmp(str);
     n = io_fread(str, 0, fptr);
+    rb_str_unlocktmp(str);
     if (n == 0) {
 	if (fptr->fd < 0) return Qnil;
         rb_str_resize(str, 0);
@@ -3841,11 +3839,10 @@ rb_io_sysread(int argc, VALUE *argv, VALUE io)
     n = fptr->fd;
     rb_thread_wait_fd(fptr->fd);
     rb_io_check_closed(fptr);
-    if (RSTRING_LEN(str) != ilen) {
-	rb_raise(rb_eRuntimeError, "buffer string modified");
-    }

+    rb_str_locktmp(str);
     n = rb_read_internal(fptr->fd, RSTRING_PTR(str), ilen);
+    rb_str_unlocktmp(str);

     if (n == -1) {
 	rb_sys_fail_path(fptr->pathv);
diff --git a/test/ruby/test_io.rb b/test/ruby/test_io.rb
index 0052a92..830bef6 100644
--- a/test/ruby/test_io.rb
+++ b/test/ruby/test_io.rb
@@ -823,15 +823,15 @@ class TestIO < Test::Unit::TestCase
     end)
   end

-  def test_readpartial_error
+  def test_readpartial_lock
     with_pipe do |r, w|
       s = ""
       t = Thread.new { r.readpartial(5, s) }
       0 until s.size == 5
-      s.clear
+      assert_raise(RuntimeError) { s.clear }
       w.write "foobarbaz"
       w.close
-      assert_raise(RuntimeError) { t.join }
+      assert_equal("fooba", t.value)
     end
   end

@@ -858,15 +858,15 @@ class TestIO < Test::Unit::TestCase
     end)
   end

-  def test_read_error
+  def test_read_lock
     with_pipe do |r, w|
       s = ""
       t = Thread.new { r.read(5, s) }
       0 until s.size == 5
-      s.clear
+      assert_raise(RuntimeError) { s.clear }
       w.write "foobarbaz"
       w.close
-      assert_raise(RuntimeError) { t.join }
+      assert_equal("fooba", t.value)
     end
   end


-- 
Yusuke ENDOH <mame@tsg.ne.jp>

成瀬です。

(2010/02/17 0:57), Yusuke ENDOH wrote:
> やってみました。make check と make test-rubyspec は完走していますが、
> どうでしょう。反対がなければコミットします。

手元でも完走する事を確認しました。
ありがとうございました。

-- 
NARUSE, Yui  <naruse@airemix.jp>