encoding/json: always verify we can get a field's value

Calling .Interface on a struct field's reflect.Value isn't always safe.
For example, if that field is an unexported anonymous struct.

We only descended into this branch if the struct type had any methods,
so this bug had gone unnoticed for a few release cycles.

Add the check, and add a simple test case.

Fixes #28145.

Change-Id: I02f7e0ab9a4a0c18a5e2164211922fe9c3d30f64
Reviewed-on: https://go-review.googlesource.com/c/141537
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index 6608415e13b20b857cf2a57fc2f9340b586c5d52..731553dca6b4833db215355888c14ffb771ad278 100644 (file)
--- a/src/encoding/json/decode.go
+++ b/src/encoding/json/decode.go
@@ -473,7 +473,7 @@ func indirect(v reflect.Value, decodingNull bool) (Unmarshaler, encoding.TextUnm
                if v.IsNil() {
                        v.Set(reflect.New(v.Type().Elem()))
                }
-               if v.Type().NumMethod() > 0 {
+               if v.Type().NumMethod() > 0 && v.CanInterface() {
                        if u, ok := v.Interface().(Unmarshaler); ok {
                                return u, nil, reflect.Value{}
                        }

diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go
index 70731a62d6fc0003aa492bfbcc8a7831987aefb1..54432600a533a47be9513cd9c4385e9b7314cbf6 100644 (file)
--- a/src/encoding/json/decode_test.go
+++ b/src/encoding/json/decode_test.go
@@ -266,6 +266,10 @@ type XYZ struct {
        Z interface{}
 }
 
+type unexportedWithMethods struct{}
+
+func (unexportedWithMethods) F() {}
+
 func sliceAddr(x []int) *[]int                 { return &x }
 func mapAddr(x map[string]int) *map[string]int { return &x }
 
@@ -2151,6 +2155,9 @@ func TestInvalidStringOption(t *testing.T) {
 //
 // (Issue 24152) If the embedded struct is given an explicit name,
 // ensure that the normal unmarshal logic does not panic in reflect.
+//
+// (Issue 28145) If the embedded struct is given an explicit name and has
+// exported methods, don't cause a panic trying to get its value.
 func TestUnmarshalEmbeddedUnexported(t *testing.T) {
        type (
                embed1 struct{ Q int }
@@ -2190,6 +2197,9 @@ func TestUnmarshalEmbeddedUnexported(t *testing.T) {
                        embed2 `json:"embed2"`
                        Q      int
                }
+               S9 struct {
+                       unexportedWithMethods `json:"embed"`
+               }
        )
 
        tests := []struct {
@@ -2251,6 +2261,11 @@ func TestUnmarshalEmbeddedUnexported(t *testing.T) {
                in:  `{"embed1": {"Q": 1}, "embed2": {"Q": 2}, "Q": 3}`,
                ptr: new(S8),
                out: &S8{embed1{1}, embed2{2}, 3},
+       }, {
+               // Issue 228145, similar to the cases above.
+               in:  `{"embed": {}}`,
+               ptr: new(S9),
+               out: &S9{},
        }}
 
        for i, tt := range tests {