Forum: Ruby-core Fwd: normal:r47322 (trunk): symbol.c (rb_sym2id): do not return garbage object

308cbef6e86dfc49cce3b2d4cf42aedc?d=identicon&s=25 SASADA Koichi (Guest)
on 2014-08-30 13:51
(Received via mailing list)
Why this fix solve your problem?
dsymbol_pindown(sym) returns symbol itself (with cast to ID).


-------- Original Message --------
Subject: [ruby-changes:35240] normal:r47322 (trunk): symbol.c
(rb_sym2id): do not return garbage object
Date: Sat, 30 Aug 2014 19:30:07 +0900 (JST)
From: normal <ko1@atdot.net>
Reply-To: ruby-changes@quickml.atdot.net
To: ruby-changes@quickml.atdot.net

normal  2014-08-30 19:30:00 +0900 (Sat, 30 Aug 2014)

  New Revision: 47322

  http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=r...

  Log:
    symbol.c (rb_sym2id): do not return garbage object

    The dynamic sym passed to rb_sym2id may be a garbage object
    (as accounted for by dsymbol_check).  This fixes an occasional
    segfault in "make test-all" for me.

    No need to backport, this is from the new symbol GC feature.

  Modified files:
    trunk/ChangeLog
    trunk/symbol.c
Index: symbol.c
===================================================================
--- symbol.c  (revision 47321)
+++ symbol.c  (revision 47322)
@@ -759,7 +759,7 @@ rb_sym2id(VALUE sym)
https://github.com/ruby/ruby/blob/trunk/symbol.c#L759
     }
     else {
   if (!SYMBOL_PINNED_P(sym)) {
-      dsymbol_pindown(sym);
+      return dsymbol_pindown(sym);
   }
   return (ID)sym;
     }
Index: ChangeLog
===================================================================
--- ChangeLog  (revision 47321)
+++ ChangeLog  (revision 47322)
@@ -1,3 +1,7 @@ https://github.com/ruby/ruby/blob/trunk/ChangeLog#L1
+Sat Aug 30 19:22:47 2014  Eric Wong  <e@80x24.org>
+
+  * symbol.c (rb_sym2id): do not return garbage object
+
 Sat Aug 30 06:39:48 2014  Aaron Patterson <aaron@tenderlovemaking.com>

   * ext/psych/lib/psych/visitors/yaml_tree.rb: fix NameError dumping
and
308cbef6e86dfc49cce3b2d4cf42aedc?d=identicon&s=25 SASADA Koichi (Guest)
on 2014-08-30 14:28
(Received via mailing list)
(2014/08/30 8:50), SASADA Koichi wrote:
> Why this fix solve your problem?
> dsymbol_pindown(sym) returns symbol itself (with cast to ID).

OMG, it is my fault.

There is a path to return another symbol value.
18813f71506ebad74179bf8c5a136696?d=identicon&s=25 Eric Wong (Guest)
on 2014-08-30 20:46
(Received via mailing list)
SASADA Koichi <ko1@atdot.net> wrote:
> (2014/08/30 8:50), SASADA Koichi wrote:
> > Why this fix solve your problem?
> > dsymbol_pindown(sym) returns symbol itself (with cast to ID).
>
> OMG, it is my fault.
>
> There is a path to return another symbol value.
>
> >  sym = dsymbol_check(sym);

Right :)  However, my fix appears incomplete.  Even with my patch
applied, I still get into a problem where fstr is 0 inside
dsymbol_check.  So there seems to be another bug...

