Rhino 1.7.7.2 can cause heap space (OutOfMemoryError) due to infinite loop on parsing

[SabineJung]

With Rhino 1.7.7.2 parsing the following syntactically false JavaScript function call causes a OutOfMemoryError, due to an infinite loop.

String test = "callMyFunction('$Request.get('rq_myVar')', getElement('413C4EC81968F867E67388E41139A9797001ACC7'), this);";

CompilerEnvirons env = new CompilerEnvirons();
env.setXmlAvailable(false);
env.setIdeMode(true);
env.setRecoverFromErrors(true);
env.setErrorReporter(new ErrorReporter()
{
	@Override
	public void warning(String msg, String name, int line, String str, int col)
	{
		System.out.println(msg + ", " + line + ", " + col + ".");
	}

	@Override
	public EvaluatorException runtimeError(String msg, String name, int line, String str, int col)
	{
		System.out.println(msg + ", " + line + ", " + col + ".");
		return new EvaluatorException(msg, name, line, str, col);
	}

	@Override
	public void error(String msg, String name, int line, String str, int col)
	{
		System.out.println(msg + ", " + line + ", " + col + ".");
	}
});

Parser parser = new Parser(env);
parser.parse(test, test, 0);

Error report:
missing ) after argument list, 0, 35.
missing ; before statement, 0, 35.
missing ; before statement, 0, 37.
missing ; before statement, 0, 100.
missing ; before statement, 0, 100.
missing ; before statement, 0, 100.
missing ; before statement, 0, 100.
missing ; before statement, 0, 100.
... [infinite loop]

With Rhino 1.7.7.1 no infinite loop occurs while executing the code.
missing ) after argument list, 0, 35.
missing ; before statement, 0, 35.
missing ; before statement, 0, 37.
missing ; before statement, 0, 100.
syntax error, 0, 100.

[gbrail]

According to some work with 'git bisect,' the problem was introduced here:

318da76

It's not clear to me what changed here and why it would cause this, however, so if anyone else has an idea then we can resolve this.

[sainaen]

The minimal test source is just a right parenthesis: String test = ")";

The Parser#primaryExpr() was updated in 318da76 to return a new EmptyExpression() when it gets a right paren, to do this:

|var f = () => 10;
^
start parsing
---
var f| = () => 10;
     ^
we're in the VariableDeclaration, saw a variable name 'f'
---
var f = |() => 10;
        ^
we're in the AssignmentExpression
---
var f = (|) => 10;
         ^
saw a left parenthesis, so it's a start of ParenthesizedExpression
(we're in `parenExpr()` now), parse inside with `expr()`

before 318da76 after descending to `primaryExpr()` we'd not find anything
that we'd expect here (`var f = ()` is invalid syntax), so we'd report
an error, but now we proceed
---
var f = (|) => 10;
         ^
return EmptyExpression from `primaryExpr()`, but don't consume token
---
var f = () |=> 10;
           ^
`matchToken(Token.RP)` in `parenExpr()` to assert that parenthesized
expression is closed (this consumes current token) and avoid reporting
an error if the next token is an arrow
---
// the rest is parsing of an arrow function

Now in the case of just a ), the primaryExpr() repeatedly creates and returns an EmptyExpression (consuming memory), Rhino's Parser reports an error, but then because ErrorReporter doesn't throw on error it continues parsing without advancing the TokenStream.

I think one (hack-ish) way to solve this would be to remove the case from primaryExpr() that allows right parens without consuming them, but create an EmptyExpression right away if the next token is ) in parenExpr(), without calling expr():

diff --git a/src/org/mozilla/javascript/Parser.java b/src/org/mozilla/javascript/Parser.java
index f8b64d14..7aa5f963 100644
--- a/src/org/mozilla/javascript/Parser.java
+++ b/src/org/mozilla/javascript/Parser.java
@@ -3064,9 +3064,6 @@ public class Parser
               pos = ts.tokenBeg; end = ts.tokenEnd;
               return new KeywordLiteral(pos, end - pos, tt);
 
-          case Token.RP:
-              return new EmptyExpression();
-
           case Token.RESERVED:
               consumeToken();
               reportError("msg.reserved.id");
@@ -3099,7 +3096,7 @@ public class Parser
             Comment jsdocNode = getAndResetJsDoc();
             int lineno = ts.lineno;
             int begin = ts.tokenBeg;
-            AstNode e = expr();
+            AstNode e = (peekToken() == Token.RP ? new EmptyExpression() : expr());
             if (peekToken() == Token.FOR) {
                 return generatorExpression(e, begin);
             }

After this change all tests still pass, but it's possible that now broken cases (particularly for arrow function) are just not covered yet.

[gbrail]

After digging through the code I'm not sure that I can suggest a better fix than this one. Perhaps we will work on a version of this and then run further tests.

[gbrail]

This pull request

#334

addresses the problem by including your patch and a test case. As you say, it passes the Rhino tests, but we don't know if there are other parsing bugs triggered when error recovery is turned on.

Are you able to test this out in your environment? It'd be a big help. Thanks!

[gbrail]

We closed pull request #334 and at least this particular use case works now. I'm going to close this, but please open a new one if something else happens.