]> Cypherpunks repositories - gostls13.git/commitdiff
encoding/json: error when trying to set an embedded pointer to unexported struct...
authorJoe Tsai <joetsai@digital-static.net>
Wed, 6 Dec 2017 06:38:36 +0000 (22:38 -0800)
committerJoe Tsai <thebrokentoaster@gmail.com>
Wed, 6 Dec 2017 19:27:26 +0000 (19:27 +0000)
This CL reverts CL 76851 and takes a different approach to #21357.
The changes in encode.go and encode_test.go are reverts that
rolls back the changed behavior in CL 76851 where
embedded pointers to unexported struct types were
unilaterally ignored in both marshal and unmarshal.

Instead, these fields are handled as before with the exception that
it returns an error when Unmarshal is unable to set an unexported field.
The behavior of Marshal is now unchanged with regards to #21357.

This policy maintains the greatest degree of backwards compatibility
and avoids silently discarding data the user may have expected to be present.

Fixes #21357

Change-Id: I7dc753280c99f786ac51acf7e6c0246618c8b2b1
Reviewed-on: https://go-review.googlesource.com/82135
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
src/encoding/json/decode.go
src/encoding/json/decode_test.go
src/encoding/json/encode.go
src/encoding/json/encode_test.go

index 4f98916105b9d9470365e65f8f6ea140f684ca2a..536f25dc7c5eee9f5c995ef6c8437ea9c07a7370 100644 (file)
@@ -707,6 +707,19 @@ func (d *decodeState) object(v reflect.Value) {
                                for _, i := range f.index {
                                        if subv.Kind() == reflect.Ptr {
                                                if subv.IsNil() {
+                                                       // If a struct embeds a pointer to an unexported type,
+                                                       // it is not possible to set a newly allocated value
+                                                       // since the field is unexported.
+                                                       //
+                                                       // See https://golang.org/issue/21357
+                                                       if !subv.CanSet() {
+                                                               d.saveError(fmt.Errorf("json: cannot set embedded pointer to unexported struct: %v", subv.Type().Elem()))
+                                                               // Invalidate subv to ensure d.value(subv) skips over
+                                                               // the JSON value without assigning it to subv.
+                                                               subv = reflect.Value{}
+                                                               destring = false
+                                                               break
+                                                       }
                                                        subv.Set(reflect.New(subv.Type().Elem()))
                                                }
                                                subv = subv.Elem()
index 27ceee471aec5c773e3b37eee7e3d6a97d0074fa..34b7ec6d97abc8fe50a302df253cca7371b586ae 100644 (file)
@@ -195,11 +195,6 @@ type embed struct {
        Q int
 }
 
-type Issue21357 struct {
-       *embed
-       R int
-}
-
 type Loop struct {
        Loop1 int `json:",omitempty"`
        Loop2 int `json:",omitempty"`
@@ -871,20 +866,6 @@ var unmarshalTests = []unmarshalTest{
                err: fmt.Errorf("json: unknown field \"extra\""),
                disallowUnknownFields: true,
        },
-
-       // Issue 21357.
-       // Ignore any embedded fields that are pointers to unexported structs.
-       {
-               in:  `{"Q":1,"R":2}`,
-               ptr: new(Issue21357),
-               out: Issue21357{R: 2},
-       },
-       {
-               in:  `{"Q":1,"R":2}`,
-               ptr: new(Issue21357),
-               err: fmt.Errorf("json: unknown field \"Q\""),
-               disallowUnknownFields: true,
-       },
 }
 
 func TestMarshal(t *testing.T) {
@@ -2107,3 +2088,81 @@ func TestInvalidStringOption(t *testing.T) {
                t.Fatalf("Unmarshal: %v", err)
        }
 }
+
+// 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) {
+       type (
+               embed1 struct{ Q int }
+               embed2 struct{ Q int }
+               embed3 struct {
+                       Q int64 `json:",string"`
+               }
+               S1 struct {
+                       *embed1
+                       R int
+               }
+               S2 struct {
+                       *embed1
+                       Q int
+               }
+               S3 struct {
+                       embed1
+                       R int
+               }
+               S4 struct {
+                       *embed1
+                       embed2
+               }
+               S5 struct {
+                       *embed3
+                       R int
+               }
+       )
+
+       tests := []struct {
+               in  string
+               ptr interface{}
+               out interface{}
+               err error
+       }{{
+               // Error since we cannot set S1.embed1, but still able to set S1.R.
+               in:  `{"R":2,"Q":1}`,
+               ptr: new(S1),
+               out: &S1{R: 2},
+               err: fmt.Errorf("json: cannot set embedded pointer to unexported struct: json.embed1"),
+       }, {
+               // The top level Q field takes precedence.
+               in:  `{"Q":1}`,
+               ptr: new(S2),
+               out: &S2{Q: 1},
+       }, {
+               // No issue with non-pointer variant.
+               in:  `{"R":2,"Q":1}`,
+               ptr: new(S3),
+               out: &S3{embed1: embed1{Q: 1}, R: 2},
+       }, {
+               // No error since both embedded structs have field R, which annihilate each other.
+               // Thus, no attempt is made at setting S4.embed1.
+               in:  `{"R":2}`,
+               ptr: new(S4),
+               out: new(S4),
+       }, {
+               // Error since we cannot set S5.embed1, but still able to set S5.R.
+               in:  `{"R":2,"Q":1}`,
+               ptr: new(S5),
+               out: &S5{R: 2},
+               err: fmt.Errorf("json: cannot set embedded pointer to unexported struct: json.embed3"),
+       }}
+
+       for i, tt := range tests {
+               err := Unmarshal([]byte(tt.in), tt.ptr)
+               if !reflect.DeepEqual(err, tt.err) {
+                       t.Errorf("#%d: %v, want %v", i, err, tt.err)
+               }
+               if !reflect.DeepEqual(tt.ptr, tt.out) {
+                       t.Errorf("#%d: mismatch\ngot:  %#+v\nwant: %#+v", i, tt.ptr, tt.out)
+               }
+       }
+}
index 0522c43495eb39360f49d4b374207654e23d4744..1e45e445d92163f9d756c7ddee66a16e816c483f 100644 (file)
@@ -1094,18 +1094,11 @@ func typeFields(t reflect.Type) []field {
                                isUnexported := sf.PkgPath != ""
                                if sf.Anonymous {
                                        t := sf.Type
-                                       isPointer := t.Kind() == reflect.Ptr
-                                       if isPointer {
+                                       if t.Kind() == reflect.Ptr {
                                                t = t.Elem()
                                        }
-                                       isStruct := t.Kind() == reflect.Struct
-                                       if isUnexported && (!isStruct || isPointer) {
-                                               // Ignore embedded fields of unexported non-struct types
-                                               // or pointers to unexported struct types.
-                                               //
-                                               // The latter is forbidden because unmarshal is unable
-                                               // to assign a new struct to the unexported field.
-                                               // See https://golang.org/issue/21357
+                                       if isUnexported && t.Kind() != reflect.Struct {
+                                               // Ignore embedded fields of unexported non-struct types.
                                                continue
                                        }
                                        // Do not ignore embedded fields of unexported struct types
index df7338c98d159abb6f2c540bdece2565ced925e1..0f194e13d2681f33e8b3752ec276fd851640ceca 100644 (file)
@@ -364,9 +364,8 @@ func TestAnonymousFields(t *testing.T) {
                want: `{"X":2,"Y":4}`,
        }, {
                // Exported fields of pointers to embedded structs should have their
-               // exported fields be serialized only for exported struct types.
-               // Pointers to unexported structs are not allowed since the decoder
-               // is unable to allocate a struct for that field
+               // exported fields be serialized regardless of whether the struct types
+               // themselves are exported.
                label: "EmbeddedStructPointer",
                makeInput: func() interface{} {
                        type (
@@ -379,7 +378,7 @@ func TestAnonymousFields(t *testing.T) {
                        )
                        return S{&s1{1, 2}, &S2{3, 4}}
                },
-               want: `{"Y":4}`,
+               want: `{"X":2,"Y":4}`,
        }, {
                // Exported fields on embedded unexported structs at multiple levels
                // of nesting should still be serialized.