~~~
--- a/symbol.c
+++ b/symbol.c
@@ -440,12 +440,15 @@ static inline VALUE
 dsymbol_check(const VALUE sym)
 {
     if (UNLIKELY(rb_objspace_garbage_object_p(sym))) {
   const VALUE fstr = RSYMBOL(sym)->fstr;
   RSYMBOL(sym)->fstr = 0;

+  if (fstr == 0)
+      rb_bug("fstr is zero");
+
   unregister_sym(fstr, sym);
   return dsymbol_alloc(rb_cSymbol, fstr, rb_enc_get(fstr));
     }
     else {
   return sym;
     }
~~~
18813f71506ebad74179bf8c5a136696?d=identicon&s=25 Eric Wong (Guest)
on 2014-08-31 05:19
(Received via mailing list)
Eric Wong <normalperson@yhbt.net> wrote:
> So there seems to be another bug...

Oops, forgot, I got backtraces on a clean build tree:

  http://80x24.org/r35240/rb-dump.txt
  http://80x24.org/r35240/gdb-bt.txt

I ran "make check -j8 TESTS=-j8" in loop for a while.
308cbef6e86dfc49cce3b2d4cf42aedc?d=identicon&s=25 SASADA Koichi (Guest)
on 2014-08-31 15:34
(Received via mailing list)
(2014/08/31 0:18), Eric Wong wrote:
> Oops, forgot, I got backtraces on a clean build tree:
>
>   http://80x24.org/r35240/rb-dump.txt
>   http://80x24.org/r35240/gdb-bt.txt
>
> I ran "make check -j8 TESTS=-j8" in loop for a while.

It is mysterious. All dynamic symbol should have fstr...
18813f71506ebad74179bf8c5a136696?d=identicon&s=25 Eric Wong (Guest)
on 2014-08-31 20:54
(Received via mailing list)
SASADA Koichi <ko1@atdot.net> wrote:
> (2014/08/31 0:18), Eric Wong wrote:
> > Oops, forgot, I got backtraces on a clean build tree:
> >
> >   http://80x24.org/r35240/rb-dump.txt
> >   http://80x24.org/r35240/gdb-bt.txt
> >
> > I ran "make check -j8 TESTS=-j8" in loop for a while.
>
> It is mysterious. All dynamic symbol should have fstr...

I'm testing the following, will report back in several hours.

--- a/symbol.c
+++ b/symbol.c
@@ -423,6 +423,7 @@ dsymbol_alloc(const VALUE klass, const VALUE str,
rb_encoding * const enc)

     rb_enc_associate(dsym, enc);
     OBJ_FREEZE(dsym);
+    if (!str) rb_bug("dsymbol_alloc: no str");
     RB_OBJ_WRITE(dsym, &RSYMBOL(dsym)->fstr, str);
     RSYMBOL(dsym)->type = type;

@@ -866,9 +867,7 @@ symbols_i(st_data_t key, st_data_t value, st_data_t
arg)
     VALUE sym = ID2SYM((ID)value);

     if (DYNAMIC_SYM_P(sym) && !SYMBOL_PINNED_P(sym) &&
rb_objspace_garbage_object_p(sym)) {
-  RSYMBOL(sym)->fstr = 0;
-  unregister_sym_id(sym);
-  return ST_DELETE;
+  return ST_CONTINUE;
     }
     else {
   rb_ary_push(ary, sym);
18813f71506ebad74179bf8c5a136696?d=identicon&s=25 Eric Wong (Guest)
on 2014-09-01 02:08
(Received via mailing list)
Eric Wong <normalperson@yhbt.net> wrote:
> I'm testing the following, will report back in several hours.

No change, same BT as before.
18813f71506ebad74179bf8c5a136696?d=identicon&s=25 Eric Wong (Guest)
on 2014-09-06 02:15
(Received via mailing list)
SASADA Koichi <ko1@atdot.net> wrote:
> (2014/08/31 0:18), Eric Wong wrote:
> > Oops, forgot, I got backtraces on a clean build tree:
> >
> >   http://80x24.org/r35240/rb-dump.txt
> >   http://80x24.org/r35240/gdb-bt.txt
> >
> > I ran "make check -j8 TESTS=-j8" in loop for a while.
>
> It is mysterious. All dynamic symbol should have fstr...

It looks like SYM2ID/rb_sym2id interacts badly with dsymbol_check
when it encounters garbage objects.

dsymbol_check replaces an invalid object and returns a new object
for the caller, but the original arg for SYM2ID remains usable
to the caller:

  id = SYM2ID(garbage_sym);
  do_something(garbage_sym); /* bad invalid object used */

Changing: rb_sym2id(VALUE *) to rb_sym2id(VALUE *)

might solve the issue, but introduces many incompatibilities in existing
code:

  id = rb_sym2id(&garbage_sym);
  do_something(garbage_sym); /* id == garbage_sym, safe to use */

Maybe we should remark garbage objects instead of allocating
replacements.
18813f71506ebad74179bf8c5a136696?d=identicon&s=25 Eric Wong (Guest)
on 2014-09-06 02:16
(Received via mailing list)
Eric Wong <normalperson@yhbt.net> wrote:
> Changing: rb_sym2id(VALUE *) to rb_sym2id(VALUE *)

Oops: changing rb_sym2id(VALUE) to rb_sym2id(VALUE *)
This topic is locked and can not be replied to.