Skip to content

Improve Plist and JSON Encoder/Decoder resilience to errors thrown during coding #12969

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

Closed
wants to merge 2 commits into from

Conversation

zintus
Copy link

@zintus zintus commented Nov 16, 2017

_JSONDecoder internal state could be corrupted by trying to decode failing nested object. Fix this by ensuring storage stack pop after error throw.

Resolves SR-6408.

Copy link
Contributor

@itaiferber itaiferber left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! SR-6408 is related to SR-6078 — I've been meaning to put together the comprehensive fixes here but have not yet had the time. If we're going to fix this, do you mind please also addressing SR-6078 and applying these fixes to PropertyListEncoder/PropertyListDecoder as well?

Also, we'd want to introduce unit tests to ensure that this does what we think it does, and doesn't regress in the future.

…ntainer stack when encoding fails with error
@zintus zintus changed the title Pop _JSONDecoder storage stack when throwing errors Improve Plist and JSON Encoder/Decoder resilience to errors thrown during coding Nov 17, 2017
@zintus
Copy link
Author

zintus commented Nov 17, 2017

Now SR-6078 should be fixed too.

I didn't invest much time in setting up build/test environment, should work on it more

@CodaFi
Copy link
Contributor

CodaFi commented Nov 14, 2019

Looks like this was covered by #13879. If I'm wrong about that, please reopen this and rebase it to fix the merge conflicts.

@CodaFi CodaFi closed this Nov 14, 2019
@zintus
Copy link
Author

zintus commented Nov 19, 2019

Should be fixed by #13879, thanks for looking at it.

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