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: 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
Assignees
Milestone

Comments

@mvdan
Copy link
Member

@mvdan 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 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

This comment has been minimized.

Copy link
Member Author

@mvdan 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

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Aug 20, 2019

@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

This comment has been minimized.

Copy link
Member Author

@mvdan 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 20, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 20, 2019

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

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Aug 21, 2019

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
@mvdan

This comment has been minimized.

Copy link
Member Author

@mvdan mvdan commented Aug 21, 2019

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

@geraldstanje

This comment has been minimized.

Copy link

@geraldstanje geraldstanje commented Aug 21, 2019

when is soon? :)

gopherbot pushed a commit that referenced this issue Aug 21, 2019
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

This comment has been minimized.

Copy link
Member

@andybons andybons commented Aug 21, 2019

Revert was landed. Closing.

@andybons andybons closed this Aug 21, 2019
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.