go: encoding/json: cannot unmarshal into unexported embedded pointer types

A consequence of fixing #21353 is that you cannot round-trip marshal/unmarshal the following:

type embed struct{ X int }
type S struct{ *embed }

in := &S{&embed{5}}
out := &S{}

b, _ := json.Marshal(in) // string(b) == `{"X": 5}`

// Panics, since out.embed is nil and it cannot initialize that field.
json.Unmarshal(b, out)

// Manually allocated embed would allow this to work:
out.embed = &embed{}
json.Unmarshal(b, out)

The problem is that the json package had been relying on a bug within the reflect package, where it was able to allocate the unexported field. With that being fixed in #21353, what should be the correct behavior when unmarshaling into a pointer of an embedded unexported struct type? We can either:

Always ignore the embedded struct
Only ignore the embedded struct if not allocated
Return an error when the embedded struct is not allocated

I believe the 3rd option is the least surprising.

\cc @zombiezen @adams-sarah @cybrcodr @jba @pongad

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 31 (27 by maintainers)

Commits related to this issue

cmd/compile: fix and improve struct field reflect information The previous logic was overly complicated, generated suboptimally encoded struct type descriptors, and mishandled embeddings of predeclar... — committed to golang/go by mdempsky 7 years ago
gen: use NoMethod The fix of golang/go#21357 breaks the generator's trick of declaring "type noMethod T" to avoid infinite recursion when unmarshaling. We just "export" noMethod by capitalizing the f... — committed to googleapis/google-api-go-client by jba 7 years ago
encoding/json: error when trying to set an embedded pointer to unexported struct types This CL reverts CL 76851 and takes a different approach to #21357. The changes in encode.go and encode_test.go a... — committed to golang/go by dsnet 7 years ago
This is unfortunate, but we need to export refs https://github.com/golang/go/issues/21357 — committed to lestrrat-go/jwx by lestrrat 6 years ago
jsonhkp.PublicKey struct needs to be exported Because of golang/go#21353, it is no longer possible for `json.Unmarshal` to set values into unexported structs. This is reported in golang/go#21357: `j... — committed to sfllaw/hkp by sfllaw 6 years ago
internal/fields: handle embedded unexported pointer fields DO NOT SUBMIT: test still fails because of bug in encoding/json fix. As of Go 1.10, a bug in reflect has been fixed, so it is no longer pos... — committed to googleapis/google-cloud-go by jba 7 years ago
samplingRule should be an exported type this commit fixes a runtime panic "cannot set embedded pointer to unexported struct: sampling.samplingRule" see https://github.com/golang/go/issues/21357 — committed to rvdwijngaard/aws-xray-sdk-go by deleted user 6 years ago

Most upvoted comments

I looked through this again. This is really unfortunate. Going back to Joe’s original example:

type embed struct{ X int }
type S struct{ *embed }

in := &S{&embed{5}}
out := &S{}

b, _ := json.Marshal(in) // string(b) == `{"X": 5}`

// Panics, since out.embed is nil and it cannot initialize that field.
json.Unmarshal(b, out)

// Manually allocated embed would allow this to work:
out.embed = &embed{}
json.Unmarshal(b, out)

We tried ignoring that field entirely, and that broke in various ways, so I think it seems pretty clear we should move on to Joe’s option 3: return an error if we want to fill out embed but are now prevented from doing that.

Ignoring the field is much more surprising than I realized: it’s a silent behavioral change. Option 3 should only change some former accidental successes into error results, which is usually preferred anyway.

@dsnet, can you look into a fix for encoding/json? (If not let me know and I will make time.) Thanks.

rsc on Dec 6, 2017