Skip to content

Revert "Merge pull request #418 from bbrks/configurable_maxDepth" #425

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

Merged
merged 1 commit into from
Dec 21, 2019

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Dec 20, 2019

This reverts commit 44a7e73, reversing
changes made to dc11f49.

This protects callers of the library by default. Given the golang parser is expected to set the same hard-coded limit in 1.15, I think more evidence is required before making this configurable and unsafe by default.

cc @taowen

…maxDepth"

This reverts commit 44a7e73, reversing
changes made to dc11f49.
@liggitt
Copy link
Contributor Author

liggitt commented Dec 20, 2019

I would strongly recommend waiting to tag a release until this is resolved. There is already an expectation from downstream consumers that this library is not vulnerable to stack overflows, even in transitive uses of the ConfigCompatibleWithStandardLibrary

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #425 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
+ Coverage   86.46%   86.49%   +0.03%     
==========================================
  Files          41       41              
  Lines        5105     5102       -3     
==========================================
- Hits         4414     4413       -1     
+ Misses        555      553       -2     
  Partials      136      136
Impacted Files Coverage Δ
config.go 89.36% <ø> (-0.17%) ⬇️
iter.go 90.09% <100%> (ø) ⬆️
reflect_struct_decoder.go 81.62% <0%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44a7e73...7c9f8c2. Read the comment docs.

@bbrks
Copy link
Contributor

bbrks commented Dec 21, 2019

I'm not sure removing the configurable depth value altogether is the right approach either.

I can see perfectly valid use-cases where you'd want to be able to parse JSON deeper than 10,000 levels, especially given that the Go stdlib is currently perfectly capable of a couple of orders of magnitude more levels than 10,000.

@taowen taowen merged commit d1af763 into json-iterator:master Dec 21, 2019
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
Revert "Merge pull request json-iterator#418 from bbrks/configurable_maxDepth"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants