golang/go

encoding/json: panic for nil map key types implementing TextMarshaler

wI2L opened this issue · 6 comments

wI2L commented

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

$ go version
go version go1.12.8 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
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/will/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/will/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/opt/go/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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-build363570579=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Given the following program that use a map key type implementing the encoding.TextMarshaler interface, when the key is nil, the code panic.
https://play.golang.org/p/KcDK-mOPbqe

What did you expect to see?

Expected a json.UnsupportedValueError, or the key to be encoded as an empty string.

What did you see instead?

Following panic stack trace.
Taken from playground output.

panic: value method net.IP.MarshalText called using nil *IP pointer [recovered]
	panic: value method net.IP.MarshalText called using nil *IP pointer

goroutine 1 [running]:
encoding/json.(*encodeState).marshal.func1(0x44af48, 0x5d4c)
	/usr/local/go/src/encoding/json/encode.go:302 +0xc0
panic(0x1275a0, 0x40c2c0)
	/usr/local/go/src/runtime/panic.go:522 +0x240
net.(*IP).MarshalText(0x0, 0x134040, 0x0, 0x1, 0xeed34048, 0x0, 0x1, 0x128d80)
	<autogenerated>:1 +0xa0
encoding/json.(*reflectWithString).resolve(0x4301e0, 0x2, 0x2, 0x0)
	/usr/local/go/src/encoding/json/encode.go:871 +0x420
encoding/json.mapEncoder.encode(0x15a05c, 0x4540c0, 0x1235c0, 0x43e360, 0x15, 0x120100)
	/usr/local/go/src/encoding/json/encode.go:690 +0x4a0
encoding/json.(*encodeState).reflectValue(0x4540c0, 0x1235c0, 0x43e360, 0x15, 0x120100, 0x207a60)
	/usr/local/go/src/encoding/json/encode.go:334 +0xa0
encoding/json.(*encodeState).marshal(0x4540c0, 0x1235c0, 0x43e360, 0x100, 0x0, 0x0)
	/usr/local/go/src/encoding/json/encode.go:306 +0x100
encoding/json.Marshal(0x1235c0, 0x43e360, 0x40a0f0, 0x5d4c, 0x444260, 0x43e280, 0x0, 0x5d4c)
	/usr/local/go/src/encoding/json/encode.go:160 +0x60
main.main()
	/tmp/sandbox157275249/prog.go:15 +0x1a0

The (encodeWithString).resolve method does not check if a reflect.Value of kind Ptr is nil before doing the type assertion to encoding.TextMarshaler with the result of the (reflect.Value).Interface call.

After adding the following check after line 869 of encoding/json/encode.go:

if w.v.Kind() == reflect.Ptr && w.v.IsNil() {
	return nil
}

I now get this output:

{"":"","127.0.0.1":"localhost"}

Am I missing something regarding nil keys in maps ?

mvdan commented

Could you check older Go versions, to see if this was a regression at some point? Could you also investigate if this happens with either encoding.TextMarshaler or json.Marshaler in other scenarios?

wI2L commented

The change was introduced after this proposal #12146 in this CL https://go-review.googlesource.com/c/go/+/20356/. It was released with go1.7 (see encoding/json changelog part https://golang.org/doc/go1.7).

@mvdan
I've tried to reproduce the panic with the sample program with the last version of all majors: 1.11.13, 1.10.8, 1.9.7, 1.8.7 and 1.7.6, and can confirm that it panics for all.

Marshaling a type that implements the encoding.TextMarshaler interface directly, or via a struct field are fine: https://play.golang.org/p/GdnWla7DMxv
I don't think json.Marshaler is impacted.
What kind of scenarios do you have in mind ?

I discovered this bug while writing the testsuite for a custom JSON encoder, as part of a personal project (most of the test cases outputs are compared against the standard library), and I found no other related issues (using last version 1.12.9).

/cc @augustoroman for original changes

mvdan commented

Fair enough, thanks. Can you send your change above as a CL, along with a test? We can continue the discussion there. Without investigating further, this seems like something to fix.

wI2L commented

@mvdan Sure thing. I'll send a CL.
edit: CL sent with new test testing both nil/non-nil cases.

gopherbot commented

Change https://golang.org/cl/190697 mentions this issue: encoding/json: fix panic for nil instances of TextMarshaler in map keys