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
big-red-dragon opened this issue Feb 27, 2018 · 10 comments

Comments

Projects
None yet
9 participants
@big-red-dragon
Copy link

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

@Ogditsira

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

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.

@big-red-dragon

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

This is related to #22546.

@dsnet

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

The underlying regression is #24153.

@dsnet dsnet added NeedsFix and removed NeedsInvestigation 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

This comment has been minimized.

Copy link

commented Feb 28, 2018

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

@gopherbot gopherbot closed this in 4338518 Mar 1, 2018

@dsnet dsnet reopened this Mar 1, 2018

@dsnet

This comment has been minimized.

Copy link
Member

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 and removed NeedsFix labels Mar 26, 2018

@spf13 spf13 added the NeedsFix label Mar 26, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

CL 97796 OK for Go 1.10.1

@gopherbot

This comment has been minimized.

Copy link

commented Mar 27, 2018

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

[release-branch.go1.10] encoding/json: avoid assuming side-effect fre…
…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>

@andybons andybons closed this Mar 29, 2018

@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.
You can’t perform that action at this time.