From 4b36e129f865f802eb87f7aa2b25e3297c5d8cfd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Thu, 11 Oct 2018 14:43:47 +0100
Subject: [PATCH] encoding/json: always verify we can get a field's value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 src/encoding/json/decode.go      |  2 +-
 src/encoding/json/decode_test.go | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index 6608415e13b..731553dca6b 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 70731a62d6f..54432600a53 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 {