Project

General

Profile

Actions

Bug #19427

closed

Marshal.load(source, freeze: true) doesn't freeze in some cases

Added by andrykonchin (Andrew Konchin) over 1 year ago. Updated 6 months ago.


Description

I've noticed that the freeze option doesn't work in the following cases:

  • when dumped object extends a module
  • when dumped object responds to #marshal_dump and #marshal_load methods
  • when dumped object responds to #_dump method

Is it expected behaviour or maybe a known issue?

Examples:

module M
end

object = Object.new
object.extend(M)

object = Marshal.load(Marshal.dump(object), freeze: true)
object.frozen? # => false
class UserMarshal
  attr_accessor :data

  def initialize
    @data = 'stuff'
  end
  def marshal_dump() :data end
  def marshal_load(data) @data = data end
end

object = Marshal.load(Marshal.dump(UserMarshal.new), freeze: true)
object.frozen? # => false
class UserDefined
  attr_reader :a, :b

  def initialize
    @a = 'stuff'
    @b = @a
  end

  def _dump(depth)
    Marshal.dump [:stuff, :stuff]
  end

  def self._load(data)
    a, b = Marshal.load data

    obj = allocate
    obj.instance_variable_set :@a, a
    obj.instance_variable_set :@b, b

    obj
  end
end

object = Marshal.load(Marshal.dump(UserDefined.new), freeze: true)
object.frozen? # => false

Related issues 1 (0 open1 closed)

Related to Ruby master - Feature #18148: Marshal.load freeze optionClosedActions
Actions #1

Updated by andrykonchin (Andrew Konchin) over 1 year ago

  • Description updated (diff)
Actions #2

Updated by Eregon (Benoit Daloze) over 1 year ago

Updated by byroot (Jean Boussier) over 1 year ago

I don't think we can do much about the _dump and other callbacks.

However I'll try to have a look at the extended objects.

Updated by Eregon (Benoit Daloze) over 1 year ago

byroot (Jean Boussier) wrote in #note-4:

I don't think we can do much about the _dump and other callbacks.

Couldn't we at least freeze 1) the object returned by _load and 2) the receiver of marshal_load after calling marshal_load?

Yeah we probably can't do much about a, b or data above.
For that to work these callbacks would need to know they should freeze and actually do it.

Updated by Eregon (Benoit Daloze) over 1 year ago

If we had (in-place) deep_freeze or even just an internal version of it, we could call that after _load/marshal_load and then it would really be deeply frozen.
And that could be efficient by using a flag on objects for "deeply frozen/immutable" (which would also imply shareable).

Updated by byroot (Jean Boussier) over 1 year ago

deep_freeze

As always the tricky part it circular references etc. But I guess in the case of freeze it's easy to use the frozen flag can be used to void cycles.

Updated by byroot (Jean Boussier) over 1 year ago

I have a PR for extended objects: https://github.com/ruby/ruby/pull/7284

Interestingly, the callback wouldn't be called either, so I suppose the bug is similar for marshal_load etc.

For those I think we could just call freeze on the return value, it wouldn't be a deep freeze, but I think that's good enough?

Actions #9

Updated by byroot (Jean Boussier) over 1 year ago

  • Status changed from Open to Closed

Applied in changeset git|7ddcee5928d8a98337077d5a5ee61136ec84a993.


Marshal.load: also freeze extended objects

[Bug #19427]

The proc wouldn't be called either, that fixes both.

Actions #10

Updated by byroot (Jean Boussier) over 1 year ago

  • Backport changed from 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN to 2.7: WONTFIX, 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED
Actions #11

Updated by byroot (Jean Boussier) over 1 year ago

  • Backport changed from 2.7: WONTFIX, 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED to 2.7: DONTNEED, 3.0: DONTNEED, 3.1: REQUIRED, 3.2: REQUIRED

Updated by andrykonchin (Andrew Konchin) over 1 year ago

The issue was closed. Does it mean that current behaviour of TYPE_USERDEF and TYPE_USRMARSHAL is expected?

Or should I create separate issues to track them independently?

Updated by byroot (Jean Boussier) over 1 year ago

  • Status changed from Closed to Open

The issue closes automatically when a commit is merged with a reference to it. I can re-open, but I'm not super hopeful about fixing these other two cases.

As explained, I think the best we can do is shallow freeze.

Updated by Eregon (Benoit Daloze) over 1 year ago

Shallow freezing of TYPE_USERDEF and TYPE_USRMARSHAL is not done:

2)
Marshal.load when called with freeze: true returns frozen object having #_dump method FAILED
Expected #<UserDefined:0x00007f241ebb8e58 @a=:stuff, @b=:stuff>.frozen?
to be truthy but was false
/home/eregon/code/rubyspec/core/marshal/shared/load.rb:146:in `block (5 levels) in <top (required)>'
/home/eregon/code/rubyspec/core/marshal/load_spec.rb:4:in `<top (required)>'

3)
Marshal.load when called with freeze: true returns frozen object responding to #marshal_dump and #marshal_load FAILED
Expected #<UserMarshal:0x00007f2430a2ab10 @data=:data>.frozen?
to be truthy but was false
/home/eregon/code/rubyspec/core/marshal/shared/load.rb:151:in `block (5 levels) in <top (required)>'
/home/eregon/code/rubyspec/core/marshal/load_spec.rb:4:in `<top (required)>'
      ruby_bug "#19427", "3.1"..."3.3" do
        it "returns frozen object having #_dump method" do
          object = Marshal.send(@method, Marshal.dump(UserDefined.new), freeze: true)
          object.should.frozen?
        end

        it "returns frozen object responding to #marshal_dump and #marshal_load" do
          object = Marshal.send(@method, Marshal.dump(UserMarshal.new), freeze: true)
          object.should.frozen?
        end

So I reopen to track shallow-freezing those.

Updated by Eregon (Benoit Daloze) over 1 year ago

  • Assignee set to byroot (Jean Boussier)
Actions #18

Updated by byroot (Jean Boussier) over 1 year ago

  • Status changed from Open to Closed

Applied in changeset git|6339cb70c3bcc54696e98c303dd4b26ed3d57afd.


marshal.c: shallow freeze user objects

When freeze: true argument is passed.

[Bug #19427]

Updated by nagachika (Tomoyuki Chikanaga) 6 months ago

  • Backport changed from 2.7: DONTNEED, 3.0: DONTNEED, 3.1: REQUIRED, 3.2: REQUIRED to 2.7: DONTNEED, 3.0: DONTNEED, 3.1: WONTFIX, 3.2: WONTFIX

I think it's obviously a bug, but I'm concerned that changing the behavior might cause FrozenError in applications. Therefore, I have decided not to backport the changesets related to this issue.
Please feel free to raise any objections to this decision.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like1Like0