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

Jump to bottom

encoding/json: panic when unmarshalling to struct with embedded struct pointer #30489

Closed
philpearl opened this issue Feb 28, 2019 · 5 comments
Closed

encoding/json: panic when unmarshalling to struct with embedded struct pointer #30489

philpearl opened this issue Feb 28, 2019 · 5 comments
Labels
FrozenDueToAge

Comments

@philpearl
Copy link
Contributor

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

1.12

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/phil/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/phil/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/phil/go/src/github.com/unravelin/core/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0j/nhsrh_152tg44s03bvq7mrh80000gn/T/go-build963035476=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

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

What did you expect to see?

Certainly no panic. Ideally the struct would be fully populated from the json

c := C{
    D: "dddd",
    A: &A{
        B: "gat",
    },
}

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0xffffffff addr=0x0 pc=0x107ad2]

goroutine 1 [running]:
main.(*A).UnmarshalJSON(0x0, 0x45a000, 0x1c, 0x1c, 0x0, 0x0)
/tmp/sandbox606414898/main.go:17 +0x12
encoding/json.(*decodeState).object(0x454060, 0x1242c0, 0x40a0e0, 0x16, 0x454078, 0x207b)
/usr/local/go/src/encoding/json/decode.go:611 +0x2920
encoding/json.(*decodeState).value(0x454060, 0x1242c0, 0x40a0e0, 0x16, 0x454078, 0x22)
/usr/local/go/src/encoding/json/decode.go:381 +0x60
encoding/json.(*decodeState).unmarshal(0x454060, 0x1242c0, 0x40a0e0, 0x454078, 0x0, 0x0)
/usr/local/go/src/encoding/json/decode.go:179 +0x240
encoding/json.Unmarshal(0x45a000, 0x1c, 0x1c, 0x1242c0, 0x40a0e0, 0x20af, 0x8, 0x1250a0)
/usr/local/go/src/encoding/json/decode.go:106 +0x180
main.main()
/tmp/sandbox606414898/main.go:23 +0xc0

Looks like the code sees that c is non-null and implements Unmarshaller, so calls UnmarshalJSON with c.A as the receiver. Unfortunately c.A is nil. Ideally the library would detect that c does not implement Unmarshaller itself and carry on with standard decoding.

The big problem here is that you can have working code, then implement a working unmarshaller on a struct and then find the code as a whole breaks. I found this after generating some easyjson unmarshallers for a subset of the structs in our codebase.

Note similarity to #22967
@dsnet
Copy link
Member

dsnet commented Feb 28, 2019

The root cause of this is #30148. Closing as duplicate.

@dsnet dsnet closed this as completed Feb 28, 2019
@philpearl
Copy link
Contributor Author

philpearl commented Mar 1, 2019 •
edited

Wow. I understand the difficulty here, but just closing (and presumably ignoring - forgive me if there's a process I don't understand here) seems to be ignoring a big issue. The whole idea of unmarshallers (and marshallers https://play.golang.org/p/KsYZvfY7kvk) seems to be a little broken with embedding.

The "spooky action at a distance" is very unsettling here. I generated some easyjson marshallers for core structs, then found much later on that this broke unrelated code that happened to embed these structs. This means I can never implement an unmarshaller on any public struct in case someone somewhere has embedded it and things will start crashing

Cheeky little /cc @rsc

@dsnet
Copy link
Member

dsnet commented Mar 1, 2019

ignoring a big issue

Perhaps, but not one specific to encoding/json. Embedding in Go is long known to lead to brittle code. There is an outstanding proposal to remove it in Go 2. See #22013

@dsnet
Copy link
Member

dsnet commented Mar 1, 2019 •
edited

This means I can never implement an unmarshaller on any public struct in case someone somewhere has embedded it and things will start crashing

I would say the opposite is true. Users should never embed a type that they do not own because it is unreasonable to expect that the author of the type never adds new fields and methods. Of course, people do this anyways and are surprised when their code breaks. Hence, the brittleness of embedding.

FYI, the Go1 compatibility calls this out:

Under some circumstances, such as when the type is embedded in a struct along with another type, the addition of the new method may break the struct by creating a conflict with an existing method of the other embedded type. We cannot protect against this rare case and do not guarantee compatibility should it arise.

@philpearl
Copy link
Contributor Author

philpearl commented Mar 1, 2019 via email

@golang golang locked and limited conversation to collaborators Feb 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@philpearl @dsnet @gopherbot