Skip to content

encoding/json: incorrect object key unmarshaling when using custom TextUnmarshaler as Key with string values [1.14 backport] #39585

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.

Sign up for GitHub

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

Jump to bottom
Closed
gopherbot opened this issue Jun 14, 2020 · 3 comments
Closed

encoding/json: incorrect object key unmarshaling when using custom TextUnmarshaler as Key with string values [1.14 backport] #39585

gopherbot opened this issue Jun 14, 2020 · 3 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.14.6

Comments

@gopherbot
Copy link
Contributor

@mvdan requested issue #39555 to be considered for backport to the next 1.14 minor release.

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 gopherbot added the CherryPickCandidate Used during the release process for point releases label Jun 14, 2020
@gopherbot gopherbot mentioned this issue Jun 14, 2020
Closed
@gopherbot gopherbot added this to the Go1.14.5 milestone Jun 14, 2020
@gopherbot
Copy link
Contributor Author

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

@dmitshur
Copy link
Member

dmitshur commented Jul 9, 2020

Approved as this 1.14-only regression is a serious issue without a workaround.

@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Jul 9, 2020
gopherbot pushed a commit that referenced this issue Jul 9, 2020
@mvdan @dmitshur
[release-branch.go1.14] encoding/json: revert "avoid work when unquot…
be0254a 
…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í <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Andrew Bonventre <[email protected]>
(cherry picked from commit 11389ba)
Reviewed-on: https://go-review.googlesource.com/c/go/+/241081
Reviewed-by: Daniel Martí <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
@gopherbot
Copy link
Contributor Author

Closed by merging be0254a to release-branch.go1.14.

@andybons andybons modified the milestones: Go1.14.5, Go1.14.6 Jul 14, 2020
@golang golang locked and limited conversation to collaborators Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.14.6
Development

No branches or pull requests
3 participants
@andybons @dmitshur @gopherbot