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: cyclic maps and slices are not detected #40745

Open
lujjjh opened this issue Aug 13, 2020 · 6 comments · May be fixed by #40756
Open

encoding/json: cyclic maps and slices are not detected #40745

lujjjh opened this issue Aug 13, 2020 · 6 comments · May be fixed by #40756

Comments

@lujjjh
Copy link

@lujjjh lujjjh commented Aug 13, 2020

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, there are still cases result in an infinite recursion in Go 1.15.

It seems that 64c9ee9 does not handle cyclic maps or slices:

package main

import "encoding/json"

func main() {
	x := map[string]interface{}{}
	x["x"] = x
	json.Marshal(x)
}

and

package main

import "encoding/json"

func main() {
	x := []interface{}{nil}
	x[0] = x
	json.Marshal(x)
}

Originally posted by @lujjjh in #10769 (comment)

@mvdan
Copy link
Member

@mvdan mvdan commented Aug 13, 2020

I agree that this is probably a case we missed and should be fixed. Do you want to send a CL?

lujjjh added a commit to lujjjh/go that referenced this issue Aug 13, 2020
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 are now added into both 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 golang#40745.
@lujjjh lujjjh linked a pull request that will close this issue Aug 13, 2020
lujjjh added a commit to lujjjh/go that referenced this issue Aug 13, 2020
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 are now added into both 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 golang#40745.
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/248358 mentions this issue: encoding/json: detect cyclic maps and slices

lujjjh added a commit to lujjjh/go that referenced this issue Aug 13, 2020
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 golang#40745.
@eaglexiang
Copy link

@eaglexiang eaglexiang commented Aug 14, 2020

great, new test case TestSliceNoCycle was created, but why there is no TestSliceCycle now and even no TestPointerCycle in the past.

@lujjjh
Copy link
Author

@lujjjh lujjjh commented Aug 14, 2020

great, new test case TestSliceNoCycle was created, but why there is no TestSliceCycle now and even no TestPointerCycle in the past.

@eaglexiang Thank you for reviewing the code. Since marshalling cyclic structures results in a UnsupportedValueError, these cases are tested in TestUnsupportedValues.

@lujjjh
Copy link
Author

@lujjjh lujjjh commented Aug 21, 2020

I agree that this is probably a case we missed and should be fixed. Do you want to send a CL?

@mvdan I sent a CL last week, but received no feedback.

This is my first time to conribute to Go. What should I do next? Or just wait?

Thank you.

@mvdan
Copy link
Member

@mvdan mvdan commented Aug 21, 2020

@lujjjh yes, I was pinged by Gerrit a week ago, and I've seen your ping yesterday on Gerrit and your ping here today. I'm taking some time off this week, or at least trying to. A week is not a long time to wait for a review, especially given that it's August.

I'll likely give a review sometime next week, unless someone beats me to it. But in general, please be a bit patient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.