Backport #8276

Marshal.load modifies restored object after calling #marshal_load

Added by Benoit Daloze about 2 years ago. Updated about 2 years 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 rb_raise (exc=4304355040, fmt=0x1001be828 "can't modify frozen %s") at error.c:1810
#1 0x000000010004344f in rb_error_frozen (what=) at error.c:2020
#2 0x0000000100004600 in rb_enc_associate_index (obj=, idx=1) at encoding.c:749
#3 0x000000010008233b in r_ivar (obj=4320803800, has_encoding=0x0, arg=0x1006b84b0) at marshal.c:1374
#4 0x00000001000811f1 in r_object0 (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
...

Associated revisions

Revision 40366
Added by Nobuyoshi Nakada about 2 years ago

marshal.c: no duplicated encoding

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

Revision 40392
Added by Nobuyoshi Nakada about 2 years ago

marshal.c: use ivars of marshal_dump data

  • marshal.c (w_object): dump no ivars to the original by marshal_dump. [Bug #8276]
  • marshal.c (r_object0): copy all ivars of marshal_dump data to the result object instead. [Bug #7627]

Revision 40559
Added by Nobuyoshi Nakada about 2 years ago

marshal.c: no overwriting ivars

  • marshal.c (copy_ivar_i): get rid of overwriting already copied instance variales. c.f. [Bug #8276]

History

#1 Updated by Nobuyoshi Nakada about 2 years 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 (w_object): do not dump encoding which is dumped with marshal_dump data. [Bug #8276]

#2 Updated by Nobuyoshi Nakada about 2 years 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 2 years ago

Note that returning frozen object from marshal_load 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 marshal_load.
Your code has a potential bug and should separate marshal_dump and initialize.

#4 Updated by Benoit Daloze about 2 years ago

nobu (Nobuyoshi Nakada) wrote:

Note that returning frozen object from marshal_load 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 marshal_load.
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 about 2 years ago

r40559 seems a related commit.

#6 Updated by Benoit Daloze about 2 years 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