Detect infinite loop/crash #5
Conversation
ychai-coursera
force-pushed
the
ychai/fix_template_loop
branch
from
7c17b4c to
88b06dd
Compare
| @@ -22,6 +25,10 @@ | |||
| private Map<String, MixinNode> mixins = new HashMap<String, MixinNode>(); | |||
| private Map<String, Filter> filter = new HashMap<String, Filter>(); | |||
|
|
|||
| // to detect infinite loop | |||
| private Map<String, Integer> visited = new HashMap<String, Integer>(); | |||
There was a problem hiding this comment.
Are you sure this class (JadeModel) and therefore this Map will be newly instantiated every time we parse a different document?
There was a problem hiding this comment.
Could add a unit test to prove this by parsing the same document multiple times.
| @@ -22,6 +25,10 @@ | |||
| private Map<String, MixinNode> mixins = new HashMap<String, MixinNode>(); | |||
| private Map<String, Filter> filter = new HashMap<String, Filter>(); | |||
|
|
|||
| // to detect infinite loop | |||
There was a problem hiding this comment.
Could we also add a comment with the cases we've seen leading to infinite loop?
| Integer ct = visited.getOrDefault(visitId, 0) +1; | ||
| if (ct > MAX_VISITED) { | ||
| String error = "Potential infinite loop: "+visitId+" has been visited more then "+MAX_VISITED+" times!"; | ||
| throw new JadeCompilerException(node, template.getTemplateLoader(), error); |
There was a problem hiding this comment.
Can we get the Template Name somehow and return it in the Exception message?
There was a problem hiding this comment.
visitId contains file path and line number
|
Can you add a unit test validating this throws the right error upon infinite loop? Could add a couple of cases that we've identified from the jade templates we've made (Eg. crash.jade, etc) |
Currently when we have templates include each other and using
blockimproperly, there is a chance jade4j got infinite loop results in StackOverflowError, this will crash the process. This PR counts how many time each node being visited and if the count for specific node reach the threshold (currently set to 100) we fail the compile instead of crash.