[GR-18163] Fix Marshal.load of multiple Symbols with an explicit encoding (#1624)

PullRequest: truffleruby/2781

Author: eregon

CHANGELOG.md

@@ -5,6 +5,7 @@ New features:
 
 Bug fixes:
 
+* Fix `Marshal.load` of multiple `Symbols` with an explicit encoding (#1624).
 
 Compatibility:
 

spec/ruby/core/marshal/dump_spec.rb

@@ -78,6 +78,20 @@
       s = "\u2192".force_encoding("binary").to_sym
       Marshal.dump(s).should == "\x04\b:\b\xE2\x86\x92"
     end
+
+    it "dumps multiple Symbols sharing the same encoding" do
+      # Note that the encoding is a link for the second Symbol
+      symbol1 = "I:\t\xE2\x82\xACa\x06:\x06ET"
+      symbol2 = "I:\t\xE2\x82\xACb\x06;\x06T"
+      value = [
+        "€a".force_encoding(Encoding::UTF_8).to_sym,
+        "€b".force_encoding(Encoding::UTF_8).to_sym
+      ]
+      Marshal.dump(value).should == "\x04\b[\a#{symbol1}#{symbol2}"
+
+      value = [*value, value[0]]
+      Marshal.dump(value).should == "\x04\b[\b#{symbol1}#{symbol2};\x00"
+    end
   end
 
   describe "with an object responding to #marshal_dump" do
@@ -343,8 +357,13 @@
     end
 
     it "dumps an extended Struct" do
-      st = Struct.new("Extended", :a, :b).new
-      Marshal.dump(st.extend(Meths)).should == "\004\be:\nMethsS:\025Struct::Extended\a:\006a0:\006b0"
+      obj = Struct.new("Extended", :a, :b).new.extend(Meths)
+      Marshal.dump(obj).should == "\004\be:\nMethsS:\025Struct::Extended\a:\006a0:\006b0"
+
+      s = 'hi'
+      obj.a = [:a, s]
+      obj.b = [:Meths, s]
+      Marshal.dump(obj).should == "\004\be:\nMethsS:\025Struct::Extended\a:\006a[\a;\a\"\ahi:\006b[\a;\000@\a"
       Struct.send(:remove_const, :Extended)
     end
   end

spec/ruby/core/marshal/fixtures/marshal_data.rb

@@ -83,7 +83,7 @@ class UserPreviouslyDefinedWithInitializedIvar
 end
 
 class UserMarshal
-  attr_reader :data
+  attr_accessor :data
 
   def initialize
     @data = 'stuff'

spec/ruby/core/marshal/shared/load.rb

@@ -309,7 +309,8 @@
 
     it "loads an extended Array object containing a user-marshaled object" do
       obj = [UserMarshal.new, UserMarshal.new].extend(Meths)
-      new_obj = Marshal.send(@method, "\x04\be:\nMeths[\ao:\x10UserMarshal\x06:\n@dataI\"\nstuff\x06:\x06ETo;\x06\x06;\aI\"\nstuff\x06;\bT")
+      dump = "\x04\be:\nMeths[\ao:\x10UserMarshal\x06:\n@dataI\"\nstuff\x06:\x06ETo;\x06\x06;\aI\"\nstuff\x06;\bT"
+      new_obj = Marshal.send(@method, dump)
 
       new_obj.should == obj
       obj_ancestors = class << obj; ancestors[1..-1]; end
@@ -399,6 +400,24 @@
       sym.should == s
       sym.encoding.should == Encoding::BINARY
     end
+
+    it "loads multiple Symbols sharing the same encoding" do
+      # Note that the encoding is a link for the second Symbol
+      symbol1 = "I:\t\xE2\x82\xACa\x06:\x06ET"
+      symbol2 = "I:\t\xE2\x82\xACb\x06;\x06T"
+      dump = "\x04\b[\a#{symbol1}#{symbol2}"
+      value = Marshal.send(@method, dump)
+      value.map(&:encoding).should == [Encoding::UTF_8, Encoding::UTF_8]
+      expected = [
+        "€a".force_encoding(Encoding::UTF_8).to_sym,
+        "€b".force_encoding(Encoding::UTF_8).to_sym
+      ]
+      value.should == expected
+
+      value = Marshal.send(@method, "\x04\b[\b#{symbol1}#{symbol2};\x00")
+      value.map(&:encoding).should == [Encoding::UTF_8, Encoding::UTF_8, Encoding::UTF_8]
+      value.should == [*expected, expected[0]]
+    end
   end
 
   describe "for a String" do
@@ -460,20 +479,23 @@
   describe "for a Struct" do
     it "loads a extended_struct having fields with same objects" do
       s = 'hi'
-      obj = Struct.new("Ure2", :a, :b).new.extend(Meths)
+      obj = Struct.new("Extended", :a, :b).new.extend(Meths)
+      dump = "\004\be:\nMethsS:\025Struct::Extended\a:\006a0:\006b0"
+      Marshal.send(@method, dump).should == obj
+
       obj.a = [:a, s]
       obj.b = [:Meths, s]
-
-      Marshal.send(@method,
-        "\004\be:\nMethsS:\021Struct::Ure2\a:\006a[\a;\a\"\ahi:\006b[\a;\000@\a"
-        ).should == obj
-      Struct.send(:remove_const, :Ure2)
+      dump = "\004\be:\nMethsS:\025Struct::Extended\a:\006a[\a;\a\"\ahi:\006b[\a;\000@\a"
+      Marshal.send(@method, dump).should == obj
+      Struct.send(:remove_const, :Extended)
     end
 
     it "loads a struct having ivar" do
       obj = Struct.new("Thick").new
       obj.instance_variable_set(:@foo, 5)
-      Marshal.send(@method, "\004\bIS:\022Struct::Thick\000\006:\t@fooi\n").should == obj
+      reloaded = Marshal.send(@method, "\004\bIS:\022Struct::Thick\000\006:\t@fooi\n")
+      reloaded.should == obj
+      reloaded.instance_variable_get(:@foo).should == 5
       Struct.send(:remove_const, :Thick)
     end
 
@@ -588,6 +610,18 @@
     end
   end
 
+  describe "for an object responding to #marshal_dump and #marshal_load" do
+    it "loads a user-marshaled object" do
+      obj = UserMarshal.new
+      obj.data = :data
+      value = [obj, :data]
+      dump = Marshal.dump(value)
+      dump.should == "\x04\b[\aU:\x10UserMarshal:\tdata;\x06"
+      reloaded = Marshal.load(dump)
+      reloaded.should == value
+    end
+  end
+
   describe "for a user object" do
     it "loads a user-marshaled extended object" do
       obj = UserMarshal.new.extend(Meths)

src/main/ruby/truffleruby/core/marshal.rb

@@ -553,7 +553,18 @@ def add_object(obj)
     end
 
     def add_symlink(obj)
-      sz = @symlinks.size
+      sz = @symbols.size
+      @symbols[sz] = obj
+      @symlinks[obj.__id__] = sz
+    end
+
+    def reserve_symlink
+      sz = @symbols.size
+      @symbols[sz] = nil
+      sz
+    end
+
+    def assign_reserved_symlink(sz, obj)
       @symbols[sz] = obj
       @symlinks[obj.__id__] = sz
     end
@@ -848,7 +859,6 @@ def construct_regexp
 
     def construct_struct
       name = get_symbol
-      store_unique_object name
 
       klass = const_lookup name, Class
       members = klass.members
@@ -859,8 +869,7 @@ def construct_struct
       construct_integer.times do |i|
         slot = get_symbol
         unless members[i].intern == slot
-          raise TypeError, 'struct %s is not compatible (%p for %p)' %
-            [klass, slot, members[i]]
+          raise TypeError, "struct #{klass} is not compatible (#{slot.inspect} for #{members[i].inspect})"
         end
 
         Primitive.object_hidden_var_set obj, slot, construct
@@ -872,6 +881,12 @@ def construct_struct
     def construct_symbol(ivar_index)
       data = get_byte_sequence
 
+      # We must use the next @symbols index for the Symbol being constructed.
+      # However, constructing a Symbol might require to construct the :E or :encoding symbols,
+      # and those must have a larger index (if they are serialized for the first time),
+      # to be binary-compatible with CRuby and pass specs. So we reserve the index and assign it later.
+      idx = reserve_symlink
+
       # A Symbol has no instance variables (it's frozen),
       # but we need to know the encoding before building the Symbol
       if ivar_index and @has_ivar[ivar_index]
@@ -881,7 +896,7 @@ def construct_symbol(ivar_index)
       end
 
       obj = data.to_sym
-      store_unique_object obj
+      assign_reserved_symlink idx, obj
 
       obj
     end
@@ -910,7 +925,6 @@ def construct_user_defined(ivar_index)
 
     def construct_user_marshal
       name = get_symbol
-      store_unique_object name
 
       klass = const_lookup name, Class
       obj = klass.allocate
@@ -976,7 +990,7 @@ def serialize(obj)
       raise ArgumentError, 'exceed depth limit' if @depth == 0
 
       # How much depth we have left.
-      @depth -= 1;
+      @depth -= 1
 
       if link = find_link(obj)
         str = Truffle::Type.binary_string("@#{serialize_integer(link)}")
@@ -1249,7 +1263,10 @@ def initialize(stream, depth, prc)
       if @stream
         @byte_array = stream.bytes
       end
+    end
 
+    def inspect
+      "#<Marshal::StringState #{@stream[@consumed..-1].inspect}>"
     end
 
     def consume(bytes)