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

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

Comments

@wI2L
Copy link
Contributor

@wI2L 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.

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

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

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.

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

This comment has been minimized.

Copy link
Member

@mvdan 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 label Sep 11, 2019
@mvdan mvdan added this to the Go1.14 milestone Sep 11, 2019
@wI2L

This comment has been minimized.

Copy link
Contributor Author

@wI2L wI2L commented Sep 11, 2019

@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 ?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 11, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.