Introduce CircuitBreaker in the Parser

elastic · Sep 21, 2018 · ff67d02 · ff67d02
1 parent 782a71d
commit ff67d02
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 20 deletions.
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java
@@ -63,7 +63,7 @@ public LogicalPlan createStatement(String sql, List<SqlTypedParamValue> params)
         if (log.isDebugEnabled()) {
             log.debug("Parsing as statement: {}", sql);
         }
-        return invokeParser("statement", sql, params, SqlBaseParser::singleStatement, AstBuilder::plan);
+        return invokeParser(sql, params, SqlBaseParser::singleStatement, AstBuilder::plan);
     }
 
     /**
@@ -81,11 +81,10 @@ public Expression createExpression(String expression, List<SqlTypedParamValue> p
             log.debug("Parsing as expression: {}", expression);
         }
 
-        return invokeParser("expression", expression, params, SqlBaseParser::singleExpression, AstBuilder::expression);
+        return invokeParser(expression, params, SqlBaseParser::singleExpression, AstBuilder::expression);
     }
 
-    private <T> T invokeParser(String name,
-                               String sql,
+    private <T> T invokeParser(String sql,
                                List<SqlTypedParamValue> params, Function<SqlBaseParser,
                                ParserRuleContext> parseFunction,
                                BiFunction<AstBuilder, ParserRuleContext, T> visitor) {
@@ -100,6 +99,7 @@ private <T> T invokeParser(String name,
         CommonTokenStream tokenStream = new CommonTokenStream(tokenSource);
         SqlBaseParser parser = new SqlBaseParser(tokenStream);
 
+        parser.addParseListener(new CircuitBreakerProcessor());
         parser.addParseListener(new PostProcessor(Arrays.asList(parser.getRuleNames())));
 
         parser.removeErrorListeners();
@@ -126,11 +126,7 @@ private <T> T invokeParser(String name,
             log.info("Parse tree {} " + tree.toStringTree());
         }
 
-        try {
-            return visitor.apply(new AstBuilder(paramTokens), tree);
-        } catch (StackOverflowError e) {
-            throw new ParsingException("{} is too large to parse (causes stack overflow)", name);
-        }
+        return visitor.apply(new AstBuilder(paramTokens), tree);
     }
 
     private static void debug(SqlBaseParser parser) {
@@ -162,7 +158,7 @@ private class PostProcessor extends SqlBaseBaseListener {
         public void exitBackQuotedIdentifier(SqlBaseParser.BackQuotedIdentifierContext context) {
             Token token = context.BACKQUOTED_IDENTIFIER().getSymbol();
             throw new ParsingException(
-                    "backquoted indetifiers not supported; please use double quotes instead",
+                    "backquoted identifiers not supported; please use double quotes instead",
                     null,
                     token.getLine(),
                     token.getCharPositionInLine());
@@ -213,6 +209,24 @@ public void exitNonReserved(SqlBaseParser.NonReservedContext context) {
         }
     }
 
+    /**
+     * Used to catch large expressions that can lead to stack overflows
+     */
+    private class CircuitBreakerProcessor extends SqlBaseBaseListener {
+
+        private static final short MAX_BOOLEAN_ELEMENTS = 1000;
+        private short countElementsInBooleanExpressions = 0;
+
+        @Override
+        public void enterLogicalBinary(SqlBaseParser.LogicalBinaryContext ctx) {
+            if (++countElementsInBooleanExpressions == MAX_BOOLEAN_ELEMENTS) {
+                throw new ParsingException("boolean expression is too large to parse, (exceeds {} elements)",
+                    MAX_BOOLEAN_ELEMENTS);
+            }
+            super.enterLogicalBinary(ctx);
+        }
+    }
+
     private static final BaseErrorListener ERROR_LISTENER = new BaseErrorListener() {
         @Override
         public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line,

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/QuotingTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/QuotingTests.java
@@ -70,7 +70,7 @@ public void testBackQuotedAttribute() {
         String name = "@timestamp";
         ParsingException ex = expectThrows(ParsingException.class, () ->
             new SqlParser().createExpression(quote + name + quote));
-        assertThat(ex.getMessage(), equalTo("line 1:1: backquoted indetifiers not supported; please use double quotes instead"));
+        assertThat(ex.getMessage(), equalTo("line 1:1: backquoted identifiers not supported; please use double quotes instead"));
     }
 
     public void testQuotedAttributeAndQualifier() {
@@ -92,7 +92,7 @@ public void testBackQuotedAttributeAndQualifier() {
         String name = "@timestamp";
         ParsingException ex = expectThrows(ParsingException.class, () ->
             new SqlParser().createExpression(quote + qualifier + quote + "." + quote + name + quote));
-        assertThat(ex.getMessage(), equalTo("line 1:1: backquoted indetifiers not supported; please use double quotes instead"));
+        assertThat(ex.getMessage(), equalTo("line 1:1: backquoted identifiers not supported; please use double quotes instead"));
     }
 
     public void testGreedyQuoting() {

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java
@@ -138,16 +138,14 @@ public void testMultiMatchQuery() {
         assertThat(mmqp.optionMap(), hasEntry("fuzzy_rewrite", "scoring_boolean"));
     }
 
-    public void testStackOverflowStatement() {
-        ParsingException e = expectThrows(ParsingException.class, () ->
-            parseStatement("SELECT " + Joiner.on(" OR ").join(nCopies(2000, "a = b"))));
-        assertEquals("statement is too large to parse (causes stack overflow)", e.getErrorMessage());
-    }
+    public void testLimitToPreventStackOverflowFromLargeBooleanExpression() {
+        // 1000 elements is ok
+        new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(1000, "a = b")));
 
-    public void testStackOverflowExpression() {
+        // 1001 elements parser's "circuit breaker" is triggered
         ParsingException e = expectThrows(ParsingException.class, () ->
-            new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(2000, "a = b"))));
-        assertEquals("expression is too large to parse (causes stack overflow)", e.getErrorMessage());
+            new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(1001, "a = b"))));
+        assertEquals("boolean expression is too large to parse, (exceeds 1000 elements)", e.getErrorMessage());
     }
 
     private LogicalPlan parseStatement(String sql) {