avoid using recursion in for JSONML and JSONTokener #726
Conversation
it will get rid of potential stack overflow errors and will use the available application heap instead
|
Nice work! |
|
No problem. I can drop the first commit, which is related to JSONML. The commits prefixed by The last commit is trickier, as I tried to simulate what the call stack does: push local variables (at least the relevant ones) and the instruction into the stack, but it would be hard to prove that the two algorithms yield the same results. So, I see two alternatives: 1) generate a considerable amount of data in the JSON format that corresponds to the grammar that this parser obeys (the grammar should be defined formally, as in https://www.json.org/json-en.html) and add tests to reduce the likelihood of missing a case (never eliminating it, of course) or 2) assume the tests we have currently are "good enough" and, should anybody find a regression, fix the parser accordingly. What do you think? |
|
A lot of code was updated. The unit tests are good for catching regressions but for this PR I don't think that is enough. Probably needs a careful review and some additional unit testing. Otherwise, I think someone could fairly ask why not create a ParserConfiguration class similar to XML and JSONML and avoid all the refactoring. |
| JSONTokener tokener = new JSONTokener(resourceAsStream); | ||
| JSONArray json_input = new JSONArray(tokener); | ||
| assertNotNull(json_input); | ||
| fail("Excepected Exception."); |
There was a problem hiding this comment.
Because with my changes a stack overflow is not expected to happen as the algorithm to parse strings to JSON is not recursive anymore. So the test would fail 100% of the time, instead of intermittently as it does currently.
There was a problem hiding this comment.
For both of these, these tests looks like they were testing to make sure that a stack overflow occurred. So these tests "pass" if the bug isn't fixed, but will fail if the bug is fixed. So... not the best test to leave in.
That said, a better test might be to ensure that the use case that previously threw an exception no longer throws an exception? So instead of removing the tests entirely, update them to test that use case, but ensure that no exception is not thrown instead of ensuring that an exception is thrown?
(I'm not a maintainer on this project, just my two cents as someone who was watching this CVE.)
| JSONTokener tokener = new JSONTokener(resourceAsStream); | ||
| JSONObject json_input = new JSONObject(tokener); | ||
| assertNotNull(json_input); | ||
| fail("Excepected Exception."); |
There was a problem hiding this comment.
Test should not be removed
There was a problem hiding this comment.
It is not good remove tests after code changes
|
I see so many changes and few unit tests |
|
This branch has conflicts that must be resolved |
|
This branch has conflicts that must be resolved |
|
I'll send this again when I rebase and have some test coverage. |
Fixes #722