Backport #8276

Marshal.load modifies restored object after calling #marshal_load

Added by Benoit Daloze about 1 year ago. Updated 12 months ago.

[ruby-core:54334]
Status:Assigned
Priority:Normal
Assignee:Tomoyuki Chikanaga

Description

Given this code, a simple class freezing instances on creation (so they are immutable):

class T
def initialize(data)
@data = data
freeze
end

def marshal_dump
@data
end

def marshal_load data
initialize(data)
end
end

t = T.new("dd")
s = Marshal.dump(t)
p Marshal.load(s) # => ...:in `load': can't modify frozen T (RuntimeError)

Freezing on creation from Marshal is needed to ensure immutability and it seems the right place to do it.
It used to work (ruby 2.0.0dev (2012-07-15 trunk 36395) [x86_64-darwin10.8.0] did fine).
The backtrace:

(gdb) bt
#0 rbraise (exc=4304355040, fmt=0x1001be828 "can't modify frozen %s") at error.c:1810
#1 0x000000010004344f in rb
errorfrozen (what=) at error.c:2020
#2 0x0000000100004600 in rb
encassociateindex (obj=, idx=1) at encoding.c:749
#3 0x000000010008233b in rivar (obj=4320803800, hasencoding=0x0, arg=0x1006b84b0) at marshal.c:1374
#4 0x00000001000811f1 in robject0 (arg=0x1006b84b0, ivp=, extmod=8) at marshal.c:1475
#5 0x00000001000821ad in r
object inlined at /Users/benoitdaloze/EREGONMS/Ruby/git/ruby/marshal.c:1890
#6 0x00000001000821ad in marshal_load (argc=, argv=) at marshal.c:1978
...

History

#1 Updated by Nobuyoshi Nakada about 1 year ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r40366.
Benoit, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


marshal.c: no duplicated encoding

  • marshal.c (wobject): do not dump encoding which is dumped with marshaldump data. [Bug #8276]

#2 Updated by Nobuyoshi Nakada about 1 year ago

  • Tracker changed from Bug to Backport
  • Project changed from ruby-trunk to Backport200
  • Category deleted (core)
  • Status changed from Closed to Assigned
  • Assignee set to Tomoyuki Chikanaga

#3 Updated by Nobuyoshi Nakada about 1 year ago

Note that returning frozen object from marshalload is wrong in common.
In this particular case, removed excess encoding information because it is duplicated to marshal
dump data, but the other instance variables are set after marshalload.
Your code has a potential bug and should separate marshal
dump and initialize.

#4 Updated by Benoit Daloze about 1 year ago

nobu (Nobuyoshi Nakada) wrote:

Note that returning frozen object from marshalload is wrong in common.
In this particular case, removed excess encoding information because it is duplicated to marshal
dump data, but the other instance variables are set after marshalload.
Your code has a potential bug and should separate marshal
dump and initialize.

How would you reliably freeze the object again then?
Should it be a feature of Marshal?

#5 Updated by Tomoyuki Chikanaga 12 months ago

r40559 seems a related commit.

#6 Updated by Benoit Daloze 12 months ago

nagachika (Tomoyuki Chikanaga) wrote:

r40559 seems a related commit.

Indeed. I still do not understand all the details of Marshal though and why instance variables are touched for classes with marshal_{dump,load}.

Anyway, for frozen objects it feels righter to use dump/self.load which allows to freeze easily at initialization.

Also available in: Atom PDF