From d93d08d6dd0b377ae2f708f97587732c75d77115 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 7 Feb 2019 16:31:12 +0200 Subject: [PATCH 1/2] SQL: Relax StatckOverflow circuit breaker for constants Constant numbers (of any form: integers, decimals, negatives, scientific) and strings shouldn't increase the depth counters as they don't contribute to the increment of the stack depth. Fixes: #38571 --- .../xpack/sql/parser/SqlParser.java | 7 +++--- .../xpack/sql/parser/SqlParserTests.java | 24 ++++++++++++++++--- 2 files changed, 24 insertions(+), 7 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 0bc02c1ba6f27..9920293794ea0 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 @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.parser; import com.carrotsearch.hppc.ObjectShortHashMap; - import org.antlr.v4.runtime.BaseErrorListener; import org.antlr.v4.runtime.CharStream; import org.antlr.v4.runtime.CommonToken; @@ -37,8 +36,6 @@ import org.elasticsearch.xpack.sql.parser.SqlBaseParser.StatementContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.StatementDefaultContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.UnquoteIdentifierContext; -import org.elasticsearch.xpack.sql.parser.SqlBaseParser.ValueExpressionContext; -import org.elasticsearch.xpack.sql.parser.SqlBaseParser.ValueExpressionDefaultContext; import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; @@ -242,7 +239,6 @@ static class CircuitBreakerListener extends SqlBaseBaseListener { ENTER_EXIT_RULE_MAPPING.put(StatementDefaultContext.class.getSimpleName(), StatementContext.class.getSimpleName()); ENTER_EXIT_RULE_MAPPING.put(QueryPrimaryDefaultContext.class.getSimpleName(), QueryTermContext.class.getSimpleName()); ENTER_EXIT_RULE_MAPPING.put(BooleanDefaultContext.class.getSimpleName(), BooleanExpressionContext.class.getSimpleName()); - ENTER_EXIT_RULE_MAPPING.put(ValueExpressionDefaultContext.class.getSimpleName(), ValueExpressionContext.class.getSimpleName()); } private boolean insideIn = false; @@ -265,6 +261,9 @@ public void enterEveryRule(ParserRuleContext ctx) { if (ctx.getClass() != UnquoteIdentifierContext.class && ctx.getClass() != QuoteIdentifierContext.class && ctx.getClass() != BackQuotedIdentifierContext.class && + ctx.getClass() != SqlBaseParser.ConstantContext.class && + ctx.getClass() != SqlBaseParser.NumberContext.class && + ctx.getClass() != SqlBaseParser.ValueExpressionContext.class && (insideIn == false || ctx.getClass() != PrimaryExpressionContext.class)) { int currentDepth = depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 1, (short) 1); 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 dd44a8e464ae4..46adaebb8b6eb 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 @@ -296,7 +296,16 @@ public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() { public void testLimitStackOverflowForInAndLiteralsIsNotApplied() { int noChildren = 100_000; LogicalPlan plan = parseStatement("SELECT * FROM t WHERE a IN(" + - Joiner.on(",").join(nCopies(noChildren, "a + b")) + ")"); + Joiner.on(",").join(nCopies(noChildren, "a + 10")) + "," + + Joiner.on(",").join(nCopies(noChildren, "-(-a - 10)")) + "," + + Joiner.on(",").join(nCopies(noChildren, "20")) + "," + + Joiner.on(",").join(nCopies(noChildren, "-20")) + "," + + Joiner.on(",").join(nCopies(noChildren, "20.1234")) + "," + + Joiner.on(",").join(nCopies(noChildren, "-20.4321")) + "," + + Joiner.on(",").join(nCopies(noChildren, "1.1234E56")) + "," + + Joiner.on(",").join(nCopies(noChildren, "-1.4321E-65")) + "," + + Joiner.on(",").join(nCopies(noChildren, "'foo'")) + "," + + Joiner.on(",").join(nCopies(noChildren, "'bar'")) + ")"); assertEquals(With.class, plan.getClass()); assertEquals(Project.class, ((With) plan).child().getClass()); @@ -305,8 +314,17 @@ public void testLimitStackOverflowForInAndLiteralsIsNotApplied() { assertEquals(In.class, filter.condition().getClass()); In in = (In) filter.condition(); assertEquals("?a", in.value().toString()); - assertEquals(noChildren, in.list().size()); - assertThat(in.list().get(0).toString(), startsWith("Add[?a,?b]")); + assertEquals(noChildren * 2 + 8, in.list().size()); + assertThat(in.list().get(0).toString(), startsWith("Add[?a,10]#")); + assertThat(in.list().get(noChildren).toString(), startsWith("Neg[Sub[Neg[?a]#")); + assertEquals("20", in.list().get(noChildren * 2).toString()); + assertEquals("-20", in.list().get(noChildren * 2 + 1).toString()); + assertEquals("20.1234", in.list().get(noChildren * 2 + 2).toString()); + assertEquals("-20.4321", in.list().get(noChildren * 2 + 3).toString()); + assertEquals("1.1234E56", in.list().get(noChildren * 2 + 4).toString()); + assertEquals("-1.4321E-65", in.list().get(noChildren * 2 + 5).toString()); + assertEquals("'foo'=foo", in.list().get(noChildren * 2 + 6).toString()); + assertEquals("'bar'=bar", in.list().get(noChildren * 2 + 7).toString()); } public void testDecrementOfDepthCounter() { From 9fc53a9ae8d31269be6598075a1b551f167eb391 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 8 Feb 2019 23:25:39 +0200 Subject: [PATCH 2/2] Reduce elements because of memory issues in CI testing --- .../java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 46adaebb8b6eb..8b275468f482a 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 @@ -294,7 +294,7 @@ public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() { } public void testLimitStackOverflowForInAndLiteralsIsNotApplied() { - int noChildren = 100_000; + int noChildren = 10_000; LogicalPlan plan = parseStatement("SELECT * FROM t WHERE a IN(" + Joiner.on(",").join(nCopies(noChildren, "a + 10")) + "," + Joiner.on(",").join(nCopies(noChildren, "-(-a - 10)")) + "," +