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, 23 insertions, 1 deletions

diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index f08b0a1c58..dca328062f 100644
--- a/src/encoding/json/decode.go
+++ b/src/encoding/json/decode.go
@@ -448,10 +448,25 @@ func (d *decodeState) valueQuoted() interface{} {
 // if it encounters an Unmarshaler, indirect stops and returns that.
 // if decodingNull is true, indirect stops at the last pointer so it can be set to nil.
 func (d *decodeState) indirect(v reflect.Value, decodingNull bool) (Unmarshaler, encoding.TextUnmarshaler, reflect.Value) {
+	// Issue #24153 indicates that it is generally not a guaranteed property
+	// that you may round-trip a reflect.Value by calling Value.Addr().Elem()
+	// and expect the value to still be settable for values derived from
+	// unexported embedded struct fields.
+	//
+	// The logic below effectively does this when it first addresses the value
+	// (to satisfy possible pointer methods) and continues to dereference
+	// subsequent pointers as necessary.
+	//
+	// After the first round-trip, we set v back to the original value to
+	// preserve the original RW flags contained in reflect.Value.
+	v0 := v
+	haveAddr := false
+
 	// If v is a named type and is addressable,
 	// start with its address, so that if the type has pointer methods,
 	// we find them.
 	if v.Kind() != reflect.Ptr && v.Type().Name() != "" && v.CanAddr() {
+		haveAddr = true
 		v = v.Addr()
 	}
 	for {
@@ -460,6 +475,7 @@ func (d *decodeState) indirect(v reflect.Value, decodingNull bool) (Unmarshaler,
 		if v.Kind() == reflect.Interface && !v.IsNil() {
 			e := v.Elem()
 			if e.Kind() == reflect.Ptr && !e.IsNil() && (!decodingNull || e.Elem().Kind() == reflect.Ptr) {
+				haveAddr = false
 				v = e
 				continue
 			}
@@ -485,7 +501,13 @@ func (d *decodeState) indirect(v reflect.Value, decodingNull bool) (Unmarshaler,
 				}
 			}
 		}
-		v = v.Elem()
+
+		if haveAddr {
+			v = v0 // restore original value after round-trip Value.Addr().Elem()
+			haveAddr = false
+		} else {
+			v = v.Elem()
+		}
 	}
 	return nil, nil, v
 }