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: panic for nil map key types implementing TextMarshaler #33675

Closed
wI2L opened this issue Aug 15, 2019 · 6 comments
Closed

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

wI2L opened this issue Aug 15, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@wI2L
Copy link
Contributor

wI2L commented Aug 15, 2019

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 ?

@wI2L wI2L changed the title encoding/json: panic for nil map keys encoding/json: panic for nil map key types implementing TextMarshaler Aug 15, 2019
@agnivade
Copy link
Contributor

@mvdan @dsnet

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 16, 2019
@agnivade agnivade added this to the Unplanned milestone Aug 16, 2019
@mvdan
Copy link
Member

mvdan commented Aug 16, 2019

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
Copy link
Contributor Author

wI2L commented Aug 16, 2019

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
Copy link
Member

mvdan commented Aug 17, 2019

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
Copy link
Contributor Author

wI2L commented Aug 17, 2019

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

@gopherbot
Copy link

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

t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
This change adds a a check in the encodeWithString.resolve method
to ensure that a reflect.Value with kind Ptr is not nil before
the type assertion to TextMarshaler.

If the value is nil, the method returns a nil error, and the map key
encodes to an empty string.

Fixes golang#33675

Change-Id: I0a04cf690ae67006f6a9c5f8cbb4cc99d236bca8
GitHub-Last-Rev: 6c987c9
GitHub-Pull-Request: golang#33700
Reviewed-on: https://go-review.googlesource.com/c/go/+/190697
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
@golang golang locked and limited conversation to collaborators Aug 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants