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

Jump to bottom

encoding/json: slice out of bounds decoder crash found via fuzzing #33728

Closed
mvdan opened this issue Aug 20, 2019 · 9 comments
Closed

encoding/json: slice out of bounds decoder crash found via fuzzing #33728

mvdan opened this issue Aug 20, 2019 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone
Go1.13

Comments

@mvdan
Copy link
Member

mvdan commented Aug 20, 2019

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=16500

Filing this issue to track it. I'll update the thread once I have a reproducer.

This is a bug introduced in 1.13, but given how late the release already is, I don't want to mark this as a release blocker. We can easily backport a fix for 1.13.1 if necessary.
@mvdan mvdan added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 20, 2019
@mvdan mvdan added this to the Go1.14 milestone Aug 20, 2019
@mvdan mvdan self-assigned this Aug 20, 2019
@mvdan
Copy link
Member Author

mvdan commented Aug 20, 2019

Manually minified repro case, which errors on 1.12.8, but panics on master (1.13): https://play.golang.org/p/MRohOdk3uIt

@mvdan mvdan changed the title encoding/json: apparent decoder crash found via fuzzing encoding/json: slice out of bounds decoder crash found via fuzzing Aug 20, 2019
@cuonglm
Copy link
Member

cuonglm commented Aug 20, 2019 •
edited

@mvdan it panics for me both with go1.12.8 and master, but with different reason. go1.12.8 gives

panic: json: invalid use of ,string struct tag, trying to unmarshal "\"" into int

@mvdan
Copy link
Member Author

mvdan commented Aug 20, 2019

That's just an error; see my panic(err) line. An error here is correct, because the nested string is missing the closing quote.

@gopherbot
Copy link

Change https://golang.org/cl/190659 mentions this issue: encoding/json: fix regression when decoding bad nested strings

@gopherbot
Copy link

Change https://golang.org/cl/190909 mentions this issue: Revert "encoding/json: avoid work when unquoting strings"

@dmitshur
Copy link
Contributor

dmitshur commented Aug 21, 2019 •
edited

I'll copy @FiloSottile's comment from https://go-review.googlesource.com/c/go/+/190909/1#message-40010dc222e17a86edce656aa3c589f8c5adb95a here:

I don't have a strong opinion on whether to rollback or fix forward. Rolling back feels like the safer option, if Daniel agrees the revert is clean.

This is here so it's easy to choose one or the other. Up to the rest of the OSP team to decide. We should not ship the RC without either though.

All that's left is to select which CL to go with, but we need to do that for 1.13 to avoid shipping a regression in behavior. This is a release blocker, but it shouldn't cause much extra delay because the fixes are ready.

@dmitshur dmitshur modified the milestones: Go1.14, Go1.13 Aug 21, 2019
@dmitshur dmitshur added release-blocker NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Aug 21, 2019
@mvdan
Copy link
Member Author

mvdan commented Aug 21, 2019

Up to you :) I just really, really hope that the release goes out soon.

@geraldstanje
Copy link

when is soon? :)

gopherbot pushed a commit that referenced this issue Aug 21, 2019
@FiloSottile @andybons
Revert "encoding/json: avoid work when unquoting strings"
eee07a8 
This reverts CL 151157.

CL 151157 introduced a crash when decoding into ",string" fields. It
came with a moderate speedup, so at this stage of the release cycle
let's just revert it, and reapply it in Go 1.14 with the fix in CL 190659.

Also applied the test cases from CL 190659.

Updates #33728

Change-Id: Ie46e2bc15224b251888580daf6b79d5865f3878e
Reviewed-on: https://go-review.googlesource.com/c/go/+/190909
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@andybons
Copy link
Member

Revert was landed. Closing.

t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@FiloSottile @t4n6a1ka
Revert "encoding/json: avoid work when unquoting strings"
20798b1 
This reverts CL 151157.

CL 151157 introduced a crash when decoding into ",string" fields. It
came with a moderate speedup, so at this stage of the release cycle
let's just revert it, and reapply it in Go 1.14 with the fix in CL 190659.

Also applied the test cases from CL 190659.

Updates golang#33728

Change-Id: Ie46e2bc15224b251888580daf6b79d5865f3878e
Reviewed-on: https://go-review.googlesource.com/c/go/+/190909
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@golang golang locked and limited conversation to collaborators Aug 20, 2020
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@andybons @dmitshur @mvdan @cuonglm @geraldstanje @gopherbot