Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding/json: json.Unmarshal on embedded unexported structs panics #24152

Closed
luxalpa opened this issue Feb 27, 2018 · 10 comments
Closed

encoding/json: json.Unmarshal on embedded unexported structs panics #24152

luxalpa opened this issue Feb 27, 2018 · 10 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@luxalpa
Copy link

luxalpa commented Feb 27, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10 linux/amd64
go version go1.10 windows/amd64

Does this issue reproduce with the latest release?

yes. Does not happen with go1.9

What operating system and processor architecture are you using (go env)?

Windows 10 amd64
Ubuntu Xenial amd64 16.04.2 LTS

What did you do?

https://play.golang.org/p/ZDEBiSJKERw

What did you expect to see?

The release notes state:

As a result of fixing a reflect bug, Unmarshal can no longer decode into fields inside embedded pointers to unexported struct types, because it cannot initialize the unexported embedded pointer to point at fresh storage. Unmarshal now returns an error in this case.

These fields however aren't pointers and it panics instead of just returning an error

What did you see instead?

panic: reflect: reflect.Value.SetString using value obtained using unexported field [recovered]
	panic: interface conversion: string is not error: missing method Error

goroutine 1 [running]:
encoding/json.(*decodeState).unmarshal.func1(0x1052ff68, 0x10556022)
	/usr/local/go/src/encoding/json/decode.go:177 +0xc0
panic(0x110120, 0x1050c178)
	/usr/local/go/src/runtime/panic.go:505 +0x2c0
reflect.flag.mustBeAssignable(0x1b8, 0x1051403c)
	/usr/local/go/src/reflect/value.go:231 +0x280
reflect.Value.SetString(0x110120, 0x1050c170, 0x1b8, 0x10514058, 0x7, 0x7)
	/usr/local/go/src/reflect/value.go:1551 +0x40
encoding/json.(*decodeState).literalStore(0x10556090, 0x1055804b, 0x9, 0x15, 0x110120, 0x1050c170, 0x1b8, 0x4400)
	/usr/local/go/src/encoding/json/decode.go:957 +0x1fe0
encoding/json.(*decodeState).literal(0x10556090, 0x110120, 0x1050c170, 0x1b8)
	/usr/local/go/src/encoding/json/decode.go:820 +0xe0
encoding/json.(*decodeState).value(0x10556090, 0x110120, 0x1050c170, 0x1b8)
	/usr/local/go/src/encoding/json/decode.go:411 +0x420
encoding/json.(*decodeState).object(0x10556090, 0x112be0, 0x1050c170, 0x1d9)
	/usr/local/go/src/encoding/json/decode.go:754 +0x15a0
encoding/json.(*decodeState).value(0x10556090, 0x112be0, 0x1050c170, 0x1d9)
	/usr/local/go/src/encoding/json/decode.go:408 +0x3e0
encoding/json.(*decodeState).object(0x10556090, 0x107f00, 0x1050c170, 0x16)
	/usr/local/go/src/encoding/json/decode.go:754 +0x15a0
encoding/json.(*decodeState).value(0x10556090, 0x107f00, 0x1050c170, 0x16)
	/usr/local/go/src/encoding/json/decode.go:408 +0x3e0
encoding/json.(*decodeState).unmarshal(0x10556090, 0x107f00, 0x1050c170, 0x105560a0, 0x0, 0x0)
	/usr/local/go/src/encoding/json/decode.go:189 +0x260
encoding/json.Unmarshal(0x10558030, 0x2b, 0x30, 0x107f00, 0x1050c170, 0x2b, 0x30, 0x111ec0)
	/usr/local/go/src/encoding/json/decode.go:108 +0x1a0
main.main()
	/tmp/sandbox919475193/main.go:17 +0xc0
@dsnet dsnet self-assigned this Feb 27, 2018
@dsnet dsnet added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 27, 2018
@Ogditsira
Copy link

Ogditsira commented Feb 27, 2018

I got it to work, the inner nesting of your struct "otherType" needs to be public -> "OtherType".
I guess the reflection to otherType cant determine its data type.
https://play.golang.org/p/8Ssi8ZrD47U

But i don't know if this is intended behavior.

@feilengcui008
Copy link
Contributor

reflect.flag.mustBeAssignable check failed will panic with a string or a struct inplements error interface.

when it panic with a string, the conversion from string to error interface in json.(*decodeState).unmarshal failed and panic.

@luxalpa
Copy link
Author

luxalpa commented Feb 27, 2018

@Ogditsira I know that you can export the type, but the point was previously you didn't have to do it and I'm very sure that this panic is not intended behavior. As noted, in 1.9 it worked flawlessly.

@fraenkel
Copy link
Contributor

This is related to #22546.

@dsnet
Copy link
Member

dsnet commented Feb 27, 2018

This is a regression from 1.9 and should be fixed for 1.11. It's possible that it will even be included in the 1.10.1 point release (whenever that is).

@dsnet
Copy link
Member

dsnet commented Feb 27, 2018

The underlying regression is #24153.

@dsnet dsnet added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 27, 2018
@dsnet dsnet added this to the Go1.11 milestone Feb 27, 2018
@dsnet dsnet modified the milestones: Go1.11, Go1.10.1 Feb 28, 2018
@gopherbot
Copy link

Change https://golang.org/cl/97796 mentions this issue: encoding/json: avoid assuming side-effect free reflect.Value.Addr().Elem()

@dsnet dsnet reopened this Mar 1, 2018
@dsnet
Copy link
Member

dsnet commented Mar 1, 2018

Re-opening for consideration into cherry-picking for Go1.10.1.

@dsnet dsnet removed their assignment Mar 1, 2018
@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 26, 2018
@spf13 spf13 added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 26, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 26, 2018
@andybons
Copy link
Member

CL 97796 OK for Go 1.10.1

@andybons andybons added CherryPickApproved Used during the release process for point releases and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 27, 2018
@gopherbot
Copy link

Change https://golang.org/cl/102784 mentions this issue: [release-branch.go1.10] encoding/json: avoid assuming side-effect free reflect.Value.Addr().Elem()

gopherbot pushed a commit that referenced this issue Mar 29, 2018
…e reflect.Value.Addr().Elem()

Consider the following:
	type child struct{ Field string }
	type parent struct{ child }

	p := new(parent)
	v := reflect.ValueOf(p).Elem().Field(0)
	v.Field(0).SetString("hello")           // v.Field = "hello"
	v = v.Addr().Elem()                     // v = *(&v)
	v.Field(0).SetString("goodbye")         // v.Field = "goodbye"

It would appear that v.Addr().Elem() should have the same value, and
that it would be safe to set "goodbye".
However, after CL 66331, any interspersed calls between Field calls
causes the RO flag to be set.
Thus, setting to "goodbye" actually causes a panic.

That CL affects decodeState.indirect which assumes that back-to-back
Value.Addr().Elem() is side-effect free. We fix that logic to keep
track of the Addr() and Elem() calls and set v back to the original
after a full round-trip has occured.

Fixes #24152
Updates #24153

Change-Id: Ie50f8fe963f00cef8515d89d1d5cbc43b76d9f9c
Reviewed-on: https://go-review.googlesource.com/97796
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/102784
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

9 participants