From 16971af97a14e2d0f1fa7bfeb280d1a914175683 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 9 Oct 2023 14:31:14 +0200 Subject: [PATCH] ESQL: Limit how many bytes `concat()` can process (#100360) (#100507) This adds a limit on the bytes length that a concatenated string can get to. The limit is set to 1MB (per entry). When the limit is hit, an exception is returned to the caller (similar to an accounting circuit breaking) and execution halted. Related: #100288. --- docs/changelog/100360.yaml | 5 +++ .../esql/qa/single_node/HeapAttackIT.java | 10 ++++- .../function/scalar/string/Concat.java | 23 +++++++++++ .../function/scalar/string/ConcatTests.java | 40 +++++++++++++++++++ 4 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 docs/changelog/100360.yaml diff --git a/docs/changelog/100360.yaml b/docs/changelog/100360.yaml new file mode 100644 index 0000000000000..6d0dcafe16a8f --- /dev/null +++ b/docs/changelog/100360.yaml @@ -0,0 +1,5 @@ +pr: 100360 +summary: "ESQL: Limit how many bytes `concat()` can process" +area: ES|QL +type: bug +issues: [] diff --git a/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/HeapAttackIT.java b/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/HeapAttackIT.java index 6cedba3e4ee28..55b4454ce2105 100644 --- a/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/HeapAttackIT.java +++ b/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/HeapAttackIT.java @@ -152,10 +152,16 @@ public void testSmallConcat() throws IOException { assertMap(map, matchesMap().entry("columns", columns).entry("values", values)); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99826") public void testHugeConcat() throws IOException { initSingleDocIndex(); - assertCircuitBreaks(() -> concat(10)); + ResponseException e = expectThrows(ResponseException.class, () -> concat(10)); + Map map = XContentHelper.convertToMap(JsonXContent.jsonXContent, EntityUtils.toString(e.getResponse().getEntity()), false); + logger.info("expected request rejected {}", map); + assertMap( + map, + matchesMap().entry("status", 400) + .entry("error", matchesMap().extraOk().entry("reason", "concatenating more than [1048576] bytes is not supported")) + ); } private Response concat(int evals) throws IOException { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java index ae805975646a6..c987513d5919e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java @@ -13,6 +13,8 @@ import org.elasticsearch.compute.operator.BreakingBytesRefBuilder; import org.elasticsearch.compute.operator.EvalOperator; import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator; +import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.xpack.esql.EsqlClientException; import org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper; import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; @@ -27,6 +29,7 @@ import java.util.function.Function; import java.util.stream.Stream; +import static org.elasticsearch.common.unit.ByteSizeUnit.MB; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT; import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isString; @@ -34,6 +37,9 @@ * Join strings. */ public class Concat extends ScalarFunction implements EvaluatorMapper { + + static final long MAX_CONCAT_LENGTH = MB.toBytes(1); + public Concat(Source source, Expression first, List rest) { super(source, Stream.concat(Stream.of(first), rest.stream()).toList()); } @@ -83,6 +89,7 @@ public ExpressionEvaluator.Factory toEvaluator(Function MAX_CONCAT_LENGTH) { + throw new EsqlClientException("concatenating more than [" + MAX_CONCAT_LENGTH + "] bytes is not supported") { + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; // return a 400 response + } + }; + } + return length; + } + @Override public Expression replaceChildren(List newChildren) { return new Concat(source(), newChildren.get(0), newChildren.subList(1, newChildren.size())); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/ConcatTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/ConcatTests.java index caec572351675..32e894e5282d5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/ConcatTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/ConcatTests.java @@ -13,22 +13,26 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.compute.data.Block; import org.elasticsearch.compute.operator.EvalOperator; +import org.elasticsearch.xpack.esql.EsqlClientException; import org.elasticsearch.xpack.esql.expression.function.AbstractFunctionTestCase; import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier; import org.elasticsearch.xpack.esql.type.EsqlDataTypes; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.Literal; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.function.Supplier; import java.util.stream.IntStream; import static org.elasticsearch.compute.data.BlockUtils.toJavaObject; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; public class ConcatTests extends AbstractFunctionTestCase { public ConcatTests(@Name("TestCase") Supplier testCaseSupplier) { @@ -118,6 +122,17 @@ public void testSomeConstant() { assertThat(expression.typeResolved().message(), equalTo(testCase.getExpectedTypeError())); return; } + + int totalLength = testDataLength(); + if (totalLength >= Concat.MAX_CONCAT_LENGTH || rarely()) { + boolean hasNulls = mix.stream().anyMatch(x -> x instanceof Literal l && l.value() == null) + || fieldValues.stream().anyMatch(Objects::isNull); + if (hasNulls == false) { + testOversized(totalLength, mix, fieldValues); + return; + } + } + try ( EvalOperator.ExpressionEvaluator eval = evaluator(expression).get(driverContext()); Block.Ref ref = eval.eval(row(fieldValues)) @@ -125,4 +140,29 @@ public void testSomeConstant() { assertThat(toJavaObject(ref.block(), 0), testCase.getMatcher()); } } + + private void testOversized(int totalLen, List mix, List fieldValues) { + for (int len; totalLen < Concat.MAX_CONCAT_LENGTH; totalLen += len) { + len = randomIntBetween(1, (int) Concat.MAX_CONCAT_LENGTH); + mix.add(new Literal(Source.EMPTY, new BytesRef(randomAlphaOfLength(len)), DataTypes.KEYWORD)); + } + Expression expression = build(testCase.getSource(), mix); + Exception e = expectThrows(EsqlClientException.class, () -> { + try ( + EvalOperator.ExpressionEvaluator eval = evaluator(expression).get(driverContext()); + Block.Ref ref = eval.eval(row(fieldValues)); + ) {} + }); + assertThat(e.getMessage(), is("concatenating more than [1048576] bytes is not supported")); + } + + private int testDataLength() { + int totalLength = 0; + for (var data : testCase.getData()) { + if (data.data() instanceof BytesRef bytesRef) { + totalLength += bytesRef.length; + } + } + return totalLength; + } }