Project

General

Profile

Actions

Bug #18141

closed

Marshal load with proc yield objects before they are fully initialized

Added by byroot (Jean Boussier) about 3 years ago. Updated almost 3 years ago.

Status:
Closed
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 3 years ago

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

Actions #3

Updated by nobu (Nobuyoshi Nakada) about 3 years ago

  • 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

Updated by byroot (Jean Boussier) about 3 years 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 3 years 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 3 years 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) about 3 years ago

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

Updated by nagachika (Tomoyuki Chikanaga) about 3 years 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.

Updated by usa (Usaku NAKAMURA) almost 3 years ago

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

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0