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

Limit nesting of json commands #2326

Closed
wants to merge 5 commits into from
Closed

Limit nesting of json commands #2326

wants to merge 5 commits into from

Conversation

HowardHinnant
Copy link
Contributor

No description provided.

@ripplelabs-jenkins
Copy link
Collaborator

ripplelabs-jenkins commented Jan 9, 2018 •
edited
Loading

Jenkins Build Summary

Built from this commit

Built at 20180116 - 21:32:39

Test Results

Build Type Result Status
clang.debug.unity 975 cases, 0 failed, t: 387s PASS ✅
coverage 975 cases, 0 failed, t: 613s PASS ✅
gcc.debug.unity 975 cases, 0 failed, t: 435s PASS ✅
clang.debug.nounity 973 cases, 0 failed, t: 607s PASS ✅
clang.release.unity 974 cases, 0 failed, t: 467s PASS ✅
gcc.debug.nounity 973 cases, 0 failed, t: 744s PASS ✅
gcc.release.unity 974 cases, 0 failed, t: 510s PASS ✅

nbougalis
nbougalis approved these changes Jan 10, 2018
View reviewed changes
Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Looks good.

Only comment is maybe set the maximum nesting depth as a constexpr at the top of the file.

scottschurr
scottschurr reviewed Jan 10, 2018
View reviewed changes
src/ripple/json/impl/json_reader.cpp Outdated
@@ -29,6 +29,8 @@ namespace Json
// Implementation of class Reader
// ////////////////////////////////

constexpr unsigned nest_limit = 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a point of reference, the code that converts JSON to STObjects/STArrays limits recursion (objects containing objects or arrays and on down) to 64: https:/ripple/rippled/blob/develop/src/ripple/protocol/impl/STParsedJSON.cpp#L693. Not suggesting any change here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might worth making a public static member that can be referenced externally, like a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such a unit test would get pretty ugly. Not sure it is worth it.

miguelportilla
miguelportilla reviewed Jan 12, 2018
View reviewed changes
src/ripple/json/impl/json_reader.cpp Outdated
@@ -141,7 +143,7 @@ Reader::readValue(unsigned depth)
{
Token token;
skipCommentTokens ( token );
if (++depth >= 1000)
if (++depth >= nest_limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be if (depth > nest_limit) ?
depth is already incremented by the readObject and readArray functions before readValue is called. Incrementing here seems to incorrectly double how many nests we think there are. With following json {"obj":{"obj":{"obj":{"obj":{"obj":{"obj":{"obj":{"obj":{"obj":{"obj":{}}}}}}}}}}} depth reaches a value of 20 instead of 10.

Copy link
Contributor Author

@HowardHinnant HowardHinnant Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

<slaps forehead> Thanks! Fixing...

@miguelportilla
Copy link
Contributor

@HowardHinnant I extended json_value_test.cpp to include a unit test for these changes. Please feel free to cherrypick (Disclaimer- tested only under Windows).

https:/miguelportilla/rippled/commits/nest_test

@HowardHinnant
Copy link
Contributor Author

@miguelportilla Nice! Thanks!

miguelportilla
miguelportilla approved these changes Jan 16, 2018
View reviewed changes
Copy link
Contributor

@miguelportilla miguelportilla left a comment

Choose a reason for hiding this comment

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

👍

@HowardHinnant HowardHinnant added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 16, 2018
@HowardHinnant
Copy link
Contributor Author

Rebased to 0.90.0-b3

@scottschurr
Copy link
Collaborator

Incorporated in 0.90.0-b4 as 0ec66b3.

@HowardHinnant HowardHinnant deleted the limit_nest branch December 6, 2018 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottschurr scottschurr scottschurr left review comments

@miguelportilla miguelportilla miguelportilla approved these changes

@nbougalis nbougalis nbougalis approved these changes

Assignees

@miguelportilla miguelportilla

@nbougalis nbougalis

Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HowardHinnant @ripplelabs-jenkins @miguelportilla @scottschurr @nbougalis