Bug #18141
closedMarshal load with proc yield objects before they are fully initialized
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 byroot (Jean Boussier) about 3 years ago
I potentially have a fix: https://github.com/ruby/ruby/pull/4797
Updated by nobu (Nobuyoshi Nakada) about 3 years ago
Should use ruby_bug "#18141", ""..."3.1"
instead of ruby_version_is
.
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 byroot (Jean Boussier) about 3 years ago
- Status changed from Open to Closed
https://github.com/ruby/ruby/pull/4866 was merged as 529fc204af84f825f98f83c34b004acbaa802615, closing.
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.