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

Fortify lottie parser #478

Merged
merged 2 commits into from Jun 3, 2021
Merged

Fortify lottie parser #478

merged 2 commits into from Jun 3, 2021

Conversation

mymedia2
Copy link
Contributor

Avoid calling the abort() function on invalid input data.

@smohantty
Copy link
Contributor

@mymedia2 ,
Yes we need to find a way to gracefully abort parsing and return a null model if the json format is not what we expect.
As its a look ahead parser , simply returning from the function when the type is not what we expect will not do the job as it will still go ahead and parse the next object and will face the same issue.

One possible solution is to replace RAPIDJSON_ASSERT() with some function which will put the parser in the error state.
and check for error state in all the function like GetString() , NextArrayValue(), NextObjectKey() etc to return appropriately if parser in error state . Then in the end of parsing check if the state is Error then we delete the model and return a null model to notify that the parsing has failed.

if you have some other idea please share.

@smohantty
Copy link
Contributor

@mymedia2 , something similar to this patch #481 .

As at the end of parsing we check for the error_state of the parser and returns null composition if state is in error state.
I hope you are trying to achieve something similar to this.

@mymedia2
Copy link
Contributor Author

Well, I patched the parser in a few places, and more consistently solution is needed that cares of parser state. Another general approach would be to throw exceptions in case of invalid input, but they are disabled here, and so we have to unwind call stack by hand or do something like that. 🤷

@mymedia2
Copy link
Contributor Author

@smohantty What do you make of replacing RAPIDJSON_ASSERT() macro with RLOTTIE_ASSERT() and leaving all the rest as is in the current shape? So whoever may receive malformed Lottie will redefine this macro as (void) and their app will not be aborted. The idea is to distinguish prerequisite failures inside RapidJSON and invalid input for rLottie.

@smohantty
Copy link
Contributor

@mymedia2 ,
have removed the Assert() in all places . could you please update the patch to keep the null pointer check on mExtra variable.

@mymedia2
Copy link
Contributor Author

mymedia2 commented Jun 1, 2021

@smohantty ok, rebased on master

@smohantty smohantty merged commit c0e16e5 into Samsung:master Jun 3, 2021
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.

None yet

2 participants