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

common/json.c: Check that JSMN result is well-formed. #3761

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

ZmnSCPxj
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj commented Jun 8, 2020

@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Jun 8, 2020

Probably deserves some kind of test though.

@ZmnSCPxj
Copy link
Collaborator Author

Modified the common/test/run-json.c, should be OK for merging now.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Nice fix, just three tiny comments/clarifications needed.

Am I correct in assuming this is O(n) despite the recursive calls, since we advance the pointers by the parsed amounts in the recursive calls?

common/json.c Outdated Show resolved Hide resolved
common/json.c Outdated Show resolved Hide resolved
common/json.c Outdated Show resolved Hide resolved
@ZmnSCPxj
Copy link
Collaborator Author

Am I correct in assuming this is O(n) despite the recursive calls, since we advance the pointers by the parsed amounts in the recursive calls?

Yes, it should be.

@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Jun 15, 2020

Am I correct in assuming this is O(n) despite the recursive calls, since we advance the pointers by the parsed amounts in the recursive calls?

Yes, it should be.

Ah, no, no. Deeply-nested objects/arrays would trigger the function being called over sub-ranges of an earlier call, so it does not achieve O(nd) where d is the nestedness of objects. Personally I think correctness is more important, we can always rewrite it faster later.

@ZmnSCPxj
Copy link
Collaborator Author

Sorry, completely rewrote the code to be O(n) on the number of tokens validated, and requiring O(d) stack space (d being nestedness of objects/arrays in the JSON input).

@ZmnSCPxj
Copy link
Collaborator Author

Renamed the validation function to validate_jsmn_parse_output.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack f3438c2

@rustyrussell
Copy link
Contributor

Nice work. Yes, I hit a similar issue previously, but that one was fixed upstream. Good to validate generally for this reason.

@rustyrussell rustyrussell merged commit e8936f9 into ElementsProject:master Jun 19, 2020
@ZmnSCPxj ZmnSCPxj deleted the well-formed-jsmn branch June 19, 2020 03:52
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