Bug #3463: 1.9.2-preview3 で [BUG] gc_sweep(): unknown data type 0x0

• Status: Closed
• Assignee: nobu (Nobuyoshi Nakada)
• Target version: -
• ruby -v: ruby 1.9.2dev (2010-05-31 revision 28117) [i686-linux]

[nagachika (Tomoyuki Chikanaga)]

=begin
1.9.2-preview3 である大きなプログラムを実行中に以下のようなエラーが発生しました。

[BUG] gc_sweep(): unknown data type 0x0(0xab7cf14)
ruby 1.9.2dev (2010-05-31 revision 28117) [i686-linux]

発生条件はかなり複合的らしく再現する小さなスクリプトを作成できていませんが、
gdb で実行してみると gc_sweep で上記のチェックにひっかかっているオブジェクトは flags が FL_TAINT だけという状態だったため、
OBJ_TAINT している場所をさらってみたところ marshal.c の clear_dump_arg が怪しいようでした。

dump_arg(T_DATA 型のオブジェクト)は GC で deffered されるため dump_arg::str が先に sweep されてしまう場合があり、
clear_dump_arg() で解放後の VALUE に対して OBJ_TAINT (や、おそらく OBJ_UNTRUST も)してしまうと発生するようでした。

とりあえず添付のパッチを当ててみると落ちなくなりました。また make test, test-all でエラーが増えていません。
=end

[nagachika (Tomoyuki Chikanaga)]

• File marshal.patch marshal.patch added

=begin
すみません、先程添付したパッチは問題がありました。
struct dump_arg::str が指すスロットが一旦回収されて他のオブジェクトに割り当てられていた時に間違って taint してしまいそうです。
T_DATA 型のオブジェクトに登録する free 関数の中では構造体が VALUE 型で持っているRubyの参照は一切さわってはいけないですね。

struct dump_arg の str は w_nbytes で書き込む時に OBJ_TAINT/OBJ_UNTRUST すれば良いのではないかと考えパッチを書き直しました。
make test-all でエラーが増えないことと、一応以下のような簡単な確認をしました。

obj = Object.new
=> #Object:0x8d3680c
Marshal.dump(obj).tainted?
=> false
obj.taint
=> #Object:0x8d3680c
Marshal.dump(obj).tainted?
=> true

=end

[mame (Yusuke Endoh)]

• Assignee set to nobu (Nobuyoshi Nakada)

=begin
遠藤です。

redmine がなかださんのメールを拾い落としているので、記録のために
パッチのあるとこだけ貼っておきます。

で、再現コードがないので確認はできないのですが、#2386 もこれでは
ないかと思いました。
marshal_dump + GC っぽいところが似ているような。

近永さん GJ です。

---

なかだです。

At Wed, 23 Jun 2010 09:27:11 +0900,
Tomoyuki Chikanaga wrote in [ruby-dev:41677]:

finaliser_at_exitの時点では、新しいオブジェクトが割り当てられる
ことは考えなくてもいいんじゃないかと思いますが。
clear_dump_arg に arg->str の flags が 0 で来てた時は、下のような
コールスタックでした。普通にメソッド呼び出しの間に遅延された
free 関数の呼び出しが挟み込まれているようです。

うっかりしてました。

ところで、単に興味から訊いてみたいのですが、なぜこのように
T_DATA型オブジェクトの解放は遅延されてるんでしょう。
GC での停止時間を分散させるためでしょうか？

dfree関数の呼び出し中にオブジェクトの割り当てをする拡張ライブラ
リがあったので、mark phaseと分離したんでした。

ついでに、TAINTEDとUNTRUSTEDをひとまとめに扱うようにしてみました。
これでどうでしょう。

diff --git a/marshal.c b/marshal.c
index fa91afa..3263a57 100644
--- a/marshal.c
+++ b/marshal.c
@@ -131,14 +131,16 @@ rb_marshal_define_compat(VALUE newclass, VALUE oldclass, VALUE (*dumper)(VALUE),
st_insert(compat_allocator_tbl, (st_data_t)allocator, (st_data_t)compat);
}

+#define MARSHAL_INFECTION (FL_TAINT|FL_UNTRUSTED)
+typedef char ruby_check_marshal_viral_flags[MARSHAL_INFECTION == (int)MARSHAL_INFECTION ? 1 : -1];
+
struct dump_arg {
VALUE str, dest;
st_table *symbols;
st_table *data;

• int taint;
• int untrust;
  st_table *compat_tbl;
  st_table *encodings;

• int infection;
  };

