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.

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

encoding/json: detect cyclic maps and slices #40756

Closed
wants to merge 5 commits into from
Closed

encoding/json: detect cyclic maps and slices #40756

wants to merge 5 commits into from

Conversation

lujjjh
Copy link
Contributor

@lujjjh lujjjh commented Aug 13, 2020 •
edited
Loading

Now reports an error if cyclic maps and slices are to be encoded
instead of an infinite recursion. This case wasn't handled in CL 187920.

Fixes #40745.

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Aug 13, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: 920ce83) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/248358 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://golang.org/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@lujjjh
encoding/json: detect cyclic maps and slices
5ea40da 
The documentation says:

JSON cannot represent cyclic data structures and Marshal does not
handle them. Passing cyclic structures to Marshal will result in
an error.

However, passing cyclic maps or slices still results in an infinite
recursion.

Cycle detection is now added into mapEncoder and sliceEncoder.

	name                                 old time/op    new time/op     delta
	CodeEncoder-16                          933µs ± 0%     1324µs ± 0%   ~     (p=1.000 n=1+1)
	CodeMarshal-16                          940µs ± 0%     1436µs ± 0%   ~     (p=1.000 n=1+1)

	name                                 old speed      new speed       delta
	CodeEncoder-16                       2.08GB/s ± 0%   1.47GB/s ± 0%   ~     (p=1.000 n=1+1)
	CodeMarshal-16                       2.06GB/s ± 0%   1.35GB/s ± 0%   ~     (p=1.000 n=1+1)

	name                                 old alloc/op   new alloc/op    delta
	CodeEncoder-16                          7.00B ± 0%  94933.00B ± 0%   ~     (p=1.000 n=1+1)
	CodeMarshal-16                         1.98MB ± 0%     2.04MB ± 0%   ~     (p=1.000 n=1+1)

	name                                 old allocs/op  new allocs/op   delta
	CodeEncoder-16                           0.00        11816.00 ± 0%   ~     (p=1.000 n=1+1)
	CodeMarshal-16                           1.00 ± 0%   11816.00 ± 0%   ~     (p=1.000 n=1+1)

Fixes #40745.
@gopherbot
Copy link
Contributor

This PR (HEAD: 5ea40da) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/248358 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: b69943d) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/248358 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 4: Run-TryBot+1

(4 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=f242f725

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4:

Build is still in progress...
This change failed on js-wasm:
See https://storage.googleapis.com/go-build-log/f242f725/js-wasm_2c4a9e92.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4: TryBot-Result-1

1 of 20 TryBots failed:
Failed on js-wasm: https://storage.googleapis.com/go-build-log/f242f725/js-wasm_2c4a9e92.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 409d916) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/248358 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: d80d86b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/248358 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 7:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 7: Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=752a5eed

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 7: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 8: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 8:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=5c8a51d7

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 8: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 8:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 6f87494) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/248358 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jiahao Lu:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 9: Run-TryBot+1 Code-Review+2 Trust+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 9:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=a5bb170d

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 9: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Bryan C. Mills:

Patch Set 9: Trust+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/248358.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Sep 24, 2020
@lujjjh @mvdan
encoding/json: detect cyclic maps and slices
4285094 
Now reports an error if cyclic maps and slices are to be encoded
instead of an infinite recursion. This case wasn't handled in CL 187920.

Fixes #40745.

Change-Id: Ia34b014ecbb71fd2663bb065ba5355a307dbcc15
GitHub-Last-Rev: 6f87494
GitHub-Pull-Request: #40756
Reviewed-on: https://go-review.googlesource.com/c/go/+/248358
Reviewed-by: Daniel Martí <[email protected]>
Trust: Daniel Martí <[email protected]>
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Go Bot <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/248358 has been merged.

@gopherbot gopherbot closed this Sep 24, 2020
@lujjjh lujjjh deleted the issue-40745 branch September 25, 2020 01:53
e-nikolov pushed a commit to e-nikolov/json that referenced this pull request Nov 25, 2021
@lujjjh @mvdan
encoding/json: detect cyclic maps and slices
38499b2 
Now reports an error if cyclic maps and slices are to be encoded
instead of an infinite recursion. This case wasn't handled in CL 187920.

Fixes #40745.

Change-Id: Ia34b014ecbb71fd2663bb065ba5355a307dbcc15
GitHub-Last-Rev: 6f874944f4065b5237babbb0fdce14c1c74a3c97
GitHub-Pull-Request: golang/go#40756
Reviewed-on: https://go-review.googlesource.com/c/go/+/248358
Reviewed-by: Daniel Martí <[email protected]>
Trust: Daniel Martí <[email protected]>
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Go Bot <[email protected]>
e-nikolov pushed a commit to e-nikolov/json that referenced this pull request Nov 25, 2021
@lujjjh @mvdan
encoding/json: detect cyclic maps and slices
0c7b0d3 
Now reports an error if cyclic maps and slices are to be encoded
instead of an infinite recursion. This case wasn't handled in CL 187920.

Fixes #40745.

Change-Id: Ia34b014ecbb71fd2663bb065ba5355a307dbcc15
GitHub-Last-Rev: 6f874944f4065b5237babbb0fdce14c1c74a3c97
GitHub-Pull-Request: golang/go#40756
Reviewed-on: https://go-review.googlesource.com/c/go/+/248358
Reviewed-by: Daniel Martí <[email protected]>
Trust: Daniel Martí <[email protected]>
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Go Bot <[email protected]>
e-nikolov pushed a commit to e-nikolov/json that referenced this pull request Nov 25, 2021
@lujjjh @mvdan
encoding/json: detect cyclic maps and slices
55dccdb 
Now reports an error if cyclic maps and slices are to be encoded
instead of an infinite recursion. This case wasn't handled in CL 187920.

Fixes #40745.

Change-Id: Ia34b014ecbb71fd2663bb065ba5355a307dbcc15
GitHub-Last-Rev: 6f874944f4065b5237babbb0fdce14c1c74a3c97
GitHub-Pull-Request: golang/go#40756
Reviewed-on: https://go-review.googlesource.com/c/go/+/248358
Reviewed-by: Daniel Martí <[email protected]>
Trust: Daniel Martí <[email protected]>
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

encoding/json: cyclic maps and slices are not detected
3 participants
@lujjjh @gopherbot @googlebot