Bug #19841: Marshal.dump stack overflow with recursive Time

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 3.2.2
• Backport: 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE, 3.4: DONE

[segiddins (Samuel Giddins)]

#!/usr/bin/env ruby

puts RUBY_VERSION

t = Time.at(0, 1, :nanosecond)
t.instance_variable_set :@itself, t

Marshal.dump(t)

Yields a stack overflow error from the Marshal.dump call, even though Marshal is explicitly able to handle cyclical references

[byroot (Jean Boussier)]

• Backport changed from 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN to 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED

It repros on 3.0, 3.1 and master.

[byroot (Jean Boussier)]

Hum, this is tricky.

Time implements _dump, so we enter this branch:

        if (rb_obj_respond_to(obj, s_dump, TRUE)) {
            VALUE ivobj2 = Qundef;
            st_index_t hasiv2;
            VALUE encname2;

            v = INT2NUM(limit);
            v = dump_funcall(arg, obj, s_dump, 1, &v);
            if (!RB_TYPE_P(v, T_STRING)) {
                rb_raise(rb_eTypeError, "_dump() must return string");
            }
            hasiv = has_ivars(obj, (encname = encoding_name(obj, arg)), &ivobj);
            hasiv2 = has_ivars(v, (encname2 = encoding_name(v, arg)), &ivobj2);
            if (hasiv2) {
                hasiv = hasiv2;
                ivobj = ivobj2;
                encname = encname2;
            }
            if (hasiv) w_byte(TYPE_IVAR, arg);
            w_class(TYPE_USERDEF, obj, arg, FALSE);
            w_bytes(RSTRING_PTR(v), RSTRING_LEN(v), arg);
            if (hasiv) {
                w_ivar(hasiv, ivobj, encname, &c_arg);
            }
            w_remember(obj, arg);
            return;
        }

w_remember is what takes care of circular references, and it's explicitly called at the end, contrary to other types where it's called as early as possible.

Calling it sooner fixes this specific issue, but cause the format to change and some other tests to fail. I'll dig more, but I fear we may not be able to fix it without breaking the format.

[nobu (Nobuyoshi Nakada)]

Not only would be data compatibility broken, but I think data created by "remembering" the index of an instance first cannot be loaded.
This is a case specific to TYPE_USERDEF.
Instance variables of such types are dumped/loaded via a transient string, using _dump/_load singleton methods.
At loading, the target instance will be created in _load, the instance variables can not refer that instance which is not created yet.

[byroot (Jean Boussier)]

Right, I fear the best we can do is to detect this case and raise a cleaner error.

[nobu (Nobuyoshi Nakada)]

https://github.com/nobu/ruby/tree/marshal-recursive-userdef

[hsbt (Hiroshi SHIBATA)]

• Backport changed from 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED to 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED, 3.4: REQUIRED

https://github.com/ruby/ruby/pull/12914

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|9459bedd84d479bb1d7d3d40bada1cecb4701c37.

---

[Bug #19841] Refine error on marshaling recursive USERDEF

[tenderlovemaking (Aaron Patterson)]

• Backport changed from 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED, 3.4: REQUIRED to 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED, 3.4: DONE

Backported to 3.4 in ae2fcdc0f705e767045c2bd5253e8eae733d2edb

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED, 3.4: DONE to 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE, 3.4: DONE

ruby_3_3 d2eda78e4091a99c1a387d43967af5794d8eac70 merged revision(s) 9459bedd84d479bb1d7d3d40bada1cecb4701c37.