Bug #11740
closedObjectSpace.each_object exposes internal metaclasses
Description
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 ;)
Files
Updated by ko1 (Koichi Sasada) over 8 years ago
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?
Updated by Eregon (Benoit Daloze) over 8 years ago
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
Updated by nobu (Nobuyoshi Nakada) over 8 years ago
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.
Updated by ko1 (Koichi Sasada) over 8 years ago
- 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.
Updated by ko1 (Koichi Sasada) over 8 years ago
- 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?
Updated by shugo (Shugo Maeda) over 8 years ago
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;
Updated by nobu (Nobuyoshi Nakada) over 8 years ago
- 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.
Updated by Eregon (Benoit Daloze) over 8 years ago
Looks good now, thanks!
Updated by usa (Usaku NAKAMURA) about 8 years ago
- Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONTNEED