ruby-changes:13456
From: nobu <ko1@a...>
Date: Sun, 4 Oct 2009 19:31:46 +0900 (JST)
Subject: [ruby-changes:13456] Ruby:r25230 (ruby_1_8, trunk): * marshal.c (struct {dump,load}_arg): manage with dfree, instead
nobu 2009-10-04 19:30:56 +0900 (Sun, 04 Oct 2009) New Revision: 25230 http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=rev&revision=25230 Log: * marshal.c (struct {dump,load}_arg): manage with dfree, instead of using local variable which may be moved by context switch. [ruby-dev:39425] Modified files: branches/ruby_1_8/ChangeLog branches/ruby_1_8/marshal.c branches/ruby_1_8/test/ruby/test_marshal.rb trunk/ChangeLog trunk/marshal.c trunk/test/ruby/test_marshal.rb Index: ChangeLog =================================================================== --- ChangeLog (revision 25229) +++ ChangeLog (revision 25230) @@ -1,3 +1,9 @@ +Sun Oct 4 19:30:54 2009 Nobuyoshi Nakada <nobu@r...> + + * marshal.c (struct {dump,load}_arg): manage with dfree, instead + of using local variable which may be moved by context switch. + [ruby-dev:39425] + Sun Oct 4 15:00:32 2009 Nobuyoshi Nakada <nobu@r...> * pack.c (NATINT_LEN, pack_pack): suppressed warnings. Index: marshal.c =================================================================== --- marshal.c (revision 25229) +++ marshal.c (revision 25230) @@ -132,14 +132,12 @@ } struct dump_arg { - VALUE obj; VALUE str, dest; st_table *symbols; st_table *data; int taint; int untrust; st_table *compat_tbl; - VALUE wrapper; st_table *encodings; }; @@ -152,12 +150,14 @@ static void check_dump_arg(struct dump_arg *arg, ID sym) { - if (!DATA_PTR(arg->wrapper)) { + if (!arg->symbols) { rb_raise(rb_eRuntimeError, "Marshal.dump reentered at %s", rb_id2name(sym)); } } +static void clear_dump_arg(struct dump_arg *arg); + static void mark_dump_arg(void *ptr) { @@ -168,6 +168,24 @@ rb_mark_hash(p->compat_tbl); } +static void +free_dump_arg(void *ptr) +{ + clear_dump_arg(ptr); + xfree(ptr); +} + +static size_t +memsize_dump_arg(const void *ptr) +{ + return ptr ? sizeof(struct dump_arg) : 0; +} + +static const rb_data_type_t dump_arg_data = { + "dump_arg", + mark_dump_arg, free_dump_arg, memsize_dump_arg +}; + static const char * must_not_be_anonymous(const char *type, VALUE path) { @@ -825,34 +843,24 @@ } } -static VALUE -dump(struct dump_call_arg *arg) +static void +clear_dump_arg(struct dump_arg *arg) { - w_object(arg->obj, arg->arg, arg->limit); - if (arg->arg->dest) { - rb_io_write(arg->arg->dest, arg->arg->str); - rb_str_resize(arg->arg->str, 0); - } - return 0; -} - -static VALUE -dump_ensure(struct dump_arg *arg) -{ - if (!DATA_PTR(arg->wrapper)) return 0; + if (!arg->symbols) return; st_free_table(arg->symbols); + arg->symbols = 0; st_free_table(arg->data); st_free_table(arg->compat_tbl); - if (arg->encodings) st_free_table(arg->encodings); - DATA_PTR(arg->wrapper) = 0; - arg->wrapper = 0; + if (arg->encodings) { + st_free_table(arg->encodings); + arg->encodings = 0; + } if (arg->taint) { OBJ_TAINT(arg->str); } if (arg->untrust) { OBJ_UNTRUST(arg->str); } - return 0; } /* @@ -886,8 +894,8 @@ { VALUE obj, port, a1, a2; int limit = -1; - struct dump_arg arg; - struct dump_call_arg c_arg; + struct dump_arg *arg; + VALUE wrapper; port = Qnil; rb_scan_args(argc, argv, "12", &obj, &a1, &a2); @@ -901,41 +909,42 @@ else if (NIL_P(a1)) goto type_error; else port = a1; } - arg.dest = 0; - arg.symbols = st_init_numtable(); - arg.data = st_init_numtable(); - arg.taint = FALSE; - arg.untrust = FALSE; - arg.compat_tbl = st_init_numtable(); - arg.encodings = 0; - arg.str = rb_str_buf_new(0); - RBASIC(arg.str)->klass = 0; - arg.wrapper = Data_Wrap_Struct(rb_cData, mark_dump_arg, 0, &arg); + wrapper = TypedData_Make_Struct(rb_cData, struct dump_arg, &dump_arg_data, arg); + arg->dest = 0; + arg->symbols = st_init_numtable(); + arg->data = st_init_numtable(); + arg->taint = FALSE; + arg->untrust = FALSE; + arg->compat_tbl = st_init_numtable(); + arg->encodings = 0; + arg->str = rb_str_tmp_new(0); if (!NIL_P(port)) { if (!rb_respond_to(port, s_write)) { type_error: rb_raise(rb_eTypeError, "instance of IO needed"); } - arg.dest = port; + arg->dest = port; if (rb_respond_to(port, s_binmode)) { rb_funcall2(port, s_binmode, 0, 0); - check_dump_arg(&arg, s_binmode); + check_dump_arg(arg, s_binmode); } } else { - port = arg.str; + port = arg->str; } - c_arg.obj = obj; - c_arg.arg = &arg; - c_arg.limit = limit; + w_byte(MARSHAL_MAJOR, arg); + w_byte(MARSHAL_MINOR, arg); - w_byte(MARSHAL_MAJOR, &arg); - w_byte(MARSHAL_MINOR, &arg); + w_object(obj, arg, limit); + if (arg->dest) { + rb_io_write(arg->dest, arg->str); + rb_str_resize(arg->str, 0); + } + RBASIC(arg->str)->klass = rb_cString; + clear_dump_arg(arg); + RB_GC_GUARD(wrapper); - rb_ensure(dump, (VALUE)&c_arg, dump_ensure, (VALUE)&arg); - RBASIC(arg.str)->klass = rb_cString; - return port; } @@ -948,18 +957,19 @@ int taint; int untrust; st_table *compat_tbl; - VALUE wrapper; }; static void check_load_arg(struct load_arg *arg, ID sym) { - if (!DATA_PTR(arg->wrapper)) { + if (!arg->symbols) { rb_raise(rb_eRuntimeError, "Marshal.load reentered at %s", rb_id2name(sym)); } } +static void clear_load_arg(struct load_arg *arg); + static void mark_load_arg(void *ptr) { @@ -970,6 +980,24 @@ rb_mark_hash(p->compat_tbl); } +static void +free_load_arg(void *ptr) +{ + clear_load_arg(ptr); + xfree(ptr); +} + +static size_t +memsize_load_arg(const void *ptr) +{ + return ptr ? sizeof(struct load_arg) : 0; +} + +static const rb_data_type_t load_arg_data = { + "load_arg", + mark_load_arg, free_load_arg, memsize_load_arg +}; + static VALUE r_entry(VALUE v, struct load_arg *arg); static VALUE r_object(struct load_arg *arg); static ID r_symbol(struct load_arg *arg); @@ -1679,22 +1707,14 @@ return r_object0(arg, 0, Qnil); } -static VALUE -load(struct load_arg *arg) +static void +clear_load_arg(struct load_arg *arg) { - return r_object(arg); -} - -static VALUE -load_ensure(struct load_arg *arg) -{ - if (!DATA_PTR(arg->wrapper)) return 0; + if (!arg->symbols) return; st_free_table(arg->symbols); + arg->symbols = 0; st_free_table(arg->data); st_free_table(arg->compat_tbl); - DATA_PTR(arg->wrapper) = 0; - arg->wrapper = 0; - return 0; } /* @@ -1712,37 +1732,39 @@ marshal_load(int argc, VALUE *argv) { VALUE port, proc; - int major, minor; - VALUE v; - struct load_arg arg; + int major, minor, taint = FALSE; + VALUE v, wrapper; + struct load_arg *arg; rb_scan_args(argc, argv, "11", &port, &proc); v = rb_check_string_type(port); if (!NIL_P(v)) { - arg.taint = OBJ_TAINTED(port); /* original taintedness */ + taint = OBJ_TAINTED(port); /* 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); } - arg.taint = TRUE; + taint = TRUE; } else { rb_raise(rb_eTypeError, "instance of IO needed"); } - arg.untrust = OBJ_UNTRUSTED(port); - arg.src = port; - arg.offset = 0; - arg.symbols = st_init_numtable(); - arg.data = st_init_numtable(); - arg.compat_tbl = st_init_numtable(); - arg.proc = 0; - arg.wrapper = Data_Wrap_Struct(rb_cData, mark_load_arg, 0, &arg); + wrapper = TypedData_Make_Struct(rb_cData, struct load_arg, &load_arg_data, arg); + arg->taint = taint; + arg->untrust = OBJ_UNTRUSTED(port); + arg->src = port; + arg->offset = 0; + arg->symbols = st_init_numtable(); + arg->data = st_init_numtable(); + arg->compat_tbl = st_init_numtable(); + arg->proc = 0; - major = r_byte(&arg); - minor = r_byte(&arg); + major = r_byte(arg); + minor = r_byte(arg); if (major != MARSHAL_MAJOR || minor > MARSHAL_MINOR) { + clear_load_arg(arg); rb_raise(rb_eTypeError, "incompatible marshal file format (can't be read)\n\ \tformat version %d.%d required; %d.%d given", MARSHAL_MAJOR, MARSHAL_MINOR, major, minor); @@ -1753,8 +1775,10 @@ MARSHAL_MAJOR, MARSHAL_MINOR, major, minor); } - if (!NIL_P(proc)) arg.proc = proc; - v = rb_ensure(load, (VALUE)&arg, load_ensure, (VALUE)&arg); + if (!NIL_P(proc)) arg->proc = proc; + v = r_object(arg); + clear_load_arg(arg); + RB_GC_GUARD(wrapper); return v; } Index: test/ruby/test_marshal.rb =================================================================== --- test/ruby/test_marshal.rb (revision 25229) +++ test/ruby/test_marshal.rb (revision 25230) @@ -212,4 +212,45 @@ c = [/#{a}/, /#{b}/] assert_equal(c, Marshal.load(Marshal.dump(c))) end + + class DumpTest + def marshal_dump + @@block.call(:marshal_dump) + end + + def dump_each(&block) + @@block = block + Marshal.dump(self) + end + end + + class LoadTest + def marshal_dump + nil + end + def marshal_load(obj) + @@block.call(:marshal_load) + end + def self.load_each(m, &block) + @@block = block + Marshal.load(m) + end + end + + def test_context_switch + o = DumpTest.new + e = o.enum_for(:dump_each) + assert_equal(:marshal_dump, e.next) + GC.start + assert(true, '[ruby-dev:39425]') + assert_raise(StopIteration) {e.next} + + o = LoadTest.new + m = Marshal.dump(o) + e = LoadTest.enum_for(:load_each, m) + assert_equal(:marshal_load, e.next) + GC.start + assert(true, '[ruby-dev:39425]') + assert_raise(StopIteration) {e.next} + end end Index: ruby_1_8/ChangeLog =================================================================== --- ruby_1_8/ChangeLog (revision 25229) +++ ruby_1_8/ChangeLog (revision 25230) @@ -1,3 +1,9 @@ +Sun Oct 4 19:30:54 2009 Nobuyoshi Nakada <nobu@r...> + + * marshal.c (struct {dump,load}_arg): manage with dfree, instead + of using local variable which may be moved by context switch. + [ruby-dev:39425] + Sun Oct 4 05:34:34 2009 Hidetoshi NAGAI <nagai@a...> * ext/tk/lib/tk/variable.rb: add TkVariable#to_hash,to_proc,to_int, Index: ruby_1_8/marshal.c =================================================================== --- ruby_1_8/marshal.c (revision 25229) +++ ruby_1_8/marshal.c (revision 25230) @@ -85,12 +85,10 @@ static ID s_getc, s_read, s_write, s_binmode; struct dump_arg { - VALUE obj; VALUE str, dest; st_table *symbols; st_table *data; int taint; - VALUE wrapper; }; struct dump_call_arg { @@ -104,22 +102,32 @@ struct dump_arg *arg; ID sym; { - if (!DATA_PTR(arg->wrapper)) { + if (!arg->symbols) { rb_raise(rb_eRuntimeError, "Marshal.dump reentered at %s", rb_id2name(sym)); } } +static void clear_dump_arg _((struct dump_arg *arg)); + static void mark_dump_arg(ptr) void *ptr; { struct dump_arg *p = ptr; - if (!ptr) + if (!p->symbols) return; rb_mark_set(p->data); } +static void +free_dump_arg(ptr) + void *ptr; +{ + clear_dump_arg(ptr); + xfree(ptr); +} + static VALUE class2path(klass) VALUE klass; @@ -700,32 +708,17 @@ } } -static VALUE -dump(arg) - struct dump_call_arg *arg; -{ - w_object(arg->obj, arg->arg, arg->limit); - if (arg->arg->dest) { - rb_io_write(arg->arg->dest, arg->arg->str); - rb_str_resize(arg->arg->str, 0); - } - return 0; -} - -static VALUE -dump_ensure(arg) +static void +clear_dump_arg(arg) struct dump_arg *arg; { - if (!DATA_PTR(arg->wrapper)) return 0; + if (!arg->symbols) return; st_free_table(arg->symbols); + arg->symbols = 0; st_free_table(arg->data); - DATA_PTR(arg->wrapper) = 0; - arg->wrapper = 0; if (arg->taint) { OBJ_TAINT(arg->str); } - - return 0; } /* @@ -761,8 +754,8 @@ { VALUE obj, port, a1, a2; int limit = -1; - struct dump_arg arg; - struct dump_call_arg c_arg; + struct dump_arg *arg; + VALUE wrapper; port = Qnil; rb_scan_args(argc, argv, "12", &obj, &a1, &a2); @@ -776,37 +769,40 @@ else if (NIL_P(a1)) goto type_error; else port = a1; } - arg.dest = 0; - arg.symbols = st_init_numtable(); - arg.data = st_init_numtable(); - arg.taint = Qfalse; - arg.str = rb_str_buf_new(0); - RBASIC(arg.str)->klass = 0; - arg.wrapper = Data_Wrap_Struct(rb_cData, mark_dump_arg, 0, &arg); + wrapper = Data_Make_Struct(rb_cData, struct dump_arg, mark_dump_arg, free_dump_arg, arg); + arg->dest = 0; + arg->symbols = st_init_numtable(); + arg->data = st_init_numtable(); + arg->taint = Qfalse; + arg->str = rb_str_buf_new(0); + RBASIC(arg->str)->klass = 0; if (!NIL_P(port)) { if (!rb_respond_to(port, s_write)) { type_error: rb_raise(rb_eTypeError, "instance of IO needed"); } - arg.dest = port; + arg->dest = port; if (rb_respond_to(port, s_binmode)) { rb_funcall2(port, s_binmode, 0, 0); - check_dump_arg(&arg, s_binmode); + check_dump_arg(arg, s_binmode); } } else { - port = arg.str; + port = arg->str; } - c_arg.obj = obj; - c_arg.arg = &arg; - c_arg.limit = limit; + w_byte(MARSHAL_MAJOR, arg); + w_byte(MARSHAL_MINOR, arg); - w_byte(MARSHAL_MAJOR, &arg); - w_byte(MARSHAL_MINOR, &arg); + w_object(obj, arg, limit); + if (arg->dest) { + rb_io_write(arg->dest, arg->str); + rb_str_resize(arg->str, 0); + } - rb_ensure(dump, (VALUE)&c_arg, dump_ensure, (VALUE)&arg); - RBASIC(arg.str)->klass = rb_cString; + RBASIC(arg->str)->klass = rb_cString; + clear_dump_arg(arg); + RB_GC_GUARD(wrapper); return port; } @@ -818,7 +814,6 @@ st_table *data; VALUE proc; int taint; - VALUE wrapper; }; static void @@ -826,22 +821,31 @@ struct load_arg *arg; ID sym; { - if (!DATA_PTR(arg->wrapper)) { + if (!arg->symbols) { rb_raise(rb_eRuntimeError, "Marshal.load reentered at %s", rb_id2name(sym)); } } +static void clear_load_arg _((struct load_arg *arg)); + static void mark_load_arg(ptr) void *ptr; { struct load_arg *p = ptr; - if (!ptr) + if (!p->symbols) return; rb_mark_tbl(p->data); } +static void +free_load_arg(void *ptr) +{ + clear_load_arg(ptr); + xfree(ptr); +} + static VALUE r_object _((struct load_arg *arg)); static int @@ -1416,23 +1420,14 @@ return r_object0(arg, arg->proc, 0, Qnil); } -static VALUE -load(arg) +static void +clear_load_arg(arg) struct load_arg *arg; { - return r_object(arg); -} - -static VALUE -load_ensure(arg) - struct load_arg *arg; -{ - if (!DATA_PTR(arg->wrapper)) return 0; + if (!arg->symbols) return; st_free_table(arg->symbols); + arg->symbols = 0; st_free_table(arg->data); - DATA_PTR(arg->wrapper) = 0; - arg->wrapper = 0; - return 0; } /* @@ -1452,35 +1447,36 @@ VALUE *argv; { VALUE port, proc; - int major, minor; - VALUE v; - struct load_arg arg; + int major, minor, taint = Qfalse; + VALUE v, wrapper; + struct load_arg *arg; rb_scan_args(argc, argv, "11", &port, &proc); v = rb_check_string_type(port); if (!NIL_P(v)) { - arg.taint = OBJ_TAINTED(port); /* original taintedness */ + taint = OBJ_TAINTED(port); /* original taintedness */ port = v; } else if (rb_respond_to(port, s_getc) && rb_respond_to(port, s_read)) { if (rb_respond_to(port, s_binmode)) { rb_funcall2(port, s_binmode, 0, 0); } - arg.taint = Qtrue; + taint = Qtrue; } else { rb_raise(rb_eTypeError, "instance of IO needed"); } - arg.src = port; - arg.offset = 0; - arg.symbols = st_init_numtable(); - arg.data = st_init_numtable(); - arg.proc = 0; - arg.wrapper = Data_Wrap_Struct(rb_cData, mark_load_arg, 0, &arg); + wrapper = Data_Make_Struct(rb_cData, struct load_arg, mark_load_arg, free_load_arg, arg); + arg->src = port; + arg->offset = 0; + arg->symbols = st_init_numtable(); + arg->data = st_init_numtable(); + arg->proc = 0; - major = r_byte(&arg); - minor = r_byte(&arg); + major = r_byte(arg); + minor = r_byte(arg); if (major != MARSHAL_MAJOR || minor > MARSHAL_MINOR) { + clear_load_arg(arg); rb_raise(rb_eTypeError, "incompatible marshal file format (can't be read)\n\ \tformat version %d.%d required; %d.%d given", MARSHAL_MAJOR, MARSHAL_MINOR, major, minor); @@ -1491,8 +1487,10 @@ MARSHAL_MAJOR, MARSHAL_MINOR, major, minor); } - if (!NIL_P(proc)) arg.proc = proc; - v = rb_ensure(load, (VALUE)&arg, load_ensure, (VALUE)&arg); + if (!NIL_P(proc)) arg->proc = proc; + v = r_object(arg); + clear_load_arg(arg); + RB_GC_GUARD(wrapper); return v; } Index: ruby_1_8/test/ruby/test_marshal.rb =================================================================== --- ruby_1_8/test/ruby/test_marshal.rb (revision 25229) +++ ruby_1_8/test/ruby/test_marshal.rb (revision 25230) @@ -71,4 +71,32 @@ } assert_equal("marshal data too short", e.message) end + + class DumpTest + def marshal_dump + loop { Thread.pass } + end + end + + class LoadTest + def marshal_dump + nil + end + def marshal_load(obj) + loop { Thread.pass } + end + end + + def test_context_switch + o = DumpTest.new + Thread.new { Marshal.dump(o) } + GC.start + assert(true, '[ruby-dev:39425]') + + o = LoadTest.new + m = Marshal.dump(o) + Thread.new { Marshal.load(m) } + GC.start + assert(true, '[ruby-dev:39425]') + end end -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/