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: marshaling a zero value encoding.TextMarshaler interface struct field panics #34235

Closed
wI2L opened this issue Sep 11, 2019 · 3 comments
Closed

encoding/json: marshaling a zero value encoding.TextMarshaler interface struct field panics #34235

wI2L opened this issue Sep 11, 2019 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.14

Comments

@wI2L
Copy link
Contributor

wI2L commented Sep 11, 2019

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

$ go version
go version go1.13 darwin/amd64

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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/will/Library/Caches/go-build"
GOENV="/Users/will/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/will/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/will/github/wI2L/jettison/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/rw/cyt9bjxx34qglfg2jsfm27jr0000gn/T/go-build698090917=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Given the following test program, that attempt to marshal a struct containing a nil instance of encoding.TextMarshaler, json.Marshal panic.
https://play.golang.org/p/ypv_Qj-QFOf

What did you expect to see?

The foollowing JSON string: {"M":null}

What did you see instead?

panic: interface conversion: interface is nil, not encoding.TextMarshaler [recovered]
	panic: interface conversion: interface is nil, not encoding.TextMarshaler

goroutine 1 [running]:
encoding/json.(*encodeState).marshal.func1(0x450f08, 0x8)
	/usr/local/go/src/encoding/json/encode.go:305 +0xc0
panic(0x137ca0, 0x43e3a0)
	/usr/local/go/src/runtime/panic.go:679 +0x240
encoding/json.textMarshalerEncoder(0x45a060, 0x1359e0, 0x40c158, 0x94, 0x100, 0x0)
	/usr/local/go/src/encoding/json/encode.go:485 +0xe0
encoding/json.structEncoder.encode(0x4580f0, 0x1, 0x1, 0x43e340, 0x45a060, 0x1342c0, 0x40c158, 0x99, 0x190100, 0x1342c0)
	/usr/local/go/src/encoding/json/encode.go:664 +0x340
encoding/json.(*encodeState).reflectValue(0x45a060, 0x1342c0, 0x40c158, 0x99, 0x20100, 0x4af2)
	/usr/local/go/src/encoding/json/encode.go:337 +0xa0
encoding/json.(*encodeState).marshal(0x45a060, 0x1342c0, 0x40c158, 0x100, 0x0, 0x0)
	/usr/local/go/src/encoding/json/encode.go:309 +0x120
encoding/json.Marshal(0x1342c0, 0x40c158, 0x1342c0, 0x40c158, 0x3, 0x0, 0x43c0c0, 0x1359e0)
	/usr/local/go/src/encoding/json/encode.go:161 +0x60
main.main()
	/tmp/sandbox165513751/prog.go:12 +0x60

Notes

The program reproducing the panic is an adaptation of an existing testcase of the encoding/json package, that can be found at the following line.

go/src/encoding/json/encode_test.go

Line 480 in 23f7398

{v: struct{ M Marshaler }{}, want: `{"M":null}`},

The marshalerEncoder function checks that the following type assertion succeeds, or write null.

go/src/encoding/json/encode.go

Line 447 in 23f7398

m, ok := v.Interface().(Marshaler)

If we rewrite the program reproducing the panic to use the json.Marshaler interface instead, we observe that the panic no longer occurs.
https://play.golang.org/p/cZVFIU7N216

The following line of the textMarshalerEncoder function should use the same check.

go/src/encoding/json/encode.go

Line 484 in 23f7398

m := v.Interface().(encoding.TextMarshaler)

I am surprised that the TestNilMarshal test is limited to the json.Marshal interface, and that the marshalerEncoder and textMarshalerEncoder functions are not equivalent in behavior. I can't think of ant reasonable reason for this, so I can only guess this is an unfortunate mistake.

The original issue that reported the panic for the json.Marshaler interface is #16042.

/cc @mvdan
@mvdan
Copy link
Member

mvdan commented Sep 11, 2019

Doesn't seem like a recent regression. It looks like you've figured out the fix - want to send a CL?

@mvdan mvdan added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 11, 2019
@mvdan mvdan added this to the Go1.14 milestone Sep 11, 2019
@wI2L
Copy link
Contributor Author

wI2L commented Sep 11, 2019 •
edited

@mvdan Found it the same way as the issue in #33675, by writing some tests for my custom JSON encoder. I guess there's good sides on rewriting the wheel.
I'll send a CL asap.
Edit: What do you think of duplicating the test cases of TestNilMarshaler for encoding.TextMarshaler while we're at it ?

@wI2L wI2L mentioned this issue Sep 11, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/194642 mentions this issue: encoding/json: encode nil encoding.TextMarshaler instance as "null"

@wI2L wI2L mentioned this issue Dec 8, 2019
Closed
@golang golang locked and limited conversation to collaborators Sep 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.14
Development

Successfully merging a pull request may close this issue.

encoding/json: encode nil encoding.TextMarshaler instance as "null"
3 participants
@mvdan @wI2L @gopherbot