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: Unmarshal case insensitive match can lead to nondeterministic behavior #36932

Closed
justinrixx opened this issue Jan 31, 2020 · 3 comments
Milestone

Comments

@justinrixx
Copy link

@justinrixx justinrixx commented Jan 31, 2020

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

$ go version
go version go1.13.3 darwin/amd64

Does this issue reproduce with the latest release?

not sure. i was able to reproduce on my mac (1.13.3), as well as the playground.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Volumes/Code/go/bin"
GOCACHE="/Users/justinr/Library/Caches/go-build"
GOENV="/Users/justinr/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Volumes/Code/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
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/_m/2v0j34ss2xnbndyltfgzgvpr0000gq/T/go-build229954690=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.13.3 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.13.3
uname -v: Darwin Kernel Version 19.3.0: Thu Jan  9 20:58:23 PST 2020; root:xnu-6153.81.5~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.15.3
BuildVersion:	19D76
lldb --version: lldb-1100.0.30.12
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)

What did you do?

https://play.golang.org/p/6lgm5E0R0Nz

essentially, test the behavior of unmarshal with case-sensitive keys.

What did you expect to see?

according to the wording in the godoc (https://godoc.org/encoding/json#Unmarshal):

Unmarshal matches incoming object keys to the keys used by Marshal (either the struct field name or its tag), preferring an exact match but also accepting a case-insensitive match.

i would expect the json key that exactly matches the tag (fieldSETid) to be unmarshalled to the struct (in this case, it should be "sdkgjfhdfsgj") in any json key ordering.

What did you see instead?

the last case-insensitive match encountered in the json object is the accepted value. this is what differs this from this issue: #11673

this would be completely acceptable if it was deterministic every time. however, json keys are not deterministic (in fact, my understanding is that golang's own marshal does not guarantee key order). this can lead to non-deterministic differences in unmarshal behavior, and there's not really a workaround other than changing the incoming payload (which may not be possible due to the fact that it may be generated using a system that does not have deterministic key order).

@dmitshur dmitshur changed the title `encoding/json` `unmarshal` case insensitive match can lead to nondeterministic behavior encoding/json: Unmarshal case insensitive match can lead to nondeterministic behavior Jan 31, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented Feb 1, 2020

I think this is a duplicate of #14750; in particular, see my proposed solution at #14750 (comment). That would essentially be amending the docs to reflect the behavior we have had for years.

The alternative would be to change the implementation to reflect the documentation - that is, to give preference to the case sensitive match, no matter the order. However, I worry that that could easily break existing programs in subtle ways.

In any case, I think the discussion should continue in that previous issue.

@justinrixx
Copy link
Author

@justinrixx justinrixx commented Feb 4, 2020

yeah, i agree that the godoc is inconsistent, but i think this is a bug regardless of what the docs say. imo, the correct bug fix is not changing the docs, it's to change the behavior. i don't think it was ever the intention to cause non-deterministic behavior. in certain scenarios, safely unmarshalling the correct key is not supported, barring writing a custom unmarshaller. if there was a flag or something that you could specify to force the match to be case sensitive, then this would be fine. but as long as order determines which key gets unmarshalled, this is going to be an unsafe and unpredictable operation. i'm fine with moving the discussion over to that issue though. thanks for pointing that out to me.

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Feb 7, 2020

@justinrixx Thank you for submitting this issue. I am going to close it as it seems like a duplicate #14750 (as mentioned earlier in the issue).

@cagedmantis cagedmantis closed this Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.