The Wayback Machine - https://web.archive.org/web/20201017053802/https://github.com/json-iterator/go/pull/418
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

Add MaxDepth as a config option #418

Merged
merged 2 commits into from Nov 12, 2019
Merged

Conversation

@bbrks
Copy link
Contributor

@bbrks bbrks commented Nov 11, 2019

Default MaxDepth is 10,000 to match the existing maxDepth constant added in #410

ConfigCompatibleWithStandardLibrary retains unlimited depth (via -1), until golang/go#31789 has been decided (it got dropped from the 1.14 milestone)

bbrks added 2 commits Nov 11, 2019
Defaults to 10,000 to match the existing maxDepth constant everywhetre,
except when using `ConfigCompatibleWithStandardLibrary` - which retains
the limitless depth (and causes a stack overflow).

Added tests for the new config, and also up to jsoniter's stack overflow limit.
@codecov
Copy link

@codecov codecov bot commented Nov 11, 2019

Codecov Report

Merging #418 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
+ Coverage   86.45%   86.54%   +0.08%     
==========================================
  Files          41       41              
  Lines        5102     5105       +3     
==========================================
+ Hits         4411     4418       +7     
+ Misses        555      551       -4     
  Partials      136      136
Impacted Files Coverage Δ
config.go 89.52% <100%> (+0.16%) ⬆️
iter.go 90.09% <100%> (ø) ⬆️
reflect_struct_decoder.go 81.89% <0%> (+0.53%) ⬆️

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 dc11f49...2834c7e. Read the comment docs.

@taowen taowen merged commit 44a7e73 into json-iterator:master Nov 12, 2019
4 checks passed
4 checks passed
codeclimate All good!
Details
codecov/patch 100% of diff hit (target 86.45%)
Details
codecov/project 86.54% (+0.08%) compared to dc11f49
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbrks bbrks deleted the bbrks:configurable_maxDepth branch Nov 12, 2019
bbrks added a commit to bbrks/bad_json_parsers that referenced this pull request Nov 12, 2019
Document Go json-iter's configurable max depth limit via `Config.MaxDepth`

json-iterator/go#418
lovasoa added a commit to lovasoa/bad_json_parsers that referenced this pull request Nov 13, 2019
Document Go json-iter's configurable max depth limit via `Config.MaxDepth`

json-iterator/go#418
@liggitt
Copy link
Contributor

@liggitt liggitt commented Dec 19, 2019

"Compatible with the standard library" in this case means "vulnerable to stack overflow". I would strongly recommend this not be configurable and default safe. golang/go#31789 is targeting go1.15

@@ -56,6 +60,7 @@ var ConfigCompatibleWithStandardLibrary = Config{
EscapeHTML: true,
SortMapKeys: true,
ValidateJsonRawMessage: true,
MaxDepth: -1, // encoding/json has no max depth (stack overflow at 2581101)

This comment has been minimized.

@liggitt

liggitt Dec 19, 2019
Contributor

this seems like a harmful default

@liggitt
Copy link
Contributor

@liggitt liggitt commented Dec 19, 2019

Making this configurable, and making a widely used default config setting unsafe means all transitive consumers of this library (caller -> library they don't control -> json-iterator) are exposed to stack overflows once again. I would strongly recommend this be reverted before tagging a release

liggitt added a commit to liggitt/json-iterator that referenced this pull request Dec 20, 2019
…maxDepth"

This reverts commit 44a7e73, reversing
changes made to dc11f49.
taowen added a commit that referenced this pull request Dec 21, 2019
Revert "Merge pull request #418 from bbrks/configurable_maxDepth"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.