Skip to content

Commit

Permalink
SQL: Fix wrong appliance of StackOverflow limit for IN
Browse files Browse the repository at this point in the history 
Fix grammar so that each element inside the list of values for IN
is a `valueExpression` and not a more generic `expression`. Also change
some names in the grammar so that they match the primary rule name
plus "Default". This helps so that the decrement of depth counts in
the Parser's `CircuitBreakerListener` works correctly.

For the list of values for `IN` don't count the
`PrimaryExpressionContext` as this is not visited on exit due to
the peculiarity in our gramamr with the `predicate` and `predicated`.

Fixes: elastic#36592
  • Loading branch information
@matriv
matriv committed Dec 17, 2018
1 parent 4103d3b commit e0023ce
Show file tree
Hide file tree
Showing 9 changed files with 73 additions and 46 deletions.
6 changes: 3 additions & 3 deletions x-pack/plugin/sql/src/main/antlr/SqlBase.g4
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ limitClause
;

queryTerm
: querySpecification #queryPrimaryDefault
: querySpecification #queryTermDefault
| '(' queryNoWith ')' #subquery
;

Expand Down Expand Up @@ -166,7 +166,7 @@ booleanExpression
| QUERY '(' queryString=string matchQueryOptions ')' #stringQuery
| MATCH '(' singleField=qualifiedName ',' queryString=string matchQueryOptions ')' #matchQuery
| MATCH '(' multiFields=string ',' queryString=string matchQueryOptions ')' #multiMatchQuery
| predicated #booleanDefault
| predicated #booleanExpressionDefault
| left=booleanExpression operator=AND right=booleanExpression #logicalBinary
| left=booleanExpression operator=OR right=booleanExpression #logicalBinary
;
Expand All @@ -186,7 +186,7 @@ predicated
// instead the property kind is used to differentiate
predicate
: NOT? kind=BETWEEN lower=valueExpression AND upper=valueExpression
| NOT? kind=IN '(' expression (',' expression)* ')'
| NOT? kind=IN '(' valueExpression (',' valueExpression)* ')'
| NOT? kind=IN '(' query ')'
| NOT? kind=LIKE pattern
| NOT? kind=RLIKE regex=string
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public Expression visitPredicated(PredicatedContext ctx) {
if (pCtx.query() != null) {
throw new ParsingException(loc, "IN query not supported yet");
}
e = new In(loc, exp, expressions(pCtx.expression()));
e = new In(loc, exp, expressions(pCtx.valueExpression()));
break;
case SqlBaseParser.LIKE:
e = new Like(loc, exp, visitPattern(pCtx.pattern()));
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlBaseBaseListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,13 @@ class SqlBaseBaseListener implements SqlBaseListener {
*
* <p>The default implementation does nothing.</p>
*/
@Override public void enterQueryPrimaryDefault(SqlBaseParser.QueryPrimaryDefaultContext ctx) { }
@Override public void enterQueryTermDefault(SqlBaseParser.QueryTermDefaultContext ctx) { }
/**
* {@inheritDoc}
*
* <p>The default implementation does nothing.</p>
*/
@Override public void exitQueryPrimaryDefault(SqlBaseParser.QueryPrimaryDefaultContext ctx) { }
@Override public void exitQueryTermDefault(SqlBaseParser.QueryTermDefaultContext ctx) { }
/**
* {@inheritDoc}
*
Expand Down Expand Up @@ -472,13 +472,13 @@ class SqlBaseBaseListener implements SqlBaseListener {
*
* <p>The default implementation does nothing.</p>
*/
@Override public void enterBooleanDefault(SqlBaseParser.BooleanDefaultContext ctx) { }
@Override public void enterBooleanExpressionDefault(SqlBaseParser.BooleanExpressionDefaultContext ctx) { }
/**
* {@inheritDoc}
*
* <p>The default implementation does nothing.</p>
*/
@Override public void exitBooleanDefault(SqlBaseParser.BooleanDefaultContext ctx) { }
@Override public void exitBooleanExpressionDefault(SqlBaseParser.BooleanExpressionDefaultContext ctx) { }
/**
* {@inheritDoc}
*
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlBaseBaseVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class SqlBaseBaseVisitor<T> extends AbstractParseTreeVisitor<T> implements SqlBa
* <p>The default implementation returns the result of calling
* {@link #visitChildren} on {@code ctx}.</p>
*/
@Override public T visitQueryPrimaryDefault(SqlBaseParser.QueryPrimaryDefaultContext ctx) { return visitChildren(ctx); }
@Override public T visitQueryTermDefault(SqlBaseParser.QueryTermDefaultContext ctx) { return visitChildren(ctx); }
/**
* {@inheritDoc}
*
Expand Down Expand Up @@ -283,7 +283,7 @@ class SqlBaseBaseVisitor<T> extends AbstractParseTreeVisitor<T> implements SqlBa
* <p>The default implementation returns the result of calling
* {@link #visitChildren} on {@code ctx}.</p>
*/
@Override public T visitBooleanDefault(SqlBaseParser.BooleanDefaultContext ctx) { return visitChildren(ctx); }
@Override public T visitBooleanExpressionDefault(SqlBaseParser.BooleanExpressionDefaultContext ctx) { return visitChildren(ctx); }
/**
* {@inheritDoc}
*
Expand Down
16 changes: 8 additions & 8 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlBaseListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,17 +202,17 @@ interface SqlBaseListener extends ParseTreeListener {
*/
void exitLimitClause(SqlBaseParser.LimitClauseContext ctx);
/**
* Enter a parse tree produced by the {@code queryPrimaryDefault}
* Enter a parse tree produced by the {@code queryTermDefault}
* labeled alternative in {@link SqlBaseParser#queryTerm}.
* @param ctx the parse tree
*/
void enterQueryPrimaryDefault(SqlBaseParser.QueryPrimaryDefaultContext ctx);
void enterQueryTermDefault(SqlBaseParser.QueryTermDefaultContext ctx);
/**
* Exit a parse tree produced by the {@code queryPrimaryDefault}
* Exit a parse tree produced by the {@code queryTermDefault}
* labeled alternative in {@link SqlBaseParser#queryTerm}.
* @param ctx the parse tree
*/
void exitQueryPrimaryDefault(SqlBaseParser.QueryPrimaryDefaultContext ctx);
void exitQueryTermDefault(SqlBaseParser.QueryTermDefaultContext ctx);
/**
* Enter a parse tree produced by the {@code subquery}
* labeled alternative in {@link SqlBaseParser#queryTerm}.
Expand Down Expand Up @@ -430,17 +430,17 @@ interface SqlBaseListener extends ParseTreeListener {
*/
void exitStringQuery(SqlBaseParser.StringQueryContext ctx);
/**
* Enter a parse tree produced by the {@code booleanDefault}
* Enter a parse tree produced by the {@code booleanExpressionDefault}
* labeled alternative in {@link SqlBaseParser#booleanExpression}.
* @param ctx the parse tree
*/
void enterBooleanDefault(SqlBaseParser.BooleanDefaultContext ctx);
void enterBooleanExpressionDefault(SqlBaseParser.BooleanExpressionDefaultContext ctx);
/**
* Exit a parse tree produced by the {@code booleanDefault}
* Exit a parse tree produced by the {@code booleanExpressionDefault}
* labeled alternative in {@link SqlBaseParser#booleanExpression}.
* @param ctx the parse tree
*/
void exitBooleanDefault(SqlBaseParser.BooleanDefaultContext ctx);
void exitBooleanExpressionDefault(SqlBaseParser.BooleanExpressionDefaultContext ctx);
/**
* Enter a parse tree produced by the {@code exists}
* labeled alternative in {@link SqlBaseParser#booleanExpression}.
Expand Down
36 changes: 15 additions & 21 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlBaseParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1533,22 +1533,22 @@ public <T> T accept(ParseTreeVisitor<? extends T> visitor) {
else return visitor.visitChildren(this);
}
}
public static class QueryPrimaryDefaultContext extends QueryTermContext {
public static class QueryTermDefaultContext extends QueryTermContext {
public QuerySpecificationContext querySpecification() {
return getRuleContext(QuerySpecificationContext.class,0);
}
public QueryPrimaryDefaultContext(QueryTermContext ctx) { copyFrom(ctx); }
public QueryTermDefaultContext(QueryTermContext ctx) { copyFrom(ctx); }
@Override
public void enterRule(ParseTreeListener listener) {
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).enterQueryPrimaryDefault(this);
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).enterQueryTermDefault(this);
}
@Override
public void exitRule(ParseTreeListener listener) {
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).exitQueryPrimaryDefault(this);
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).exitQueryTermDefault(this);
}
@Override
public <T> T accept(ParseTreeVisitor<? extends T> visitor) {
if ( visitor instanceof SqlBaseVisitor ) return ((SqlBaseVisitor<? extends T>)visitor).visitQueryPrimaryDefault(this);
if ( visitor instanceof SqlBaseVisitor ) return ((SqlBaseVisitor<? extends T>)visitor).visitQueryTermDefault(this);
else return visitor.visitChildren(this);
}
}
Expand All @@ -1560,7 +1560,7 @@ public final QueryTermContext queryTerm() throws RecognitionException {
setState(263);
switch (_input.LA(1)) {
case SELECT:
_localctx = new QueryPrimaryDefaultContext(_localctx);
_localctx = new QueryTermDefaultContext(_localctx);
enterOuterAlt(_localctx, 1);
{
setState(258);
Expand Down Expand Up @@ -2935,22 +2935,22 @@ public <T> T accept(ParseTreeVisitor<? extends T> visitor) {
else return visitor.visitChildren(this);
}
}
public static class BooleanDefaultContext extends BooleanExpressionContext {
public static class BooleanExpressionDefaultContext extends BooleanExpressionContext {
public PredicatedContext predicated() {
return getRuleContext(PredicatedContext.class,0);
}
public BooleanDefaultContext(BooleanExpressionContext ctx) { copyFrom(ctx); }
public BooleanExpressionDefaultContext(BooleanExpressionContext ctx) { copyFrom(ctx); }
@Override
public void enterRule(ParseTreeListener listener) {
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).enterBooleanDefault(this);
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).enterBooleanExpressionDefault(this);
}
@Override
public void exitRule(ParseTreeListener listener) {
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).exitBooleanDefault(this);
if ( listener instanceof SqlBaseListener ) ((SqlBaseListener)listener).exitBooleanExpressionDefault(this);
}
@Override
public <T> T accept(ParseTreeVisitor<? extends T> visitor) {
if ( visitor instanceof SqlBaseVisitor ) return ((SqlBaseVisitor<? extends T>)visitor).visitBooleanDefault(this);
if ( visitor instanceof SqlBaseVisitor ) return ((SqlBaseVisitor<? extends T>)visitor).visitBooleanExpressionDefault(this);
else return visitor.visitChildren(this);
}
}
Expand Down Expand Up @@ -3164,7 +3164,7 @@ private BooleanExpressionContext booleanExpression(int _p) throws RecognitionExc
break;
case 6:
{
_localctx = new BooleanDefaultContext(_localctx);
_localctx = new BooleanExpressionDefaultContext(_localctx);
_ctx = _localctx;
_prevctx = _localctx;
setState(465);
Expand Down Expand Up @@ -3363,12 +3363,6 @@ public ValueExpressionContext valueExpression(int i) {
return getRuleContext(ValueExpressionContext.class,i);
}
public TerminalNode NOT() { return getToken(SqlBaseParser.NOT, 0); }
public List<ExpressionContext> expression() {
return getRuleContexts(ExpressionContext.class);
}
public ExpressionContext expression(int i) {
return getRuleContext(ExpressionContext.class,i);
}
public TerminalNode IN() { return getToken(SqlBaseParser.IN, 0); }
public QueryContext query() {
return getRuleContext(QueryContext.class,0);
Expand Down Expand Up @@ -3449,7 +3443,7 @@ public final PredicateContext predicate() throws RecognitionException {
setState(502);
match(T__0);
setState(503);
expression();
valueExpression(0);
setState(508);
_errHandler.sync(this);
_la = _input.LA(1);
Expand All @@ -3459,7 +3453,7 @@ public final PredicateContext predicate() throws RecognitionException {
setState(504);
match(T__2);
setState(505);
expression();
valueExpression(0);
}
}
setState(510);
Expand Down Expand Up @@ -6616,7 +6610,7 @@ private boolean valueExpression_sempred(ValueExpressionContext _localctx, int pr
"\u01f0\7\16\2\2\u01f0\u01f1\5<\37\2\u01f1\u01f2\7\n\2\2\u01f2\u01f3\5"+
"<\37\2\u01f3\u021b\3\2\2\2\u01f4\u01f6\7=\2\2\u01f5\u01f4\3\2\2\2\u01f5"+
"\u01f6\3\2\2\2\u01f6\u01f7\3\2\2\2\u01f7\u01f8\7-\2\2\u01f8\u01f9\7\3"+
"\2\2\u01f9\u01fe\5,\27\2\u01fa\u01fb\7\5\2\2\u01fb\u01fd\5,\27\2\u01fc"+
"\2\2\u01f9\u01fe\5<\37\2\u01fa\u01fb\7\5\2\2\u01fb\u01fd\5<\37\2\u01fc"+
"\u01fa\3\2\2\2\u01fd\u0200\3\2\2\2\u01fe\u01fc\3\2\2\2\u01fe\u01ff\3\2"+
"\2\2\u01ff\u0201\3\2\2\2\u0200\u01fe\3\2\2\2\u0201\u0202\7\4\2\2\u0202"+
"\u021b\3\2\2\2\u0203\u0205\7=\2\2\u0204\u0203\3\2\2\2\u0204\u0205\3\2"+
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlBaseVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,12 @@ interface SqlBaseVisitor<T> extends ParseTreeVisitor<T> {
*/
T visitLimitClause(SqlBaseParser.LimitClauseContext ctx);
/**
* Visit a parse tree produced by the {@code queryPrimaryDefault}
* Visit a parse tree produced by the {@code queryTermDefault}
* labeled alternative in {@link SqlBaseParser#queryTerm}.
* @param ctx the parse tree
* @return the visitor result
*/
T visitQueryPrimaryDefault(SqlBaseParser.QueryPrimaryDefaultContext ctx);
T visitQueryTermDefault(SqlBaseParser.QueryTermDefaultContext ctx);
/**
* Visit a parse tree produced by the {@code subquery}
* labeled alternative in {@link SqlBaseParser#queryTerm}.
Expand Down Expand Up @@ -260,12 +260,12 @@ interface SqlBaseVisitor<T> extends ParseTreeVisitor<T> {
*/
T visitStringQuery(SqlBaseParser.StringQueryContext ctx);
/**
* Visit a parse tree produced by the {@code booleanDefault}
* Visit a parse tree produced by the {@code booleanExpressionDefault}
* labeled alternative in {@link SqlBaseParser#booleanExpression}.
* @param ctx the parse tree
* @return the visitor result
*/
T visitBooleanDefault(SqlBaseParser.BooleanDefaultContext ctx);
T visitBooleanExpressionDefault(SqlBaseParser.BooleanExpressionDefaultContext ctx);
/**
* Visit a parse tree produced by the {@code exists}
* labeled alternative in {@link SqlBaseParser#booleanExpression}.
Expand Down
34 changes: 31 additions & 3 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ private class CircuitBreakerListener extends SqlBaseBaseListener {

private static final short MAX_RULE_DEPTH = 200;

private boolean insideIn = false;

// Keep current depth for every rule visited.
// The totalDepth alone cannot be used as expressions like: e1 OR e2 OR e3 OR ...
// are processed as e1 OR (e2 OR (e3 OR (... and this results in the totalDepth not growing
Expand All @@ -226,9 +228,18 @@ private class CircuitBreakerListener extends SqlBaseBaseListener {

@Override
public void enterEveryRule(ParserRuleContext ctx) {
if (inDetected(ctx)) {
insideIn = true;
}

// Skip PrimaryExpressionContext for IN as it's not visited on exit due to
// the grammar's peculiarity rule with "predicated" and "predicate".
// Also skip the Identifiers as they are "cheap".
if (ctx.getClass() != SqlBaseParser.UnquoteIdentifierContext.class &&
ctx.getClass() != SqlBaseParser.QuoteIdentifierContext.class &&
ctx.getClass() != SqlBaseParser.BackQuotedIdentifierContext.class) {
ctx.getClass() != SqlBaseParser.BackQuotedIdentifierContext.class &&
(insideIn == false || ctx.getClass() != SqlBaseParser.PrimaryExpressionContext.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; " +
Expand All @@ -240,12 +251,29 @@ public void enterEveryRule(ParserRuleContext ctx) {

@Override
public void exitEveryRule(ParserRuleContext ctx) {
if (inDetected(ctx)) {
insideIn = false;
}

// Avoid having negative numbers
if (depthCounts.containsKey(ctx.getClass().getSimpleName())) {
depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 0, (short) -1);
if (depthCounts.containsKey(getSimpleName(ctx))) {
depthCounts.putOrAdd(getSimpleName(ctx), (short) 0, (short) -1);
}
super.exitEveryRule(ctx);
}

private String getSimpleName(ParserRuleContext ctx) {
// e.g.: ValueExpressionContext can exit as ValueExpressionDefaultContext
return ctx.getClass().getSimpleName().replace("Default", "");
}

private boolean inDetected(ParserRuleContext ctx) {
if (ctx.getParent() != null && ctx.getParent().getClass() == SqlBaseParser.PredicateContext.class) {
SqlBaseParser.PredicateContext pc = (SqlBaseParser.PredicateContext) ctx.getParent();
return pc.kind != null && pc.kind.getType() == SqlBaseParser.IN;
}
return false;
}
}

private static final BaseErrorListener ERROR_LISTENER = new BaseErrorListener() {
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,11 @@ public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() {
e.getMessage());
}

public void testLimitStackOverflowForInAndLiteralsIsNotApplied() {
new SqlParser().createStatement("SELECT * FROM t WHERE a IN(" +
Joiner.on(",").join(nCopies(100_000, "a + b")) + ")");
}

private LogicalPlan parseStatement(String sql) {
return new SqlParser().createStatement(sql);
}
Expand Down

0 comments on commit e0023ce

Please sign in to comment.