From 09d24c77983cd31919706848c0f93ea31ca0a30e Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Sat, 14 Mar 2020 12:54:21 +0100 Subject: [PATCH 1/3] SQL: Fix NPE for parameterized LIKE/RLIKE Fix NPE when `null` is passed as a parameter for a parameterized pattern of LIKE/RLIKE. e.g.: `field LIKE ?` params=[null]` Check for null pattern in LIKE/RLIKE as for RLIKE (RegexpQuery) we get an IllegalArgumentExpression from Lucence but for LIKE (WildcardQuery) we get an NPE. Fixes: #53557 --- .../predicate/regex/RegexMatch.java | 10 ++++++- .../xpack/sql/parser/ExpressionBuilder.java | 3 ++ .../analyzer/VerifierErrorMessagesTests.java | 28 +++++++++++++++++-- .../xpack/sql/parser/ExpressionTests.java | 12 ++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java index 517c243992c24..4a0f098b93147 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java @@ -16,6 +16,7 @@ import java.util.Objects; +import static org.elasticsearch.common.logging.LoggerMessageFormat.format; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact; public abstract class RegexMatch extends UnaryScalarFunction { @@ -46,7 +47,14 @@ public Nullability nullable() { @Override protected TypeResolution resolveType() { - return isStringAndExact(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT); + TypeResolution resolution = isStringAndExact(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT); + if (resolution.unresolved()) { + return resolution; + } + if (pattern == null) { + return new TypeResolution(format(null, "[{}] pattern must not be [null]", sourceText())); + } + return TypeResolution.TYPE_RESOLVED; } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java index 0455039a34e25..d80af708ee9d8 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java @@ -263,6 +263,9 @@ public LikePattern visitPattern(PatternContext ctx) { } String pattern = string(ctx.value); + if (pattern == null) { + return null; + } int pos = pattern.indexOf('*'); if (pos >= 0) { throw new ParsingException(source(ctx.value), diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index bb54f5c7c719f..c4e105276a215 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -23,10 +23,13 @@ import org.elasticsearch.xpack.sql.expression.predicate.conditional.Least; import org.elasticsearch.xpack.sql.expression.predicate.conditional.NullIf; import org.elasticsearch.xpack.sql.parser.SqlParser; +import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import org.elasticsearch.xpack.sql.stats.Metrics; import java.util.Arrays; +import java.util.Collections; import java.util.LinkedHashMap; +import java.util.List; import java.util.Locale; import java.util.Map; @@ -44,12 +47,21 @@ public class VerifierErrorMessagesTests extends ESTestCase { loadMapping("mapping-multi-field-with-nested.json"))); private String error(String sql) { - return error(indexResolution, sql); + return error(indexResolution, sql, Collections.emptyList()); + } + + private String error(String sql, List params) { + return error(indexResolution, sql, params); } private String error(IndexResolution getIndexResult, String sql) { + return error(getIndexResult, sql, Collections.emptyList()); + } + + private String error(IndexResolution getIndexResult, String sql, List params) { Analyzer analyzer = new Analyzer(SqlTestUtils.TEST_CFG, new SqlFunctionRegistry(), getIndexResult, new Verifier(new Metrics())); - VerificationException e = expectThrows(VerificationException.class, () -> analyzer.analyze(parser.createStatement(sql), true)); + VerificationException e = expectThrows(VerificationException.class, + () -> analyzer.analyze(parser.createStatement(sql, params), true)); String message = e.getMessage(); assertTrue(message.startsWith("Found ")); String pattern = "\nline "; @@ -745,12 +757,24 @@ public void testInvalidTypeForLikeMatch() { error("SELECT * FROM test WHERE text LIKE 'foo'")); } + public void testInvalidPatternForLikeMatch() { + assertEquals("1:26: [keyword LIKE ?] pattern must not be [null]", + error("SELECT * FROM test WHERE keyword LIKE ?", + Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null)))); + } + public void testInvalidTypeForRLikeMatch() { assertEquals("1:26: [text RLIKE 'foo'] cannot operate on field of data type [text]: " + "No keyword/multi-field defined exact matches for [text]; define one or use MATCH/QUERY instead", error("SELECT * FROM test WHERE text RLIKE 'foo'")); } + public void testInvalidPatternForRLikeMatch() { + assertEquals("1:26: [keyword RLIKE ?] pattern must not be [null]", + error("SELECT * FROM test WHERE keyword RLIKE ?", + Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null)))); + } + public void testMatchAndQueryFunctionsNotAllowedInSelect() { assertEquals("1:8: Cannot use MATCH() or QUERY() full-text search functions in the SELECT clause", error("SELECT MATCH(text, 'foo') FROM test")); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java index ad2d3066e121f..e86b66e80fe77 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java @@ -13,6 +13,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullEquals; +import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; import org.elasticsearch.xpack.sql.SqlTestUtils; import org.elasticsearch.xpack.sql.expression.function.scalar.Cast; import org.elasticsearch.xpack.sql.expression.literal.interval.Interval; @@ -21,16 +22,19 @@ import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Add; import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Sub; +import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import java.time.Duration; import java.time.Period; import java.time.temporal.TemporalAmount; +import java.util.Collections; import java.util.Locale; import static java.lang.String.format; import static org.elasticsearch.xpack.ql.type.DataTypes.BOOLEAN; import static org.elasticsearch.xpack.ql.type.DataTypes.DOUBLE; import static org.elasticsearch.xpack.ql.type.DataTypes.INTEGER; +import static org.elasticsearch.xpack.ql.type.DataTypes.KEYWORD; import static org.elasticsearch.xpack.ql.type.DataTypes.LONG; import static org.hamcrest.Matchers.startsWith; @@ -542,4 +546,12 @@ public void testCaseWithOperand() { assertEquals("WHEN 1 THEN 'one'", ifc.sourceText()); assertEquals("many", c.elseResult().toString()); } + + public void testLikePatternWithNullParameter() { + Expression expr = parser.createExpression("a LIKE ?", + Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null))); + assertEquals(Like.class, expr.getClass()); + Like like = (Like) expr; + assertNull(like.pattern()); + } } From 676a3a597dc2cd3aac631123d8a7e5b147577d55 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 16 Mar 2020 12:09:23 +0100 Subject: [PATCH 2/3] move null check to Parser --- .../predicate/regex/RegexMatch.java | 10 +------ .../xpack/sql/parser/ExpressionBuilder.java | 2 +- .../analyzer/VerifierErrorMessagesTests.java | 28 ++----------------- .../xpack/sql/parser/ExpressionTests.java | 11 ++++---- 4 files changed, 9 insertions(+), 42 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java index 4a0f098b93147..517c243992c24 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/RegexMatch.java @@ -16,7 +16,6 @@ import java.util.Objects; -import static org.elasticsearch.common.logging.LoggerMessageFormat.format; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact; public abstract class RegexMatch extends UnaryScalarFunction { @@ -47,14 +46,7 @@ public Nullability nullable() { @Override protected TypeResolution resolveType() { - TypeResolution resolution = isStringAndExact(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT); - if (resolution.unresolved()) { - return resolution; - } - if (pattern == null) { - return new TypeResolution(format(null, "[{}] pattern must not be [null]", sourceText())); - } - return TypeResolution.TYPE_RESOLVED; + return isStringAndExact(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java index d80af708ee9d8..7c9973d6e405a 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java @@ -264,7 +264,7 @@ public LikePattern visitPattern(PatternContext ctx) { String pattern = string(ctx.value); if (pattern == null) { - return null; + throw new ParsingException(source(ctx.value), "Pattern must not be [null]"); } int pos = pattern.indexOf('*'); if (pos >= 0) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index c4e105276a215..bb54f5c7c719f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -23,13 +23,10 @@ import org.elasticsearch.xpack.sql.expression.predicate.conditional.Least; import org.elasticsearch.xpack.sql.expression.predicate.conditional.NullIf; import org.elasticsearch.xpack.sql.parser.SqlParser; -import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import org.elasticsearch.xpack.sql.stats.Metrics; import java.util.Arrays; -import java.util.Collections; import java.util.LinkedHashMap; -import java.util.List; import java.util.Locale; import java.util.Map; @@ -47,21 +44,12 @@ public class VerifierErrorMessagesTests extends ESTestCase { loadMapping("mapping-multi-field-with-nested.json"))); private String error(String sql) { - return error(indexResolution, sql, Collections.emptyList()); - } - - private String error(String sql, List params) { - return error(indexResolution, sql, params); + return error(indexResolution, sql); } private String error(IndexResolution getIndexResult, String sql) { - return error(getIndexResult, sql, Collections.emptyList()); - } - - private String error(IndexResolution getIndexResult, String sql, List params) { Analyzer analyzer = new Analyzer(SqlTestUtils.TEST_CFG, new SqlFunctionRegistry(), getIndexResult, new Verifier(new Metrics())); - VerificationException e = expectThrows(VerificationException.class, - () -> analyzer.analyze(parser.createStatement(sql, params), true)); + VerificationException e = expectThrows(VerificationException.class, () -> analyzer.analyze(parser.createStatement(sql), true)); String message = e.getMessage(); assertTrue(message.startsWith("Found ")); String pattern = "\nline "; @@ -757,24 +745,12 @@ public void testInvalidTypeForLikeMatch() { error("SELECT * FROM test WHERE text LIKE 'foo'")); } - public void testInvalidPatternForLikeMatch() { - assertEquals("1:26: [keyword LIKE ?] pattern must not be [null]", - error("SELECT * FROM test WHERE keyword LIKE ?", - Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null)))); - } - public void testInvalidTypeForRLikeMatch() { assertEquals("1:26: [text RLIKE 'foo'] cannot operate on field of data type [text]: " + "No keyword/multi-field defined exact matches for [text]; define one or use MATCH/QUERY instead", error("SELECT * FROM test WHERE text RLIKE 'foo'")); } - public void testInvalidPatternForRLikeMatch() { - assertEquals("1:26: [keyword RLIKE ?] pattern must not be [null]", - error("SELECT * FROM test WHERE keyword RLIKE ?", - Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null)))); - } - public void testMatchAndQueryFunctionsNotAllowedInSelect() { assertEquals("1:8: Cannot use MATCH() or QUERY() full-text search functions in the SELECT clause", error("SELECT MATCH(text, 'foo') FROM test")); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java index e86b66e80fe77..6c22c7b4f3323 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java @@ -547,11 +547,10 @@ public void testCaseWithOperand() { assertEquals("many", c.elseResult().toString()); } - public void testLikePatternWithNullParameter() { - Expression expr = parser.createExpression("a LIKE ?", - Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null))); - assertEquals(Like.class, expr.getClass()); - Like like = (Like) expr; - assertNull(like.pattern()); + public void testLikePatternWithNullParameterNotAllowed() { + ParsingException e = expectThrows(ParsingException.class, + () -> parser.createExpression("a LIKE ?", + Collections.singletonList(new SqlTypedParamValue(KEYWORD.typeName(), null)))); + assertEquals("line 1:9: Pattern must not be [null]", e.getMessage()); } } From a946dc09d2b5054f5de19f98656c9eaf6d7116a4 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 16 Mar 2020 12:20:25 +0100 Subject: [PATCH 3/3] remove unused import --- .../java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java index 6c22c7b4f3323..6211f40e0f0e5 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java @@ -13,7 +13,6 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullEquals; -import org.elasticsearch.xpack.ql.expression.predicate.regex.Like; import org.elasticsearch.xpack.sql.SqlTestUtils; import org.elasticsearch.xpack.sql.expression.function.scalar.Cast; import org.elasticsearch.xpack.sql.expression.literal.interval.Interval;