Project

General

Profile

Actions

Bug #19841

open

Marshal.dump stack overflow with recursive Time

Added by segiddins (Samuel Giddins) 10 months ago. Updated 9 months ago.

Status:
Open
Assignee:
-
Target version:
-
[ruby-core:114410]

Description

#!/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

Updated by byroot (Jean Boussier) 9 months ago

  • 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.

Updated by byroot (Jean Boussier) 9 months ago

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.

Updated by nobu (Nobuyoshi Nakada) 9 months ago

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.

Updated by byroot (Jean Boussier) 9 months ago

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

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0