From: Daniel Martí <mvdan@mvdan.cc>
Date: Thu, 11 Oct 2018 13:43:47 +0000 (+0100)
Subject: encoding/json: always verify we can get a field's value
X-Git-Tag: go1.12beta1~758
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=4b36e129f865f802eb87f7aa2b25e3297c5d8cfd;p=gostls13.git

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 6608415e13..731553dca6 100644
--- 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 70731a62d6..54432600a5 100644
--- 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 {