internal/vm: Marshal carries the instance variables of String, Array, Hash and Regexp

internal/vm/marshal_codec.go

@@ -134,12 +134,7 @@ func (d *mDumper) writeValue(v object.Value) {
 		if d.link(x) {
 			return
 		}
-		d.writeExtended(x)
-		d.buf = append(d.buf, '[')
-		d.writeLong(len(x.Elems))
-		for _, e := range x.Elems {
-			d.writeValue(e)
-		}
+		d.writeArray(x)
 	case *object.Hash:
 		if d.link(x) {
 			return
@@ -264,6 +259,64 @@ func (d *mDumper) writeEncodingIvar(e *marshalEncIvar) {
 	d.writeValue(object.NewStringBytesEnc([]byte(e.name), "ASCII-8BIT"))
 }
 
+// marshalIvars returns v's instance variables in first-assignment order and the
+// table to read their values from, or nil when it has none to dump.
+//
+// It answers for the kinds with no ivar field of their own — String, Array,
+// Hash, Regexp — whose variables live in the VM-wide generic table, MRI's
+// generic_iv_tbl_. That table is what #674 added; before it, a write to one of
+// those kinds was discarded in silence (#672), so Marshal had nothing to
+// serialise and emitting nothing was indistinguishable from correct. It is
+// correct no longer: MRI's has_ivars reaches every one of these kinds through
+// its `default: generic:` label and counts what rb_ivar_foreach reports.
+//
+// The order pointer is non-nil for every kind routed here (genericIvars records
+// the assignment order), so a nil one means there is nothing to report.
+func marshalIvars(v object.Value) ([]string, map[string]object.Value) {
+	st := ivarStoreOf(v, false)
+	if len(st.tbl) == 0 || st.order == nil {
+		return nil, nil
+	}
+	names := make([]string, 0, len(*st.order))
+	for _, n := range *st.order {
+		if _, live := st.tbl[n]; live {
+			names = append(names, n)
+		}
+	}
+	return names, st.tbl
+}
+
+// writeIvarWrapped emits one value that may carry instance variables, an
+// encoding, or both, in MRI's w_object order: the 'I' (TYPE_IVAR) byte, then
+// w_uclass's 'e'/'C' prefixes, then the payload, then ONE ivar list covering
+// both the encoding ivar and the object's own variables.
+//
+// TYPE_IVAR WRAPS the object; it is not a field of it. The count and the pairs
+// come after the payload, and the encoding ivar comes first within them
+// (w_ivar subtracts what w_encoding wrote from the count it already emitted).
+// The 'I' byte takes no object-link id of its own: on the dump side w_remember
+// runs before it, and on the load side r_object0 registers the wrapped object,
+// so the wrapper is transparent to every back-reference.
+func (d *mDumper) writeIvarWrapped(v object.Value, enc *marshalEncIvar, payload func()) {
+	names, tbl := marshalIvars(v)
+	n := len(names)
+	if enc != nil {
+		n++
+	}
+	if n > 0 {
+		d.buf = append(d.buf, 'I')
+	}
+	d.writeExtended(v)
+	payload()
+	if n > 0 {
+		d.writeLong(n)
+		if enc != nil {
+			d.writeEncodingIvar(enc)
+		}
+		d.writeIvarPairs(names, tbl)
+	}
+}
+
 // writeExtended emits the 'e' (TYPE_EXTENDED) prefix naming each module
 // singleton-extended into v, mirroring MRI's w_extended (marshal.c) called with
 // check=TRUE: the singleton class is stepped over and every module between it
@@ -296,18 +349,25 @@ func (d *mDumper) writeString(s *object.String) {
 	if d.link(s) {
 		return
 	}
-	// MRI writes the 'I' ivar wrapper first, then w_uclass's 'e'/'C' prefixes,
-	// then the payload, then the ivar list (w_object in marshal.c). A binary
-	// string has no encoding ivar and so needs no wrapper.
-	enc := marshalEncodingIvar(s.EncName())
-	if enc != nil {
-		d.buf = append(d.buf, 'I')
-	}
-	d.writeExtended(s)
-	d.writeStringPayload(s)
-	if enc != nil {
-		d.writeLong(1)
-		d.writeEncodingIvar(enc)
+	// A binary string with no instance variables has no encoding ivar either, so
+	// it needs no wrapper at all; one with either takes the 'I' container.
+	d.writeIvarWrapped(s, marshalEncodingIvar(s.EncName()), func() { d.writeStringPayload(s) })
+}
+
+// writeArray emits an Array: the 'I' wrapper when it carries instance
+// variables (an Array is not encoding-capable, so it never has an encoding
+// ivar), then the 'e' prefixes, then the elements, then the ivar list.
+func (d *mDumper) writeArray(a *object.Array) {
+	d.writeIvarWrapped(a, nil, func() { d.writeArrayPayload(a) })
+}
+
+// writeArrayPayload emits just the TYPE_ARRAY tag and elements — the Array
+// counterpart of writeStringPayload, so a 'C' container can own the wrapper.
+func (d *mDumper) writeArrayPayload(a *object.Array) {
+	d.buf = append(d.buf, '[')
+	d.writeLong(len(a.Elems))
+	for _, e := range a.Elems {
+		d.writeValue(e)
 	}
 }
 
@@ -323,7 +383,15 @@ func (d *mDumper) writeHash(h *object.Hash) {
 	if !object.IsNil(h.DefaultProc) {
 		raise("TypeError", "can't dump hash with default proc")
 	}
-	d.writeExtended(h)
+	// A Hash is not encoding-capable, so the wrapper is the ivar list alone. It
+	// opens BEFORE the compare_by_identity 'C' container below, because MRI
+	// writes TYPE_IVAR before w_uclass and before that container.
+	d.writeIvarWrapped(h, nil, func() { d.writeHashPayload(h) })
+}
+
+// writeHashPayload emits the Hash body: the compare_by_identity 'C' container
+// when it applies, then the entries and the default value.
+func (d *mDumper) writeHashPayload(h *object.Hash) {
 	// A Hash put into compare_by_identity mode carries no inline flag in the
 	// stream, so MRI wraps it in a 'C' container naming the Hash class; loading
 	// that container re-applies compare_by_identity.
@@ -393,17 +461,10 @@ func (d *mDumper) writeRegexp(r *Regexp) {
 	opt := regexpMarshalOpts(r)
 	// As for a String: the 'I' wrapper, then w_uclass's 'e'/'C' prefixes, then the
 	// payload, then the ivar list. A binary (ASCII-8BIT) Regexp carries no
-	// encoding ivar, so it is emitted bare.
-	enc := marshalEncodingIvar(regexpMarshalEnc(r))
-	if enc != nil {
-		d.buf = append(d.buf, 'I')
-	}
-	d.writeExtended(r)
-	d.writeRegexpPayload(r, opt)
-	if enc != nil {
-		d.writeLong(1)
-		d.writeEncodingIvar(enc)
-	}
+	// encoding ivar, so one with no variables of its own is emitted bare.
+	d.writeIvarWrapped(r, marshalEncodingIvar(regexpMarshalEnc(r)), func() {
+		d.writeRegexpPayload(r, opt)
+	})
 }
 
 // writeRegexpPayload emits just the TYPE_REGEXP tag, source and option byte —
@@ -716,16 +777,29 @@ func (d *mDumper) writeUserDef(o *RObject) {
 	// skips as "counted elsewhere" — and for a USERDEF container there is no
 	// elsewhere, so the object's own variables are never written. Only the
 	// String's contribute: its encoding, and any variable #_dump set on it.
+	//
+	// writeIvarWrapped is not used here: this container writes no 'e' prefix
+	// (MRI calls w_class with check=FALSE, so w_extended emits nothing), and the
+	// object must be remembered AFTER its variables, which is the reverse of
+	// every other container.
 	enc := marshalEncodingIvar(str.EncName())
+	names, tbl := marshalIvars(str)
+	n := len(names)
 	if enc != nil {
+		n++
+	}
+	if n > 0 {
 		d.buf = append(d.buf, 'I')
 	}
 	d.buf = append(d.buf, 'u')
 	d.writeSymbol(o.class.name)
 	d.writeBytes(str.Str())
-	if enc != nil {
-		d.writeLong(1)
-		d.writeEncodingIvar(enc)
+	if n > 0 {
+		d.writeLong(n)
+		if enc != nil {
+			d.writeEncodingIvar(enc)
+		}
+		d.writeIvarPairs(names, tbl)
 	}
 	d.remember(o)
 }
@@ -1092,6 +1166,10 @@ func (r *mReader) readIvarWrapped() object.Value {
 func (r *mReader) applyIvar(base object.Value, name string, val object.Value) {
 	switch b := base.(type) {
 	case *object.String:
+		// Only :E and :encoding are the encoding; MRI's r_ivar sends everything
+		// sym2encidx does not recognise to rb_ivar_set. Without the default below
+		// each of those was dropped on the floor, so a String round-tripped
+		// through rbgo lost every variable MRI had written for it.
 		switch name {
 		case "E":
 			if val.Truthy() {
@@ -1103,6 +1181,8 @@ func (r *mReader) applyIvar(base object.Value, name string, val object.Value) {
 			if s, ok := val.(*object.String); ok {
 				b.Enc = s.Str()
 			}
+		default:
+			setIvar(base, name, val)
 		}
 	case *Regexp:
 		// The encoding ivar records the Regexp's source encoding, which #encoding
@@ -1118,6 +1198,8 @@ func (r *mReader) applyIvar(base object.Value, name string, val object.Value) {
 			if s, ok := val.(*object.String); ok {
 				b.srcEnc = s.Str()
 			}
+		default:
+			setIvar(base, name, val)
 		}
 	case *RObject:
 		// A built-in subclass loaded from a 'C' container is ONE Ruby object: the

internal/vm/marshal_wave46_test.go

@@ -0,0 +1,154 @@
+package vm_test
+
+import "testing"
+
+// TestMarshalWave46Ivars pins Marshal's handling of the instance variables of
+// the kinds that have no ivar field of their own -- String, Array, Hash, Regexp
+// -- and of the String a #_dump hook returns.
+//
+// Every want value was produced by running the SAME source under the reference
+// `ruby` (MRI 4.0.5) and capturing its output verbatim, so these are witnesses
+// to MRI's bytes rather than transcriptions of rbgo's. The generator refused any
+// case MRI could not run.
+//
+// Cases marked "witness" FAIL on the code before this change: it consulted only
+// the encoding ivar when dumping, and dropped every non-encoding ivar of a
+// String or Regexp when loading. Cases marked "guard" PASS before and after --
+// they are here so that emitting a record MRI does not expect cannot pass
+// unnoticed, which is the way a fix like this breaks specs that used to pass by
+// omission.
+func TestMarshalWave46Ivars(t *testing.T) {
+	cases := []struct{ name, src, want string }{
+		// witness
+		{"str_one_ivar",
+			"s = \"ab\".dup; s.instance_variable_set(:@foo, \"bar\"); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 0, 84]\n"},
+		// witness
+		{"str_three_ivars",
+			"s = \"ab\".dup; s.instance_variable_set(:@a, 1); s.instance_variable_set(:@b, :sym); s.instance_variable_set(:@c, [1]); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 9, 58, 6, 69, 84, 58, 7, 64, 97, 105, 6, 58, 7, 64, 98, 58, 8, 115, 121, 109, 58, 7, 64, 99, 91, 6, 105, 6]\n"},
+		// witness
+		{"str_usascii_ivar",
+			"s = \"ab\".dup.force_encoding(\"US-ASCII\"); s.instance_variable_set(:@foo, 1); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 7, 58, 6, 69, 70, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// witness
+		{"str_latin1_ivar",
+			"s = \"ab\".dup.force_encoding(\"ISO-8859-1\"); s.instance_variable_set(:@foo, 1); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 7, 58, 13, 101, 110, 99, 111, 100, 105, 110, 103, 34, 15, 73, 83, 79, 45, 56, 56, 53, 57, 45, 49, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// witness
+		{"str_binary_ivar_only",
+			"s = \"ab\".dup.force_encoding(\"binary\"); s.instance_variable_set(:@foo, 1); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// guard
+		{"str_binary_no_ivar",
+			"s = \"ab\".dup.force_encoding(\"binary\"); p Marshal.dump(s).bytes",
+			"[4, 8, 34, 7, 97, 98]\n"},
+		// guard
+		{"str_no_ivar_utf8",
+			"p Marshal.dump(\"ab\".dup).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 6, 58, 6, 69, 84]\n"},
+		// witness
+		{"str_ivar_nested_ivar",
+			"i = \"in\".dup; i.instance_variable_set(:@deep, 1); s = \"ou\".dup; s.instance_variable_set(:@inner, i); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 111, 117, 7, 58, 6, 69, 84, 58, 11, 64, 105, 110, 110, 101, 114, 73, 34, 7, 105, 110, 7, 59, 0, 84, 58, 10, 64, 100, 101, 101, 112, 105, 6]\n"},
+		// witness
+		{"str_removed_ivar",
+			"s = \"ab\".dup; s.instance_variable_set(:@a, 1); s.instance_variable_set(:@b, 2); s.send(:remove_instance_variable, :@a); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 7, 64, 98, 105, 7]\n"},
+		// witness
+		{"str_ext_and_ivar",
+			"module Mv; end; s = \"ab\".dup; s.instance_variable_set(:@foo, 1); s.extend(Mv); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 101, 58, 7, 77, 118, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// witness
+		{"ary_ivar",
+			"a = [1]; a.instance_variable_set(:@foo, 1); p Marshal.dump(a).bytes",
+			"[4, 8, 73, 91, 6, 105, 6, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// guard
+		{"ary_no_ivar",
+			"p Marshal.dump([1]).bytes",
+			"[4, 8, 91, 6, 105, 6]\n"},
+		// witness
+		{"ary_ext_ivar",
+			"module Mw; end; a = [1]; a.instance_variable_set(:@foo, 1); a.extend(Mw); p Marshal.dump(a).bytes",
+			"[4, 8, 73, 101, 58, 7, 77, 119, 91, 6, 105, 6, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// witness
+		{"hash_ivar",
+			"h = {1=>2}; h.instance_variable_set(:@foo, 1); p Marshal.dump(h).bytes",
+			"[4, 8, 73, 123, 6, 105, 6, 105, 7, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// witness
+		{"hash_default_ivar",
+			"h = Hash.new(9); h[1] = 2; h.instance_variable_set(:@foo, 1); p Marshal.dump(h).bytes",
+			"[4, 8, 73, 125, 6, 105, 6, 105, 7, 105, 14, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// witness
+		{"hash_identity_ivar",
+			"h = {}.compare_by_identity; h.instance_variable_set(:@foo, 1); p Marshal.dump(h).bytes",
+			"[4, 8, 73, 67, 58, 9, 72, 97, 115, 104, 123, 0, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// guard
+		{"hash_no_ivar",
+			"p Marshal.dump({1=>2}).bytes",
+			"[4, 8, 123, 6, 105, 6, 105, 7]\n"},
+		// witness
+		{"regexp_ivar",
+			"r = Regexp.new(\"ab\"); r.instance_variable_set(:@foo, 1); p Marshal.dump(r).bytes",
+			"[4, 8, 73, 47, 7, 97, 98, 0, 7, 58, 6, 69, 70, 58, 9, 64, 102, 111, 111, 105, 6]\n"},
+		// guard
+		{"regexp_no_ivar",
+			"p Marshal.dump(Regexp.new(\"ab\")).bytes",
+			"[4, 8, 73, 47, 7, 97, 98, 0, 6, 58, 6, 69, 70]\n"},
+		// witness
+		{"link_same_ivar_string_twice",
+			"s = \"ab\".dup; s.instance_variable_set(:@foo, \"bar\"); p Marshal.dump([s, s]).bytes",
+			"[4, 8, 91, 7, 73, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 0, 84, 64, 6]\n"},
+		// witness
+		{"link_shared_ivar_value",
+			"v = \"sh\".dup; s = \"ab\".dup; s.instance_variable_set(:@a, v); s.instance_variable_set(:@b, v); p Marshal.dump(s).bytes",
+			"[4, 8, 73, 34, 7, 97, 98, 8, 58, 6, 69, 84, 58, 7, 64, 97, 73, 34, 7, 115, 104, 6, 59, 0, 84, 58, 7, 64, 98, 64, 6]\n"},
+		// witness
+		{"udump_payload_ivar",
+			"class UDv; def _dump(d); s = \"pl\".dup; s.instance_variable_set(:@foo, \"bar\"); s; end; def self._load(x); new; end; end; p Marshal.dump(UDv.new).bytes",
+			"[4, 8, 73, 117, 58, 8, 85, 68, 118, 7, 112, 108, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 6, 84]\n"},
+		// witness
+		{"udump_payload_ivar_link",
+			"class UDw; def _dump(d); s = \"pl\".dup; s.instance_variable_set(:@foo, \"bar\"); s; end; def self._load(x); new; end; end; o = UDw.new; p Marshal.dump([o, o]).bytes",
+			"[4, 8, 91, 7, 73, 117, 58, 8, 85, 68, 119, 7, 112, 108, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 6, 84, 64, 7]\n"},
+		// guard
+		{"udump_object_ivar_not_dumped",
+			"class UDx; def initialize; @objiv = 1; end; def _dump(d); \"pl\".dup.force_encoding(\"binary\"); end; def self._load(x); new; end; end; p Marshal.dump(UDx.new).bytes",
+			"[4, 8, 117, 58, 8, 85, 68, 120, 7, 112, 108]\n"},
+		// witness
+		{"load_str_ivar",
+			"s = Marshal.load(\"\\x04\\bI\\\"\\aab\\a:\\x06ET:\\t@fooi\\x06\"); p [s, s.encoding.to_s, s.instance_variable_get(:@foo)]",
+			"[\"ab\", \"UTF-8\", 1]\n"},
+		// witness
+		{"load_str_ivar_no_enc",
+			"s = Marshal.load(\"\\x04\\bI\\\"\\aab\\x06:\\t@fooi\\x06\"); p [s, s.encoding.to_s, s.instance_variable_get(:@foo)]",
+			"[\"ab\", \"ASCII-8BIT\", 1]\n"},
+		// witness
+		{"load_regexp_ivar",
+			"r = Marshal.load(\"\\x04\\bI/\\aab\\x00\\a:\\x06EF:\\t@fooi\\x06\"); p [r.source, r.encoding.to_s, r.instance_variable_get(:@foo)]",
+			"[\"ab\", \"US-ASCII\", 1]\n"},
+		// guard
+		{"load_ary_ivar",
+			"a = Marshal.load(\"\\x04\\bI[\\x06i\\x06\\x06:\\t@fooi\\a\"); p [a, a.instance_variable_get(:@foo)]",
+			"[[1], 2]\n"},
+		// guard
+		{"load_hash_ivar",
+			"h = Marshal.load(\"\\x04\\bI{\\x06i\\x06i\\a\\x06:\\t@fooi\\b\"); p [h, h.instance_variable_get(:@foo)]",
+			"[{1 => 2}, 3]\n"},
+		// witness
+		{"load_link_ivar_string_twice",
+			"a = Marshal.load(\"\\x04\\b[\\aI\\\"\\aab\\a:\\x06ET:\\t@foo\\\"\\bbar@\\x06\"); p [a[0].equal?(a[1]), a[0].instance_variable_get(:@foo)]",
+			"[true, \"bar\"]\n"},
+		// witness
+		{"roundtrip_str_ivar",
+			"s = \"ab\".dup; s.instance_variable_set(:@foo, \"bar\"); t = Marshal.load(Marshal.dump(s)); p [t, t.instance_variables, t.instance_variable_get(:@foo)]",
+			"[\"ab\", [:@foo], \"bar\"]\n"},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			if got := eval(t, c.src); got != c.want {
+				t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want)
+			}
+		})
+	}
+}