From 42dcdd0aa8b887097c1e1626da3202492a6e350c Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 7 Nov 2018 12:08:32 +0100 Subject: [PATCH] SQL: Improve CircuitBreaker logic for SqlParser (#35300) Grammar's identifiers can be completely skipped from counting depths as they just add another level to the tree and they are always children of some other expression which gets counted. Increased maximum depth from 100 to 200. After testing on production configuration with -Xss1m, depths of at least 250 can be used, so being conservative we put the limit lower. Fixes: #35299 --- .../xpack/sql/parser/SqlParser.java | 19 ++-- .../xpack/sql/parser/SqlParserTests.java | 90 +++++++++++++------ 2 files changed, 76 insertions(+), 33 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 index 2aee552907b..fcc0d50bd05 100644 --- 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 @@ -40,6 +40,7 @@ import java.util.function.BiFunction; import java.util.function.Function; import static java.lang.String.format; +import static org.elasticsearch.xpack.sql.parser.AbstractBuilder.source; public class SqlParser { @@ -215,7 +216,7 @@ public class SqlParser { */ private class CircuitBreakerListener extends SqlBaseBaseListener { - private static final short MAX_RULE_DEPTH = 100; + private static final short MAX_RULE_DEPTH = 200; // Keep current depth for every rule visited. // The totalDepth alone cannot be used as expressions like: e1 OR e2 OR e3 OR ... @@ -225,16 +226,24 @@ public class SqlParser { @Override public void enterEveryRule(ParserRuleContext ctx) { - short currentDepth = depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 1, (short) 1); - if (currentDepth > MAX_RULE_DEPTH) { - throw new ParsingException("expression is too large to parse, (tree's depth exceeds {})", MAX_RULE_DEPTH); + if (ctx.getClass() != SqlBaseParser.UnquoteIdentifierContext.class && + ctx.getClass() != SqlBaseParser.QuoteIdentifierContext.class && + ctx.getClass() != SqlBaseParser.BackQuotedIdentifierContext.class) { + int currentDepth = depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 1, (short) 1); + if (currentDepth > MAX_RULE_DEPTH) { + throw new ParsingException(source(ctx), "SQL statement too large; " + + "halt parsing to prevent memory errors (stopped at depth {})", MAX_RULE_DEPTH); + } } super.enterEveryRule(ctx); } @Override public void exitEveryRule(ParserRuleContext ctx) { - depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 0, (short) -1); + // Avoid having negative numbers + if (depthCounts.containsKey(ctx.getClass().getSimpleName())) { + depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 0, (short) -1); + } super.exitEveryRule(ctx); } } 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 index aab25349a1d..2794481dc80 100644 --- 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,71 +138,104 @@ public class SqlParserTests extends ESTestCase { assertThat(mmqp.optionMap(), hasEntry("fuzzy_rewrite", "scoring_boolean")); } + public void testLimitToPreventStackOverflowFromLongListOfQuotedIdentifiers() { + // Create expression in the form of "t"."field","t"."field", ... + + // 200 elements is ok + new SqlParser().createStatement("SELECT " + + Joiner.on(",").join(nCopies(200, "\"t\".\"field\"")) + " FROM t"); + + // 201 elements parser's "circuit breaker" is triggered + ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createStatement("SELECT " + + Joiner.on(",").join(nCopies(201, "\"t\".\"field\"")) + " FROM t")); + assertEquals("line 1:2409: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); + } + + public void testLimitToPreventStackOverflowFromLongListOfUnQuotedIdentifiers() { + // Create expression in the form of t.field,t.field, ... + + // 250 elements is ok + new SqlParser().createStatement("SELECT " + + Joiner.on(",").join(nCopies(200, "t.field")) + " FROM t"); + + // 251 elements parser's "circuit breaker" is triggered + ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createStatement("SELECT " + + Joiner.on(",").join(nCopies(201, "t.field")) + " FROM t")); + assertEquals("line 1:1609: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); + } + public void testLimitToPreventStackOverflowFromLargeUnaryBooleanExpression() { // Create expression in the form of NOT(NOT(NOT ... (b) ...) - // 40 elements is ok + // 99 elements is ok new SqlParser().createExpression( - Joiner.on("").join(nCopies(40, "NOT(")).concat("b").concat(Joiner.on("").join(nCopies(40, ")")))); + Joiner.on("").join(nCopies(99, "NOT(")).concat("b").concat(Joiner.on("").join(nCopies(99, ")")))); // 100 elements parser's "circuit breaker" is triggered ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createExpression( Joiner.on("").join(nCopies(100, "NOT(")).concat("b").concat(Joiner.on("").join(nCopies(100, ")"))))); - assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); + assertEquals("line 1:402: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); } public void testLimitToPreventStackOverflowFromLargeBinaryBooleanExpression() { // Create expression in the form of a = b OR a = b OR ... a = b - // 50 elements is ok - new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(50, "a = b"))); + // 100 elements is ok + new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(100, "a = b"))); - // 100 elements parser's "circuit breaker" is triggered + // 101 elements parser's "circuit breaker" is triggered ParsingException e = expectThrows(ParsingException.class, () -> - new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(100, "a = b")))); - assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); + new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(101, "a = b")))); + assertEquals("line 1:902: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); } public void testLimitToPreventStackOverflowFromLargeUnaryArithmeticExpression() { // Create expression in the form of abs(abs(abs ... (i) ...) - // 50 elements is ok + // 199 elements is ok new SqlParser().createExpression( - Joiner.on("").join(nCopies(50, "abs(")).concat("i").concat(Joiner.on("").join(nCopies(50, ")")))); + Joiner.on("").join(nCopies(199, "abs(")).concat("i").concat(Joiner.on("").join(nCopies(199, ")")))); - // 101 elements parser's "circuit breaker" is triggered + // 200 elements parser's "circuit breaker" is triggered ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createExpression( - Joiner.on("").join(nCopies(101, "abs(")).concat("i").concat(Joiner.on("").join(nCopies(101, ")"))))); - assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); + Joiner.on("").join(nCopies(200, "abs(")).concat("i").concat(Joiner.on("").join(nCopies(200, ")"))))); + assertEquals("line 1:802: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); } public void testLimitToPreventStackOverflowFromLargeBinaryArithmeticExpression() { // Create expression in the form of a + a + a + ... + a - // 100 elements is ok - new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(100, "a"))); + // 200 elements is ok + new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(200, "a"))); - // 101 elements parser's "circuit breaker" is triggered + // 201 elements parser's "circuit breaker" is triggered ParsingException e = expectThrows(ParsingException.class, () -> - new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(101, "a")))); - assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); + new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(201, "a")))); + assertEquals("line 1:802: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); } public void testLimitToPreventStackOverflowFromLargeSubselectTree() { // Test with queries in the form of `SELECT * FROM (SELECT * FROM (... t) ...) - // 100 elements is ok + // 200 elements is ok new SqlParser().createStatement( - Joiner.on(" (").join(nCopies(100, "SELECT * FROM")) + Joiner.on(" (").join(nCopies(200, "SELECT * FROM")) .concat("t") - .concat(Joiner.on("").join(nCopies(99, ")")))); + .concat(Joiner.on("").join(nCopies(199, ")")))); - // 101 elements parser's "circuit breaker" is triggered + // 201 elements parser's "circuit breaker" is triggered ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createStatement( - Joiner.on(" (").join(nCopies(101, "SELECT * FROM")) + Joiner.on(" (").join(nCopies(201, "SELECT * FROM")) .concat("t") - .concat(Joiner.on("").join(nCopies(100, ")"))))); - assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); + .concat(Joiner.on("").join(nCopies(200, ")"))))); + assertEquals("line 1:3002: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); } public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() { @@ -210,14 +243,15 @@ public class SqlParserTests extends ESTestCase { new SqlParser().createStatement( Joiner.on(" (").join(nCopies(20, "SELECT ")). - concat(Joiner.on(" OR ").join(nCopies(50, "true"))).concat(" FROM") + concat(Joiner.on(" OR ").join(nCopies(180, "true"))).concat(" FROM") .concat("t").concat(Joiner.on("").join(nCopies(19, ")")))); ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createStatement( Joiner.on(" (").join(nCopies(20, "SELECT ")). - concat(Joiner.on(" OR ").join(nCopies(100, "true"))).concat(" FROM") + concat(Joiner.on(" OR ").join(nCopies(190, "true"))).concat(" FROM") .concat("t").concat(Joiner.on("").join(nCopies(19, ")"))))); - assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); + assertEquals("line 1:1628: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)", + e.getMessage()); } private LogicalPlan parseStatement(String sql) {