Project

General

Profile

Bug #2712

TCPServer#gets gets stuck

Added by mame (Yusuke Endoh) almost 10 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
Backport:
[ruby-dev:40317]

Description

=begin
遠藤です。

単一スレッドの状態で TCPServer#gets を呼ぶと Errno::ENOTCONN が
投げられますが、複数のスレッドがいる状態だと接続があるまでブロック
します。

Thread.new { sleep }
TCPServer.new(0).gets

rubyspec がそういうテストを入れてくれたので困ってます。

原因は gets が読み込みを行う前に (rb_thread_wait_fd 経由で) select
を行うことです。
io.c の READ_CHECK の中で rb_thread_wait_fd を呼ぶ必要はあるので
しょうか。「windows で必要」との akr さんのお告げがあったので、
_WIN32 以外では呼ばないようにするパッチを作ってみました。

make check と test-rubyspec では問題が見つかりませんでした。
正確には 2 つほどテストが失敗しますが、瑣末な問題なので、ちょっと
修正すれば直せます。どうでしょう。

diff --git a/io.c b/io.c
index 4017778..8b68603 100644
--- a/io.c
+++ b/io.c
@@ -175,9 +175,15 @@ static int max_file_descriptor = NOFILE;
#define READ_DATA_PENDING_PTR(fptr) ((fptr)->rbuf+(fptr)->rbuf_off)
#define READ_DATA_BUFFERED(fptr) READ_DATA_PENDING(fptr)

+#if defined(_WIN32)
+#define WAIT_FD_IN_WIN32(fptr) rb_thread_wait_fd((fptr)->fd);
+#else
+#define WAIT_FD_IN_WIN32(fptr) ;
+#endif
+
#define READ_CHECK(fptr) do {\
if (!READ_DATA_PENDING(fptr)) {\

  • rb_thread_wait_fd((fptr)->fd);\
  • WAIT_FD_IN_WIN32(fptr);\
    rb_io_check_closed(fptr);\
    }\
    } while(0)
    @@ -1637,8 +1643,7 @@ fill_cbuf(rb_io_t *fptr, int ec_flags)

      if (res == econv_source_buffer_empty) {
          if (fptr->rbuf_len == 0) {
    
  •            rb_thread_wait_fd(fptr->fd);
    
  •            rb_io_check_closed(fptr);
    
  •  READ_CHECK(fptr);
              if (io_fillbuf(fptr) == -1) {
                  ds = dp = (unsigned char *)fptr->cbuf +
    

    fptr->cbuf_off + fptr->cbuf_len;
    de = (unsigned char *)fptr->cbuf + fptr->cbuf_capa;
    @@ -2228,8 +2233,7 @@ appendline(rb_io_t *fptr, int delim, VALUE
    *strp, long *lp)
    if (limit == 0)
    return (unsigned char)RSTRING_PTR(str)[RSTRING_LEN(str)-1];
    }

  • rb_thread_wait_fd(fptr->fd);

  • rb_io_check_closed(fptr);

  • READ_CHECK(fptr);
    } while (io_fillbuf(fptr) >= 0);
    lp = limit;
    return EOF;
    @@ -2251,8 +2255,7 @@ swallow(rb_io_t *fptr, int term)
    if (!read_buffered_data(buf, cnt - i, fptr)) /
    must not fail */
    rb_sys_fail_path(fptr->pathv);
    }

  • rb_thread_wait_fd(fptr->fd);

  • rb_io_check_closed(fptr);

  • READ_CHECK(fptr);
    } while (io_fillbuf(fptr) == 0);
    return FALSE;
    }
    @@ -2290,8 +2293,7 @@ rb_io_getline_fast(rb_io_t *fptr, rb_encoding *enc)
    pos = rb_str_coderange_scan_restartable(RSTRING_PTR(str) + pos,
    RSTRING_PTR(str) + len, enc, &cr);
    if (e) break;
    }

  • rb_thread_wait_fd(fptr->fd);

  • rb_io_check_closed(fptr);

  • READ_CHECK(fptr);
    if (io_fillbuf(fptr) < 0) {
    if (NIL_P(str)) return Qnil;
    break;

--
Yusuke ENDOH mame@tsg.ne.jp
=end


Related issues

Has duplicate Ruby master - Bug #2748: fix for READ_CHECK causes failures on FreeBSD 8.0Closed02/16/2010Actions

History

#1

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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

=end

#2

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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

=end

#3

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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

=end

#4

Updated by mame (Yusuke Endoh) almost 10 years ago

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

=begin
This issue was solved with changeset r26625.
Yusuke, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end

#5

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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 '

のように、

  • 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

=end

#6

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

連投すみません。

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

=end

#7

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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

=end

#8

Updated by kosaki (Motohiro KOSAKI) almost 10 years ago

=begin
kosakiです。

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

EFAULT のケースだと、buffer string に書くのは read システムコールなので、
カーネルが EFAULT で済ましてくれるというのを期待することになりますね。
システムコールを発行する前にはメモリはちゃんとあって検査のしようがないし。
それでは済まない OS が無いことを期待する、ということになりますかね。

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

EFAULT ってどのくらいあてになるんでしょうね。read 発行直後はちゃんと
バッファがあったけど、ブロック中に別スレッドが free/realloc したなんて
のも助けてくれるのかなあ。

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

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

=end

#9

Updated by taca (Takahiro Kambe) almost 10 years ago

=begin
おはようございます。

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 仕事場

=end

#10

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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

=end

#11

Updated by mame (Yusuke Endoh) almost 10 years ago

=begin
遠藤です。

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

=end

#12

Updated by naruse (Yui NARUSE) almost 10 years ago

=begin
成瀬です。

(2010/02/17 0:57), Yusuke ENDOH wrote:

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

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

--
NARUSE, Yui naruse@airemix.jp

=end

Also available in: Atom PDF