Bug #18141: Marshal load with proc yield objects before they are fully initialized

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 2.6: REQUIRED, 2.7: DONE, 3.0: DONE

[byroot (Jean Boussier)]

I assume this is a bug because I can't find any spec or test for this behaviour:

Consider the following script:

payload = Marshal.dump("foo")

Marshal.load(payload, -> (obj) {
  if obj.is_a?(String)
    p [obj, obj.encoding]
  end
  obj
})
p [:final, string, string.encoding]

outputs:

["foo", #<Encoding:ASCII-8BIT>]
[:final, "foo", #<Encoding:UTF-8>]

So Marshal call the proc before the string get its encoding assigned, this is because the encoding is stored alongside as a TYPE_IVAR. I think in such cases Marshal should delay calling the proc until the object is fully restored.

A corollary to this behaviour is that the following code:

Marshal.load(payload, :freeze.to_proc)

raises with can't modify frozen String: "foo" (FrozenError).

The same happens with any instance variable on Array or Hash

foo = {}
foo.instance_variable_set(:@bar, 42)

payload = Marshal.dump(foo)

object = Marshal.load(payload, ->(obj) {
  if obj.is_a?(Hash)
    p [obj, obj.instance_variable_get(:@bar)]
    obj.freeze
  end
  obj
})

[{}, nil]
/tmp/marshal.rb:6:in `load': can't modify frozen Hash: {} (FrozenError)
	from /tmp/marshal.rb:6:in `<main>

[byroot (Jean Boussier)]

I potentially have a fix: https://github.com/ruby/ruby/pull/4797

[nobu (Nobuyoshi Nakada)]

Should use ruby_bug "#18141", ""..."3.1" instead of ruby_version_is.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• Backport changed from 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED

[byroot (Jean Boussier)]

So while working on https://bugs.ruby-lang.org/issues/18148, I discovered that many other types of objects are impacted.

Just a few examples:

def round_trip(obj, proc = ->(o) { o.freeze })
  Marshal.load(Marshal.dump(obj), proc)
end

h = {}
h.instance_variable_set(:@foo, 42) #<FrozenError: can't modify frozen Hash>
round_trip(h) rescue p $!

a = []
a.instance_variable_set(:@foo, 42) #<FrozenError: can't modify frozen Array>
round_trip(a) rescue p $!

Also, probably by design, but since you can replace the oject by what the proc returns:

a = {}
a.instance_variable_set(:@foo, 42)
round_trip(a, proc { 24 }) rescue p $! #<FrozenError: can't modify frozen Integer>

I fixed most cases in https://github.com/ruby/ruby/pull/4859, which is my current attempt at implementing https://bugs.ruby-lang.org/issues/18148, but since I just noticed this was marked for backport, I might need to split the bug fix from the new feature. No?

[nagachika (Tomoyuki Chikanaga)]

Hello byroot,
Thank you for the investigation about the issue. Yes, the patch with only the bug fix is very helpful to maintain stable branches.

[byroot (Jean Boussier)]

I made a followup patch: https://github.com/ruby/ruby/pull/4866

It now handle similar bugs with Array, Hash and other mutable objects. It also handle circular objects.

[byroot (Jean Boussier)]

• Subject changed from Marshal load with proc yield strings before they are fully initialized to Marshal load with proc yield objects before they are fully initialized
• Description updated (diff)
• Status changed from Closed to Open

I took the liberty to re-open this issue and to rewrite it to be more generic.

I wonder if it wouldn't be simpler to revert the string only fix (https://github.com/ruby/ruby/pull/4797), and then to merge the more general one (https://github.com/ruby/ruby/pull/4866), this way it would be simpler to backport.

Any opinions?

[byroot (Jean Boussier)]

• Status changed from Open to Closed

https://github.com/ruby/ruby/pull/4866 was merged as 529fc204af84f825f98f83c34b004acbaa802615, closing.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE

ruby_3_0 fe9d33beb78d5c7932a5c2ca3953045c0ae751d5 merged revision(s) 89242279e61b023a81c58065c62a82de8829d0b3,529fc204af84f825f98f83c34b004acbaa802615.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE to 2.6: REQUIRED, 2.7: DONE, 3.0: DONE

ruby_2_7 419266d44c54c6b75f1e824f060c8b388f7a405b merged revision(s) 89242279e61b023a81c58065c62a82de8829d0b3,529fc204af84f825f98f83c34b004acbaa802615.