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

Fix MaxDepth problems introduced between 12.0.3 and 13.0.1 #2504

Closed
wants to merge 1 commit into from

Conversation

mcon
Copy link

@mcon mcon commented Mar 25, 2021

Fixes #2501 - have verified that the example provided in F# fails with no change and is resolved with the change.

Between 12.0.3 and 13.0.1 looks like _maxDepth = 64; was set in the JsonReader constructor - any inheritors which use this default constructor end up with a max depth of 64 as a result - from the existing tests on max depth, it seems that this setting is not actually required in order to ensure the max depth is respected.

I've also updated a few doc comments which erroneously claim the max depth as 128.

Update: as much as the initial fix (#2462) prevents a DOS attack, it does create other problems, such as 2501 - max depth is a bit tricky throughout the library, one option would be to make it mandatory on the JsonReader perhaps.

@Smaug123
Copy link

Suggest adding MaxDepth tests to verify that you can extend MaxDepth to be bigger than the default?

@JamesNK
Copy link
Owner

JamesNK commented Mar 27, 2021

Thanks for looking at this.

I've fixed this in a different way here #2505

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.

MaxDepth default value of null or 100 is not respected
3 participants