struct dump_call_arg {
@@ -224,9 +226,8 @@ w_nbyte(const char *s, long n, struct dump_arg *arg)
{
VALUE buf = arg->str;
rb_str_buf_cat(buf, s, n);

• RBASIC(buf)->flags |= arg->infection;
  if (arg->dest && RSTRING_LEN(buf) >= BUFSIZ) {

• if (arg->taint) OBJ_TAINT(buf);
• if (arg->untrust) OBJ_UNTRUST(buf);
  rb_io_write(arg->dest, buf);
  rb_str_resize(buf, 0);
  }
  @@ -636,8 +637,7 @@ w_object(VALUE obj, struct dump_arg *arg, int limit)
  w_symbol(SYM2ID(obj), arg);
  }
  else {
• if (OBJ_TAINTED(obj)) arg->taint = TRUE;
• if (OBJ_UNTRUSTED(obj)) arg->untrust = TRUE;

• arg->infection |= FL_TEST(obj, MARSHAL_INFECTION);

  if (rb_respond_to(obj, s_mdump)) {
  volatile VALUE v;
  @@ -856,12 +856,6 @@ clear_dump_arg(struct dump_arg *arg)
  st_free_table(arg->encodings);
  arg->encodings = 0;
  }

• if (arg->taint) {
• OBJ_TAINT(arg->str);
• }
• if (arg->untrust) {
• OBJ_UNTRUST(arg->str);
• }
  }

/*
@@ -922,8 +916,7 @@ marshal_dump(int argc, VALUE *argv)
arg->dest = 0;
arg->symbols = st_init_numtable();
arg->data = st_init_numtable();

• arg->taint = FALSE;
• arg->untrust = FALSE;

• arg->infection = 0;
  arg->compat_tbl = st_init_numtable();
  arg->encodings = 0;
  arg->str = rb_str_buf_new(0);
  @@ -962,9 +955,8 @@ struct load_arg {
  st_table *symbols;
  st_table *data;
  VALUE proc;

• int taint;
• int untrust;
  st_table *compat_tbl;

• int infection;
  };

static void
@@ -1118,8 +1110,7 @@ r_bytes0(long len, struct load_arg *arg)
if (NIL_P(str)) goto too_short;
StringValue(str);
if (RSTRING_LEN(str) != len) goto too_short;

• if (OBJ_TAINTED(str)) arg->taint = TRUE;
• if (OBJ_UNTRUSTED(str)) arg->untrust = TRUE;

• arg->infection |= FL_TEST(str, MARSHAL_INFECTION);
  }
  return str;
  }
  @@ -1220,15 +1211,10 @@ r_entry0(VALUE v, st_index_t num, struct load_arg *arg)
  else {
  st_insert(arg->data, num, (st_data_t)v);
  }

• if (arg->taint) {

•    OBJ_TAINT(v);
  

•    if ((VALUE)real_obj != Qundef)
  

•        OBJ_TAINT((VALUE)real_obj);
  

• }
• if (arg->untrust) {

•    OBJ_UNTRUST(v);
  

•    if ((VALUE)real_obj != Qundef)
  

•        OBJ_UNTRUST((VALUE)real_obj);
  

• if (arg->infection) {
• FL_SET(v, arg->infection);
• if ((VALUE)real_obj != Qundef)

•  FL_SET((VALUE)real_obj, arg->infection);
  

  }
  return v;
  }
  @@ -1765,7 +1751,7 @@ static VALUE
  marshal_load(int argc, VALUE *argv)
  {
  VALUE port, proc;

• int major, minor, taint = FALSE;

• int major, minor, infection = 0;
  VALUE v;
  volatile VALUE wrapper;
  struct load_arg *arg;
  @@ -1773,21 +1759,20 @@ marshal_load(int argc, VALUE *argv)
  rb_scan_args(argc, argv, "11", &port, &proc);
  v = rb_check_string_type(port);
  if (!NIL_P(v)) {

• taint = OBJ_TAINTED(port); /* original taintedness */

• infection = FL_TEST(port, MARSHAL_INFECTION); /* original taintedness */
  port = v;
  }
  else if (rb_respond_to(port, s_getbyte) && rb_respond_to(port, s_read)) {
  if (rb_respond_to(port, s_binmode)) {
  rb_funcall2(port, s_binmode, 0, 0);
  }

• taint = TRUE;

• infection = FL_TAINT | FL_TEST(port, FL_UNTRUSTED);;
  }
  else {
  rb_raise(rb_eTypeError, "instance of IO needed");
  }
  wrapper = TypedData_Make_Struct(rb_cData, struct load_arg, &load_arg_data, arg);

• arg->taint = taint;
• arg->untrust = OBJ_UNTRUSTED(port);

• arg->infection = infection;
  arg->src = port;
  arg->offset = 0;
  arg->symbols = st_init_numtable();

--
--- 僕の前にBugはない。
--- 僕の後ろにBugはできる。
中田 伸悦
=end

[nobu (Nobuyoshi Nakada)]

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

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

=end