Project

General

Profile

Actions

Bug #18141

closed

Marshal load with proc yield objects before they are fully initialized

Added by byroot (Jean Boussier) about 2 months ago. Updated 18 days ago.

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

Description

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>

Updated by nobu (Nobuyoshi Nakada) about 2 months ago

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

Actions #3

Updated by nobu (Nobuyoshi Nakada) about 1 month ago

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

Updated by byroot (Jean Boussier) about 1 month ago

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?

Updated by nagachika (Tomoyuki Chikanaga) about 1 month ago

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.

Updated by byroot (Jean Boussier) about 1 month ago

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.

Updated by byroot (Jean Boussier) 29 days ago

  • Status changed from Closed to Open
  • Description updated (diff)
  • 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

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?

Updated by nagachika (Tomoyuki Chikanaga) 18 days ago

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

Actions

Also available in: Atom PDF