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: incorrect object key unmarshaling when using custom TextUnmarshaler as Key with string values #39555

Closed
mrwonko opened this issue Jun 12, 2020 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mrwonko
Copy link

mrwonko commented Jun 12, 2020

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

$ go version
1.14.3

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"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

What did you do?

I have a custom type with MarshalText and UnmarshalText methods. I use it as a key in a map, and the value of the map is string (not reproducible with struct{}), and I json.Marshal that map and then json.Unmarshal it again.

https://play.golang.org/p/D1_ncEiDYT-

What did you expect to see?

UnmarshalText should receive the data that was returned by MarshalText.

What did you see instead?

The data passed to UnmarshalText is an escaped version of the data returned by MarshalText, for example " turns into \" and \ turns into \\.

At first I thought I was running into #38105, but the issue still exists in Go 1.14.4.

@toothrot toothrot added this to the Backlog milestone Jun 12, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 12, 2020
@toothrot
Copy link
Contributor

/cc @rsc

@networkimprov
Copy link

cc @mvdan

@mvdan
Copy link
Member

mvdan commented Jun 14, 2020

Thank you for the report. This is indeed a different regression caused by the same original performance optimization, https://golang.org/cl/190659. The previous fix was good as far as I can tell, but it evidently wasn't a complete fix.

It's pretty obvious that the original optimization was a mistake, and applying more best-effort fixes seems too little and too late at this point. I'll send a CL to revert the entire thing, and add more tests. We can backport this into 1.14.5.

@gopherbot please consider this for backport to 1.14, it's a regression.

@gopherbot
Copy link

Backport issue(s) opened: #39585 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@mvdan mvdan modified the milestones: Backlog, Go1.15 Jun 14, 2020
@mvdan mvdan self-assigned this Jun 14, 2020
@mvdan mvdan added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 14, 2020
@gopherbot
Copy link

Change https://golang.org/cl/237838 mentions this issue: encoding/json: revert "avoid work when unquoting strings, take 2"

@gopherbot
Copy link

Change https://golang.org/cl/241081 mentions this issue: [release-branch.go1.14] encoding/json: revert "avoid work when unquoting strings, take 2"

gopherbot pushed a commit that referenced this issue Jul 9, 2020
…ing strings, take 2"

This reverts golang.org/cl/190659 and golang.org/cl/226218, minus the
regression tests in the latter.

The original work happened in golang.org/cl/151157, which was reverted
in golang.org/cl/190909 due to a crash found by fuzzing.

We tried a second time in golang.org/cl/190659, which shipped with Go
1.14. A bug was found, where strings would be mangled in certain edge
cases. The fix for that was golang.org/cl/226218, which was backported
into Go 1.14.4.

Unfortunately, a second regression was just reported in #39555, which is
a similar case of strings getting mangled when decoding under certain
conditions. It would be possible to come up with another small patch to
fix that edge case, but instead, let's just revert the entire
optimization, as it has proved to do more harm than good. Moreover, it's
hard to argue or prove that there will be no more such regressions.

However, all the work wasn't for nothing. First, we learned that the way
the decoder unquotes tokenized strings isn't simple; initially, we had
wrongly assumed that each string was unquoted exactly once and in order.
Second, we have gained a number of regression tests which will be useful
to prevent the same mistakes in the future, including the test cases we
add in this CL.

For #39555.
Fixes #39585.

Change-Id: I66a6919c2dd6d9789232482ba6cf3814eaa70f61
Reviewed-on: https://go-review.googlesource.com/c/go/+/237838
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
(cherry picked from commit 11389ba)
Reviewed-on: https://go-review.googlesource.com/c/go/+/241081
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Jul 6, 2021
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants