Bug #11740: ObjectSpace.each_object exposes internal metaclasses

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: -
• ruby -v: ruby 2.3.0dev (2015-11-19 trunk 52672) [x86_64-linux]
• Backport: 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONTNEED

[Eregon (Benoit Daloze)]

ObjectSpace.each_object exposes internal metaclasses and
this might result in assumptions being violated since the metaclass structure is not well preserved.

See the attached script for an example.
The #bla method should always be defined on the metaclass of "klass".

See https://bugs.ruby-lang.org/issues/11360#note-2 as well in which I warned against this problem ;)

[ko1 (Koichi Sasada)]

• Assignee set to ko1 (Koichi Sasada)

I'll fix it.

[ko1 (Koichi Sasada)]

Index: class.c
===================================================================
--- class.c	(revision 53150)
+++ class.c	(working copy)
@@ -442,6 +442,12 @@ rb_singleton_class_attached(VALUE klass,
  */
 #define META_CLASS_OF_CLASS_CLASS_P(k)  (METACLASS_OF(k) == (k))
 
+int
+rb_singleton_class_has_metaclass_p(VALUE sklass)
+{
+    return rb_ivar_get(METACLASS_OF(sklass), id_attached) == sklass;
+}
+
 /*!
  * whether k has a metaclass
  * @retval 1 if \a k has a metaclass
@@ -449,7 +455,13 @@ rb_singleton_class_attached(VALUE klass,
  */
 #define HAVE_METACLASS_P(k) \
     (FL_TEST(METACLASS_OF(k), FL_SINGLETON) && \
-     rb_ivar_get(METACLASS_OF(k), id_attached) == (k))
+     rb_singleton_class_has_metaclass_p(k))
+
+int
+rb_class_has_metaclass_p(VALUE klass)
+{
+    return HAVE_METACLASS_P(klass);
+}
 
 /*!
  * ensures \a klass belongs to its own eigenclass.
Index: gc.c
===================================================================
--- gc.c	(revision 53150)
+++ gc.c	(working copy)
@@ -2400,6 +2400,13 @@ internal_object_p(VALUE obj)
 	  case T_NODE:
 	  case T_ZOMBIE:
 	    break;
+	  case T_CLASS:
+	    {
+		if (FL_TEST(obj, FL_SINGLETON)) {
+		    int rb_singleton_class_has_metaclass_p(VALUE sklass);
+		    return rb_singleton_class_has_metaclass_p(obj) == 0;
+		}
+	    }
 	  default:
 	    if (!p->as.basic.klass) break;
 	    return 0;

This patch solves this problem.
Could you check it?

[Eregon (Benoit Daloze)]

Thanks for looking at this!

I tried, but I observed a few issues.

First, when running the test above with -v:

ruby 2.3.0dev (2015-12-16 trunk 53152) [x86_64-linux]
#<Class:0x0055cb4c0f3a58>
[#<Class:0x0055cb4c0f3a58>, Array, Enumerable, Object, Kernel, BasicObject]
objspace_expose_intern_meta.rb:10: warning: instance variable __attached__ not initialized
objspace_expose_intern_meta.rb:10: warning: instance variable __attached__ not initialized
nil

I think it would be better to hide this warning from the user.

Second, while the patch now hides the internal class, it seems to also hide sclass exposed to the user.

p c = Class.new
p i = c.new
p s = i.singleton_class
p s.is_a? c.singleton_class
p ObjectSpace.each_object(c.singleton_class).to_a
s.singleton_class
p ObjectSpace.each_object(c.singleton_class).to_a

outputs:

...
[#<Class:0x00558227ca5f90>] # c.singleton_class
[#<Class:#<#<Class:0x00558227ca5f90>:0x00558227ca5748>>, #<Class:0x00558227ca5f90>] # s, c.singleton_class

So s is not shown by each_object, even though the user already has a reference to it (with s, and it has a metaclass I believe).
So it would seem the check is not correct, but I found no obvious fix.
There is a difference between HAVE_METACLASS_P() and the check in gc.c though.

This failure can also be found by running RubySpec core/objectspace/each_object_spec.rb.

PS: How should I apply the patch? I tried patch < x.patch but I got a rejected hunk in gc.c

[nobu (Nobuyoshi Nakada)]

Koichi Sasada wrote:

+int
+rb_singleton_class_has_metaclass_p(VALUE sklass)
+{
+    return rb_ivar_get(METACLASS_OF(sklass), id_attached) == sklass;
+}

Should be rb_attr_get instead.

Benoit Daloze wrote:

PS: How should I apply the patch? I tried patch < x.patch but I got a rejected hunk in gc.c

Redmine seems expand tabs, you can see markdown source from a pencil icon.

[ko1 (Koichi Sasada)]

• Status changed from Open to Closed

Applied in changeset r53228.

---

• gc.c (internal_object_p): should not expose singleton classes
  without a metaclass.
  [Bug #11740]

• class.c (rb_singleton_class_has_metaclass_p): added.

• test/ruby/test_class.rb: add a test.

[ko1 (Koichi Sasada)]

• Status changed from Closed to Assigned

This patch breaks rubyspec.

1)
ObjectSpace.each_object walks a class and its normal descendants when passed the class's singleton class FAILED
Expected [#<Class:0x00000001860d98>,
 #<Class:0x00000001860de8>,
 #<Class:0x00000001860e38>,
 #<Class:0x00000001860e88>]
to equal [#<Class:#<#<Class:0x00000001860de8>:0x00000001860d20>>,
 #<Class:0x00000001860d98>,
 #<Class:0x00000001860de8>,
 #<Class:0x00000001860e38>,
 #<Class:0x00000001860e88>]

/mnt/sdb1/ruby/trunk/spec/rubyspec/core/objectspace/each_object_spec.rb:222:in `block (2 levels) in <top (required)>'
/mnt/sdb1/ruby/trunk/spec/rubyspec/core/objectspace/each_object_spec.rb:4:in `<top (required)>'

2)
ObjectSpace.each_object on singleton classes walks singleton classes FAILED
Expected [#<Class:0x0000000189a958>]

Maybe I missed some points.
Any idea?

[shugo (Shugo Maeda)]

Koichi Sasada wrote:

Maybe I missed some points.
Any idea?

You shouldn't hide singleton classes of non-class objects.

diff --git a/gc.c b/gc.c
index 12be1ea..a574656 100644
--- a/gc.c
+++ b/gc.c
@@ -2400,6 +2400,14 @@ internal_object_p(VALUE obj)
 	  case T_NODE:
 	  case T_ZOMBIE:
 	    break;
+	  case T_CLASS:
+	    {
+		if (FL_TEST(obj, FL_SINGLETON)) {
+		    int rb_singleton_class_has_metaclass_p(VALUE sklass);
+		    return RB_TYPE_P(rb_attr_get(obj, id__attached__), T_CLASS) &&
+			rb_singleton_class_has_metaclass_p(obj) == 0;
+		}
+	    }
 	  default:
 	    if (!p->as.basic.klass) break;
 	    return 0;

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed

Applied in changeset r53243.

---

gc.c: do not expose internal singleton class

• gc.c (internal_object_p): should not expose singleton classes
  without a metaclass. based on patches by ko1 and shugo.
  [Bug #11740]
• class.c (rb_singleton_class_object_p): added.

[Eregon (Benoit Daloze)]

Looks good now, thanks!

[usa (Usaku NAKAMURA)]

• Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONTNEED