honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Marios Trivyzas 2018-11-07 12:08:32 +01:00 committed by GitHub
parent 9f3effd690
commit 42dcdd0aa8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 76 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java
View File

@ -40,6 +40,7 @@ import java.util.function.BiFunction;
import java.util.function.Function; import java.util.function.Function;
import static java.lang.String.format; import static java.lang.String.format;
import static org.elasticsearch.xpack.sql.parser.AbstractBuilder.source;
public class SqlParser { public class SqlParser {
@ -215,7 +216,7 @@ public class SqlParser {
*/ */
private class CircuitBreakerListener extends SqlBaseBaseListener { 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. // Keep current depth for every rule visited.
// The totalDepth alone cannot be used as expressions like: e1 OR e2 OR e3 OR ... // The totalDepth alone cannot be used as expressions like: e1 OR e2 OR e3 OR ...
@ -225,16 +226,24 @@ public class SqlParser {
@Override @Override
public void enterEveryRule(ParserRuleContext ctx) { public void enterEveryRule(ParserRuleContext ctx) {
short currentDepth = depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 1, (short) 1); if (ctx.getClass() != SqlBaseParser.UnquoteIdentifierContext.class &&
if (currentDepth > MAX_RULE_DEPTH) { ctx.getClass() != SqlBaseParser.QuoteIdentifierContext.class &&
throw new ParsingException("expression is too large to parse, (tree's depth exceeds {})", MAX_RULE_DEPTH); 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); super.enterEveryRule(ctx);
} }
@Override @Override
public void exitEveryRule(ParserRuleContext ctx) { 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); super.exitEveryRule(ctx);
} }
} }

90
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java
View File

@ -138,71 +138,104 @@ public class SqlParserTests extends ESTestCase {
assertThat(mmqp.optionMap(), hasEntry("fuzzy_rewrite", "scoring_boolean")); 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() { public void testLimitToPreventStackOverflowFromLargeUnaryBooleanExpression() {
// Create expression in the form of NOT(NOT(NOT ... (b) ...) // Create expression in the form of NOT(NOT(NOT ... (b) ...)
// 40 elements is ok // 99 elements is ok
new SqlParser().createExpression( 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 // 100 elements parser's "circuit breaker" is triggered
ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createExpression( ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createExpression(
Joiner.on("").join(nCopies(100, "NOT(")).concat("b").concat(Joiner.on("").join(nCopies(100, ")"))))); 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() { public void testLimitToPreventStackOverflowFromLargeBinaryBooleanExpression() {
// Create expression in the form of a = b OR a = b OR ... a = b // Create expression in the form of a = b OR a = b OR ... a = b
// 50 elements is ok // 100 elements is ok
new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(50, "a = b"))); 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, () -> ParsingException e = expectThrows(ParsingException.class, () ->
new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(100, "a = b")))); new SqlParser().createExpression(Joiner.on(" OR ").join(nCopies(101, "a = b"))));
assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); assertEquals("line 1:902: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)",
e.getMessage());
} }
public void testLimitToPreventStackOverflowFromLargeUnaryArithmeticExpression() { public void testLimitToPreventStackOverflowFromLargeUnaryArithmeticExpression() {
// Create expression in the form of abs(abs(abs ... (i) ...) // Create expression in the form of abs(abs(abs ... (i) ...)
// 50 elements is ok // 199 elements is ok
new SqlParser().createExpression( 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( ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createExpression(
Joiner.on("").join(nCopies(101, "abs(")).concat("i").concat(Joiner.on("").join(nCopies(101, ")"))))); Joiner.on("").join(nCopies(200, "abs(")).concat("i").concat(Joiner.on("").join(nCopies(200, ")")))));
assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); assertEquals("line 1:802: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)",
e.getMessage());
} }
public void testLimitToPreventStackOverflowFromLargeBinaryArithmeticExpression() { public void testLimitToPreventStackOverflowFromLargeBinaryArithmeticExpression() {
// Create expression in the form of a + a + a + ... + a // Create expression in the form of a + a + a + ... + a
// 100 elements is ok // 200 elements is ok
new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(100, "a"))); 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, () -> ParsingException e = expectThrows(ParsingException.class, () ->
new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(101, "a")))); new SqlParser().createExpression(Joiner.on(" + ").join(nCopies(201, "a"))));
assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); assertEquals("line 1:802: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)",
e.getMessage());
} }
public void testLimitToPreventStackOverflowFromLargeSubselectTree() { public void testLimitToPreventStackOverflowFromLargeSubselectTree() {
// Test with queries in the form of `SELECT * FROM (SELECT * FROM (... t) ...) // Test with queries in the form of `SELECT * FROM (SELECT * FROM (... t) ...)
// 100 elements is ok // 200 elements is ok
new SqlParser().createStatement( new SqlParser().createStatement(
Joiner.on(" (").join(nCopies(100, "SELECT * FROM")) Joiner.on(" (").join(nCopies(200, "SELECT * FROM"))
.concat("t") .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( 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("t")
.concat(Joiner.on("").join(nCopies(100, ")"))))); .concat(Joiner.on("").join(nCopies(200, ")")))));
assertEquals("expression is too large to parse, (tree's depth exceeds 100)", e.getErrorMessage()); assertEquals("line 1:3002: SQL statement too large; halt parsing to prevent memory errors (stopped at depth 200)",
e.getMessage());
} }
public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() { public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() {
@ -210,14 +243,15 @@ public class SqlParserTests extends ESTestCase {
new SqlParser().createStatement( new SqlParser().createStatement(
Joiner.on(" (").join(nCopies(20, "SELECT ")). 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, ")")))); .concat("t").concat(Joiner.on("").join(nCopies(19, ")"))));
ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createStatement( ParsingException e = expectThrows(ParsingException.class, () -> new SqlParser().createStatement(
Joiner.on(" (").join(nCopies(20, "SELECT ")). 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, ")"))))); .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) { private LogicalPlan parseStatement(String sql) {