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

Add MaxDepth as a config option #418

Merged
merged 2 commits into from
Nov 12, 2019
Merged

Add MaxDepth as a config option #418

merged 2 commits into from
Nov 12, 2019

Conversation

bbrks
Copy link
Contributor

@bbrks bbrks commented Nov 11, 2019 •
edited
Loading

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
Adds MaxDepth config option
d296277 
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.
@bbrks
Remove large test values that fail on 32-bit architectures
2834c7e
@codecov
Copy link

codecov bot commented Nov 11, 2019 •
edited
Loading

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
@bbrks bbrks deleted the configurable_maxDepth branch November 12, 2019 16:50
bbrks added a commit to bbrks/bad_json_parsers that referenced this pull request Nov 12, 2019
@bbrks
Go json-iter configurable MaxDepth
05aa427 
Document Go json-iter's configurable max depth limit via `Config.MaxDepth`

json-iterator/go#418
@bbrks bbrks mentioned this pull request Nov 12, 2019
Merged
lovasoa pushed a commit to lovasoa/bad_json_parsers that referenced this pull request Nov 13, 2019
@bbrks @lovasoa
Go json-iter configurable MaxDepth (#39)
f8f3dbc 
Document Go json-iter's configurable max depth limit via `Config.MaxDepth`

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

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

liggitt
liggitt reviewed Dec 19, 2019
View reviewed changes
config.go
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a harmful default

@liggitt
Copy link
Contributor

liggitt commented Dec 19, 2019 •
edited
Loading

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
@liggitt
Revert "Merge pull request json-iterator#418 from bbrks/configurable_…
7c9f8c2 
…maxDepth"

This reverts commit 44a7e73, reversing
changes made to dc11f49.
taowen added a commit that referenced this pull request Dec 21, 2019
@taowen
Merge pull request #425 from liggitt/default-max-depth
d1af763 
Revert "Merge pull request #418 from bbrks/configurable_maxDepth"
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
@taowen
Merge pull request json-iterator#418 from bbrks/configurable_maxDepth
67614f9 
Add MaxDepth as a config option
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
@liggitt
Revert "Merge pull request json-iterator#418 from bbrks/configurable_…
bc78e4b 
…maxDepth"

This reverts commit 44a7e73, reversing
changes made to dc11f49.
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
@taowen
Merge pull request json-iterator#425 from liggitt/default-max-depth
81e656a 
Revert "Merge pull request json-iterator#418 from bbrks/configurable_maxDepth"
@saurav-malani
Copy link

@liggitt I am unable to understand why you guys reverted the changes of this PR.

Given that you guys were making it configurable with a default value of 10k, how is this going to expose the caller to stack overflow issue, unless they have explicitly configured it to some value greater than 10k?

NOTE: The reason I am raising this point is, we at rudderstack are using this library and time and again it happens with us that we try decoding some payload that we received & end up getting OOM killed even though we have a limit of the payload size. But, since depth of the payload also has a role in it, which we can't control. We kind of feel helpless in avoiding the OOM kill issue. If I am unaware of any existing way to avoid this issue, then please do let me know. It would be a great help.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bbrks @liggitt @saurav-malani @taowen