Project

General

Profile

Actions

Bug #18189

closed

`rb_cString` can be NULL during `Init_Object`

Added by ioquatix (Samuel Williams) about 1 month ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:105391]

Description

It's possible for rb_cString to be NULL during Init_Object and thus Init_class_hierarchy which means that rb_fstring_lit, which invokes setup_fake_str, invokes RBASIC_SET_CLASS_RAW(..., NULL) (or possibly just something totally random if it's not zero initialized!).

Later on in register_fstring we have an assertion which also fails to detect the abnormality:

assert(RBASIC_CLASS(args.fstr) == rb_cString);

Because both are NULL. Oops.

It seems that later on, rb_cString is set on that specific fstring. But in my own usage of rb_define_module_under during InitVM_Object, this creates invalid class names which fail when passed into Ruby land.

Actions #1

Updated by ioquatix (Samuel Williams) about 1 month ago

  • Description updated (diff)

Updated by ioquatix (Samuel Williams) about 1 month ago

The order of operations between here, for anything involving strings, could be a problem:

    CALL(Object);
    CALL(top_self);
    CALL(Encoding);
    CALL(Comparable);
    CALL(Enumerable);
    CALL(String);

Updated by ioquatix (Samuel Williams) about 1 month ago

I see, there is some kind of patch up:

    rb_cString  = rb_define_class("String", rb_cObject);
    assert(rb_vm_fstring_table());
    st_foreach(rb_vm_fstring_table(), fstring_set_class_i, rb_cString);

So for some things it's working but for rb_define_module_under it seems not working.

Updated by nobu (Nobuyoshi Nakada) about 1 month ago

With adding the following lines after Init_class_hierarchy() in InitVM_Object, it seems working.

    VALUE rb_mImmutable = rb_define_module("Immutable");
    VALUE rb_mImmutableObject = rb_define_module_under(rb_mImmutable, "Object");
$ ./ruby -e 'class X; include Immutable::Object; end; p X.new'
#<X:0x0000000108e7c300>

Updated by ioquatix (Samuel Williams) about 1 month ago

nobu (Nobuyoshi Nakada) with your changes you made, please try running make test. It fails.

Then, use the sample I gave:

> ./ruby -e 'p Immutable::Object.to_s'
-e:1:in `p': method `inspect' called on hidden T_STRING object (0x0000000108b6fa90 flags=0x844005) (NotImplementedError)
    from -e:1:in `<main>'

This seems invalid, no?

Updated by nobu (Nobuyoshi Nakada) about 1 month ago

I see.

diff --git i/variable.c w/variable.c
index aa1fdd022eb..c6e1012bddc 100644
--- i/variable.c
+++ w/variable.c
@@ -202,8 +202,7 @@ build_const_pathname(VALUE head, VALUE tail)
     VALUE path = rb_str_dup(head);
     rb_str_cat2(path, "::");
     rb_str_append(path, tail);
-    OBJ_FREEZE(path);
-    return path;
+    return rb_fstring(path);
 }

 static VALUE

Updated by ioquatix (Samuel Williams) about 1 month ago

nobu (Nobuyoshi Nakada) that makes total sense.

I also had one other idea.

We obviously have a lot of:

VALUE rb_cString;

We initialize this dynamically.

Why not initialize it statically?

struct RClass _cString = ...;
VALUE rb_cString = & _cString;

It seems (1) performance improvement and (2) more predictable usage at least for strings.

Updated by ioquatix (Samuel Williams) about 1 month ago

  • Status changed from Open to Closed

It's fixed.

Actions

Also available in: Atom PDF