encoding/json: avoid assuming side-effect free reflect.Value.Addr().Elem()

Consider the following:
	type child struct{ Field string }
	type parent struct{ child }

	p := new(parent)
	v := reflect.ValueOf(p).Elem().Field(0)
	v.Field(0).SetString("hello")           // v.Field = "hello"
	v = v.Addr().Elem()                     // v = *(&v)
	v.Field(0).SetString("goodbye")         // v.Field = "goodbye"

It would appear that v.Addr().Elem() should have the same value, and
that it would be safe to set "goodbye".
However, after CL 66331, any interspersed calls between Field calls
causes the RO flag to be set.
Thus, setting to "goodbye" actually causes a panic.

That CL affects decodeState.indirect which assumes that back-to-back
Value.Addr().Elem() is side-effect free. We fix that logic to keep
track of the Addr() and Elem() calls and set v back to the original
after a full round-trip has occured.

Fixes #24152
Updates #24153

Change-Id: Ie50f8fe963f00cef8515d89d1d5cbc43b76d9f9c
Reviewed-on: https://go-review.googlesource.com/97796
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

1 files changed, 46 insertions, 4 deletions

diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go
index 90fdf93dbd..259c8e7cd5 100644
--- a/src/encoding/json/decode_test.go
+++ b/src/encoding/json/decode_test.go
@@ -2089,10 +2089,14 @@ func TestInvalidStringOption(t *testing.T) {
 	}
 }
 
-// Test unmarshal behavior with regards to embedded pointers to unexported structs.
-// If unallocated, this returns an error because unmarshal cannot set the field.
-// Issue 21357.
-func TestUnmarshalEmbeddedPointerUnexported(t *testing.T) {
+// Test unmarshal behavior with regards to embedded unexported structs.
+//
+// (Issue 21357) If the embedded struct is a pointer and is unallocated,
+// this returns an error because unmarshal cannot set the field.
+//
+// (Issue 24152) If the embedded struct is given an explicit name,
+// ensure that the normal unmarshal logic does not panic in reflect.
+func TestUnmarshalEmbeddedUnexported(t *testing.T) {
 	type (
 		embed1 struct{ Q int }
 		embed2 struct{ Q int }
@@ -2119,6 +2123,18 @@ func TestUnmarshalEmbeddedPointerUnexported(t *testing.T) {
 			*embed3
 			R int
 		}
+		S6 struct {
+			embed1 `json:"embed1"`
+		}
+		S7 struct {
+			embed1 `json:"embed1"`
+			embed2
+		}
+		S8 struct {
+			embed1 `json:"embed1"`
+			embed2 `json:"embed2"`
+			Q      int
+		}
 	)
 
 	tests := []struct {
@@ -2154,6 +2170,32 @@ func TestUnmarshalEmbeddedPointerUnexported(t *testing.T) {
 		ptr: new(S5),
 		out: &S5{R: 2},
 		err: fmt.Errorf("json: cannot set embedded pointer to unexported struct: json.embed3"),
+	}, {
+		// Issue 24152, ensure decodeState.indirect does not panic.
+		in:  `{"embed1": {"Q": 1}}`,
+		ptr: new(S6),
+		out: &S6{embed1{1}},
+	}, {
+		// Issue 24153, check that we can still set forwarded fields even in
+		// the presence of a name conflict.
+		//
+		// This relies on obscure behavior of reflect where it is possible
+		// to set a forwarded exported field on an unexported embedded struct
+		// even though there is a name conflict, even when it would have been
+		// impossible to do so according to Go visibility rules.
+		// Go forbids this because it is ambiguous whether S7.Q refers to
+		// S7.embed1.Q or S7.embed2.Q. Since embed1 and embed2 are unexported,
+		// it should be impossible for an external package to set either Q.
+		//
+		// It is probably okay for a future reflect change to break this.
+		in:  `{"embed1": {"Q": 1}, "Q": 2}`,
+		ptr: new(S7),
+		out: &S7{embed1{1}, embed2{2}},
+	}, {
+		// Issue 24153, similar to the S7 case.
+		in:  `{"embed1": {"Q": 1}, "embed2": {"Q": 2}, "Q": 3}`,
+		ptr: new(S8),
+		out: &S8{embed1{1}, embed2{2}, 3},
 	}}
 
 	for i, tt := range tests {