Skip to content

Commit 7fa3154

Browse files
matrivmatriv
committed
SQL: Fix wrong appliance of StackOverflow limit for IN (#36724)
Fix grammar so that each element inside the list of values for IN is a valueExpression and not a more generic expression. Introduce a mapping for context names as some rules in the grammar are exited with a different rule from the one they entered.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 exitRule() due to the peculiarity in our gramamr with the predicate and predicated. Fixes: #36592
1 parent b5c5954 commit 7fa3154

File tree

5 files changed

+147
-24
lines changed

5 files changed

+147
-24
lines changed

‎x-pack/plugin/sql/src/main/antlr/SqlBase.g4‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ predicated
186186
// instead the property kind is used to differentiate
187187
predicate
188188
: NOT? kind=BETWEEN lower=valueExpression AND upper=valueExpression
189-
| NOT? kind=IN '(' expression (',' expression)* ')'
189+
| NOT? kind=IN '(' valueExpression (',' valueExpression)* ')'
190190
| NOT? kind=IN '(' query ')'
191191
| NOT? kind=LIKE pattern
192192
| NOT? kind=RLIKE regex=string

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ public Expression visitPredicated(PredicatedContext ctx) {
202202
if (pCtx.query() != null) {
203203
throw new ParsingException(loc, "IN query not supported yet");
204204
}
205-
e = new In(loc, exp, expressions(pCtx.expression()));
205+
e = new In(loc, exp, expressions(pCtx.valueExpression()));
206206
break;
207207
case SqlBaseParser.LIKE:
208208
e = new Like(loc, exp, visitPattern(pCtx.pattern()));

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlBaseParser.java‎

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,27 @@
11
// ANTLR GENERATED CODE: DO NOT EDIT
22
package org.elasticsearch.xpack.sql.parser;
3-
import org.antlr.v4.runtime.atn.*;
3+
4+
import org.antlr.v4.runtime.FailedPredicateException;
5+
import org.antlr.v4.runtime.NoViableAltException;
6+
import org.antlr.v4.runtime.Parser;
7+
import org.antlr.v4.runtime.ParserRuleContext;
8+
import org.antlr.v4.runtime.RecognitionException;
9+
import org.antlr.v4.runtime.RuleContext;
10+
import org.antlr.v4.runtime.RuntimeMetaData;
11+
import org.antlr.v4.runtime.Token;
12+
import org.antlr.v4.runtime.TokenStream;
13+
import org.antlr.v4.runtime.Vocabulary;
14+
import org.antlr.v4.runtime.VocabularyImpl;
15+
import org.antlr.v4.runtime.atn.ATN;
16+
import org.antlr.v4.runtime.atn.ATNDeserializer;
17+
import org.antlr.v4.runtime.atn.ParserATNSimulator;
18+
import org.antlr.v4.runtime.atn.PredictionContextCache;
419
import org.antlr.v4.runtime.dfa.DFA;
5-
import org.antlr.v4.runtime.*;
6-
import org.antlr.v4.runtime.misc.*;
7-
import org.antlr.v4.runtime.tree.*;
20+
import org.antlr.v4.runtime.tree.ParseTreeListener;
21+
import org.antlr.v4.runtime.tree.ParseTreeVisitor;
22+
import org.antlr.v4.runtime.tree.TerminalNode;
23+
824
import java.util.List;
9-
import java.util.Iterator;
10-
import java.util.ArrayList;
1125

1226
@SuppressWarnings({"all", "warnings", "unchecked", "unused", "cast"})
1327
class SqlBaseParser extends Parser {
@@ -3276,12 +3290,6 @@ public ValueExpressionContext valueExpression(int i) {
32763290
return getRuleContext(ValueExpressionContext.class,i);
32773291
}
32783292
public TerminalNode NOT() { return getToken(SqlBaseParser.NOT, 0); }
3279-
public List<ExpressionContext> expression() {
3280-
return getRuleContexts(ExpressionContext.class);
3281-
}
3282-
public ExpressionContext expression(int i) {
3283-
return getRuleContext(ExpressionContext.class,i);
3284-
}
32853293
public TerminalNode IN() { return getToken(SqlBaseParser.IN, 0); }
32863294
public QueryContext query() {
32873295
return getRuleContext(QueryContext.class,0);
@@ -3362,7 +3370,7 @@ public final PredicateContext predicate() throws RecognitionException {
33623370
setState(490);
33633371
match(T__0);
33643372
setState(491);
3365-
expression();
3373+
valueExpression(0);
33663374
setState(496);
33673375
_errHandler.sync(this);
33683376
_la = _input.LA(1);
@@ -3372,7 +3380,7 @@ public final PredicateContext predicate() throws RecognitionException {
33723380
setState(492);
33733381
match(T__2);
33743382
setState(493);
3375-
expression();
3383+
valueExpression(0);
33763384
}
33773385
}
33783386
setState(498);
@@ -6162,7 +6170,7 @@ private boolean valueExpression_sempred(ValueExpressionContext _localctx, int pr
61626170
"\7\16\2\2\u01e4\u01e5\5<\37\2\u01e5\u01e6\7\n\2\2\u01e6\u01e7\5<\37\2"+
61636171
"\u01e7\u020f\3\2\2\2\u01e8\u01ea\7\62\2\2\u01e9\u01e8\3\2\2\2\u01e9\u01ea"+
61646172
"\3\2\2\2\u01ea\u01eb\3\2\2\2\u01eb\u01ec\7\'\2\2\u01ec\u01ed\7\3\2\2\u01ed"+
6165-
"\u01f2\5,\27\2\u01ee\u01ef\7\5\2\2\u01ef\u01f1\5,\27\2\u01f0\u01ee\3\2"+
6173+
"\u01f2\5<\37\2\u01ee\u01ef\7\5\2\2\u01ef\u01f1\5<\37\2\u01f0\u01ee\3\2"+
61666174
"\2\2\u01f1\u01f4\3\2\2\2\u01f2\u01f0\3\2\2\2\u01f2\u01f3\3\2\2\2\u01f3"+
61676175
"\u01f5\3\2\2\2\u01f4\u01f2\3\2\2\2\u01f5\u01f6\7\4\2\2\u01f6\u020f\3\2"+
61686176
"\2\2\u01f7\u01f9\7\62\2\2\u01f8\u01f7\3\2\2\2\u01f8\u01f9\3\2\2\2\u01f9"+

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java‎

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,18 @@
2626
import org.apache.logging.log4j.LogManager;
2727
import org.apache.logging.log4j.Logger;
2828
import org.elasticsearch.xpack.sql.expression.Expression;
29+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.BackQuotedIdentifierContext;
30+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.BooleanDefaultContext;
31+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.BooleanExpressionContext;
32+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.PrimaryExpressionContext;
33+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryPrimaryDefaultContext;
34+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryTermContext;
35+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QuoteIdentifierContext;
36+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.StatementContext;
37+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.StatementDefaultContext;
38+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.UnquoteIdentifierContext;
39+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.ValueExpressionContext;
40+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.ValueExpressionDefaultContext;
2941
import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan;
3042
import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue;
3143

@@ -214,10 +226,26 @@ public void exitNonReserved(SqlBaseParser.NonReservedContext context) {
214226
/**
215227
* Used to catch large expressions that can lead to stack overflows
216228
*/
217-
private class CircuitBreakerListener extends SqlBaseBaseListener {
229+
static class CircuitBreakerListener extends SqlBaseBaseListener {
218230

219231
private static final short MAX_RULE_DEPTH = 200;
220232

233+
/**
234+
* Due to the structure of the grammar and our custom handling in {@link ExpressionBuilder}
235+
* some expressions can exit with a different class than they entered:
236+
* e.g.: ValueExpressionContext can exit as ValueExpressionDefaultContext
237+
*/
238+
private static final Map<String, String> ENTER_EXIT_RULE_MAPPING = new HashMap<>();
239+
240+
static {
241+
ENTER_EXIT_RULE_MAPPING.put(StatementDefaultContext.class.getSimpleName(), StatementContext.class.getSimpleName());
242+
ENTER_EXIT_RULE_MAPPING.put(QueryPrimaryDefaultContext.class.getSimpleName(), QueryTermContext.class.getSimpleName());
243+
ENTER_EXIT_RULE_MAPPING.put(BooleanDefaultContext.class.getSimpleName(), BooleanExpressionContext.class.getSimpleName());
244+
ENTER_EXIT_RULE_MAPPING.put(ValueExpressionDefaultContext.class.getSimpleName(), ValueExpressionContext.class.getSimpleName());
245+
}
246+
247+
private boolean insideIn = false;
248+
221249
// Keep current depth for every rule visited.
222250
// The totalDepth alone cannot be used as expressions like: e1 OR e2 OR e3 OR ...
223251
// are processed as e1 OR (e2 OR (e3 OR (... and this results in the totalDepth not growing
@@ -226,9 +254,18 @@ private class CircuitBreakerListener extends SqlBaseBaseListener {
226254

227255
@Override
228256
public void enterEveryRule(ParserRuleContext ctx) {
229-
if (ctx.getClass() != SqlBaseParser.UnquoteIdentifierContext.class &&
230-
ctx.getClass() != SqlBaseParser.QuoteIdentifierContext.class &&
231-
ctx.getClass() != SqlBaseParser.BackQuotedIdentifierContext.class) {
257+
if (inDetected(ctx)) {
258+
insideIn = true;
259+
}
260+
261+
// Skip PrimaryExpressionContext for IN as it's not visited on exit due to
262+
// the grammar's peculiarity rule with "predicated" and "predicate".
263+
// Also skip the Identifiers as they are "cheap".
264+
if (ctx.getClass() != UnquoteIdentifierContext.class &&
265+
ctx.getClass() != QuoteIdentifierContext.class &&
266+
ctx.getClass() != BackQuotedIdentifierContext.class &&
267+
(insideIn == false || ctx.getClass() != PrimaryExpressionContext.class)) {
268+
232269
int currentDepth = depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 1, (short) 1);
233270
if (currentDepth > MAX_RULE_DEPTH) {
234271
throw new ParsingException(source(ctx), "SQL statement too large; " +
@@ -240,12 +277,35 @@ public void enterEveryRule(ParserRuleContext ctx) {
240277

241278
@Override
242279
public void exitEveryRule(ParserRuleContext ctx) {
243-
// Avoid having negative numbers
244-
if (depthCounts.containsKey(ctx.getClass().getSimpleName())) {
245-
depthCounts.putOrAdd(ctx.getClass().getSimpleName(), (short) 0, (short) -1);
280+
if (inDetected(ctx)) {
281+
insideIn = false;
246282
}
283+
284+
decrementCounter(ctx);
247285
super.exitEveryRule(ctx);
248286
}
287+
288+
ObjectShortHashMap<String> depthCounts() {
289+
return depthCounts;
290+
}
291+
292+
private void decrementCounter(ParserRuleContext ctx) {
293+
String className = ctx.getClass().getSimpleName();
294+
String classNameToDecrement = ENTER_EXIT_RULE_MAPPING.getOrDefault(className, className);
295+
296+
// Avoid having negative numbers
297+
if (depthCounts.containsKey(classNameToDecrement)) {
298+
depthCounts.putOrAdd(classNameToDecrement, (short) 0, (short) -1);
299+
}
300+
}
301+
302+
private boolean inDetected(ParserRuleContext ctx) {
303+
if (ctx.getParent() != null && ctx.getParent().getClass() == SqlBaseParser.PredicateContext.class) {
304+
SqlBaseParser.PredicateContext pc = (SqlBaseParser.PredicateContext) ctx.getParent();
305+
return pc.kind != null && pc.kind.getType() == SqlBaseParser.IN;
306+
}
307+
return false;
308+
}
249309
}
250310

251311
private static final BaseErrorListener ERROR_LISTENER = new BaseErrorListener() {

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,19 @@
1515
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate;
1616
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MultiMatchQueryPredicate;
1717
import org.elasticsearch.xpack.sql.expression.predicate.fulltext.StringQueryPredicate;
18+
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In;
19+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.BooleanExpressionContext;
20+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryPrimaryDefaultContext;
21+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryTermContext;
22+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.StatementContext;
23+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.StatementDefaultContext;
24+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.ValueExpressionContext;
25+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.ValueExpressionDefaultContext;
1826
import org.elasticsearch.xpack.sql.plan.logical.Filter;
1927
import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan;
2028
import org.elasticsearch.xpack.sql.plan.logical.OrderBy;
2129
import org.elasticsearch.xpack.sql.plan.logical.Project;
30+
import org.elasticsearch.xpack.sql.plan.logical.With;
2231

2332
import java.util.ArrayList;
2433
import java.util.List;
@@ -28,6 +37,7 @@
2837
import static org.hamcrest.Matchers.hasEntry;
2938
import static org.hamcrest.Matchers.hasSize;
3039
import static org.hamcrest.Matchers.instanceOf;
40+
import static org.hamcrest.Matchers.startsWith;
3141

3242
public class SqlParserTests extends ESTestCase {
3343

@@ -254,6 +264,51 @@ public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() {
254264
e.getMessage());
255265
}
256266

267+
public void testLimitStackOverflowForInAndLiteralsIsNotApplied() {
268+
int noChildren = 100_000;
269+
LogicalPlan plan = parseStatement("SELECT * FROM t WHERE a IN(" +
270+
Joiner.on(",").join(nCopies(noChildren, "a + b")) + ")");
271+
272+
assertEquals(With.class, plan.getClass());
273+
assertEquals(Project.class, ((With) plan).child().getClass());
274+
assertEquals(Filter.class, ((Project) ((With) plan).child()).child().getClass());
275+
Filter filter = (Filter) ((Project) ((With) plan).child()).child();
276+
assertEquals(In.class, filter.condition().getClass());
277+
In in = (In) filter.condition();
278+
assertEquals("?a", in.value().toString());
279+
assertEquals(noChildren, in.list().size());
280+
assertThat(in.list().get(0).toString(), startsWith("(a) + (b)#"));
281+
}
282+
283+
public void testDecrementOfDepthCounter() {
284+
SqlParser.CircuitBreakerListener cbl = new SqlParser.CircuitBreakerListener();
285+
StatementContext sc = new StatementContext();
286+
QueryTermContext qtc = new QueryTermContext();
287+
ValueExpressionContext vec = new ValueExpressionContext();
288+
BooleanExpressionContext bec = new BooleanExpressionContext();
289+
290+
cbl.enterEveryRule(sc);
291+
cbl.enterEveryRule(sc);
292+
cbl.enterEveryRule(qtc);
293+
cbl.enterEveryRule(qtc);
294+
cbl.enterEveryRule(qtc);
295+
cbl.enterEveryRule(vec);
296+
cbl.enterEveryRule(bec);
297+
cbl.enterEveryRule(bec);
298+
299+
cbl.exitEveryRule(new StatementDefaultContext(sc));
300+
cbl.exitEveryRule(new StatementDefaultContext(sc));
301+
cbl.exitEveryRule(new QueryPrimaryDefaultContext(qtc));
302+
cbl.exitEveryRule(new QueryPrimaryDefaultContext(qtc));
303+
cbl.exitEveryRule(new ValueExpressionDefaultContext(vec));
304+
cbl.exitEveryRule(new SqlBaseParser.BooleanDefaultContext(bec));
305+
306+
assertEquals(0, cbl.depthCounts().get(SqlBaseParser.StatementContext.class.getSimpleName()));
307+
assertEquals(1, cbl.depthCounts().get(SqlBaseParser.QueryTermContext.class.getSimpleName()));
308+
assertEquals(0, cbl.depthCounts().get(SqlBaseParser.ValueExpressionContext.class.getSimpleName()));
309+
assertEquals(1, cbl.depthCounts().get(SqlBaseParser.BooleanExpressionContext.class.getSimpleName()));
310+
}
311+
257312
private LogicalPlan parseStatement(String sql) {
258313
return new SqlParser().createStatement(sql);
259314
}

0 commit comments

Comments
 (0)