diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index d67e7742b625..84ea31b9a741 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -337,6 +337,12 @@ Improvements * GITHUB#16271: Add detail to KNN no-match explanations. (Jakub Slowinski) +* GITHUB#16423: Add (configurable) guards for complexity of expressions. Previously + the expressions parser could throw StackOverflowError on deeply nested expressions + or also cause VerifyError and other LinkageErrors when the expression was to complex. + The new code allows to add a hard limit for nesting (default is 1024) and no longer + throws virtual machine errors. (Uwe Schindler, Seth Kraft) + Optimizations --------------------- * GITHUG#16280: Single-pass writeString fast path for short strings in ByteBuffersDataOutput (neoremind) diff --git a/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java b/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java index d8c1c6a663eb..ab956d987d9b 100644 --- a/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java +++ b/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java @@ -58,6 +58,7 @@ import org.antlr.v4.runtime.CharStreams; import org.antlr.v4.runtime.CommonTokenStream; import org.antlr.v4.runtime.DiagnosticErrorListener; +import org.antlr.v4.runtime.ParserRuleContext; import org.antlr.v4.runtime.RecognitionException; import org.antlr.v4.runtime.Recognizer; import org.antlr.v4.runtime.atn.PredictionMode; @@ -100,6 +101,13 @@ * be resolved correctly using a private {@link Lookup} instance. Ideally the methods should be * {@code static}, but you can use {@link MethodHandle#bindTo(Object)} to bind it to a receiver. * + *

The parser has a default maximum nesting limit of 1024 (see {@link + * #DEFAULT_MAX_NESTING_DEPTH}) which is enforced during parsing. Violating it will cause a {@link + * ParseException}. Please keep in mind that also code like + * a + b + c + d + ... + z is treated as if it would look like this: + * (((((a + b) + c) + d) + ...) + z) (because of the nature of + * the JVM as a stack-based machine). + * * @lucene.experimental */ public final class JavascriptCompiler { @@ -129,8 +137,12 @@ public final class JavascriptCompiler { private static final ExceptionsAttribute ATTR_THROWS_IOEXCEPTION = ExceptionsAttribute.ofSymbols(IOException.class.describeConstable().orElseThrow()); + /** The default maximum depth of nesting (function calls, precedence - also implicit) */ + public static final int DEFAULT_MAX_NESTING_DEPTH = 1024; + final String sourceText; final Map functions; + final int maxNestingDepth; final boolean picky; /** @@ -139,6 +151,8 @@ public final class JavascriptCompiler { * @param sourceText The expression to compile * @return A new compiled expression * @throws ParseException on failure to compile + * @throws IllegalStateException if the resulting expression class fails to link (e.g., + * complexity) */ public static Expression compile(String sourceText) throws ParseException { return compile(sourceText, DEFAULT_FUNCTIONS); @@ -155,10 +169,36 @@ public static Expression compile(String sourceText) throws ParseException { * @param functions map of String names to {@link MethodHandle}s * @return A new compiled expression * @throws ParseException on failure to compile + * @throws IllegalArgumentException if any of the functions does not have correct signature + * @throws IllegalStateException if the resulting expression class fails to link (e.g., + * complexity) */ public static Expression compile(String sourceText, Map functions) throws ParseException { - return compile(sourceText, functions, false); + return compile(sourceText, functions, DEFAULT_MAX_NESTING_DEPTH); + } + + /** + * Compiles the given expression with the supplied custom functions using a custom maximum nesting + * depth. + * + *

Functions must be {@code public static}, return {@code double} and can take from zero to 256 + * {@code double} parameters. + * + * @param sourceText The expression to compile + * @param functions map of String names to {@link MethodHandle}s + * @param maxNestingDepth the maximum depth of nesting (function calls, precedence - also + * implicit) + * @return A new compiled expression + * @throws ParseException on failure to compile + * @throws IllegalArgumentException if any of the functions does not have correct signature + * @throws IllegalStateException if the resulting expression class fails to link (e.g., + * complexity) + */ + public static Expression compile( + String sourceText, Map functions, int maxNestingDepth) + throws ParseException { + return compile(sourceText, functions, maxNestingDepth, false); } /** @@ -169,17 +209,24 @@ public static Expression compile(String sourceText, Map fu * * @param sourceText The expression to compile * @param functions map of String names to {@link MethodHandle}s + * @param maxNestingDepth the maximum depth of nesting (function calls, precedence - also + * implicit) * @param picky whether to throw exception on ambiguity or other internal parsing issues (this * option makes things slower too, it is only for debugging). * @return A new compiled expression * @throws ParseException on failure to compile + * @throws IllegalArgumentException if any of the functions does not have correct signature + * @throws IllegalStateException if the resulting expression class fails to link (e.g., + * complexity) */ - static Expression compile(String sourceText, Map functions, boolean picky) + static Expression compile( + String sourceText, Map functions, int maxNestingDepth, boolean picky) throws ParseException { for (MethodHandle m : functions.values()) { checkFunction(m); } - return new JavascriptCompiler(sourceText, functions, picky).compileExpression(); + return new JavascriptCompiler(sourceText, functions, maxNestingDepth, picky) + .compileExpression(); } /** @@ -188,9 +235,10 @@ static Expression compile(String sourceText, Map functions * @param sourceText The expression to compile */ private JavascriptCompiler( - String sourceText, Map functions, boolean picky) { + String sourceText, Map functions, int maxNestingDepth, boolean picky) { this.sourceText = Objects.requireNonNull(sourceText, "sourceText"); this.functions = Map.copyOf(functions); + this.maxNestingDepth = maxNestingDepth; this.picky = picky; } @@ -218,6 +266,12 @@ private Expression compileExpression() throws ParseException { throw cause; } throw re; + } catch (StackOverflowError soe) { + // we should catch this before in the visitor, but too high limits may cause this. + final var e = + new ParseException("Invalid expression '" + sourceText + "': Nesting level too deep", -1); + e.initCause(soe); + throw e; } try { @@ -225,10 +279,12 @@ private Expression compileExpression() throws ParseException { LOOKUP.defineHiddenClassWithClassData( classFile, constantsMap.keySet().stream().map(functions::get).toList(), true); return invokeConstructor(lookup, lookup.lookupClass(), externalsMap); - } catch (ReflectiveOperationException exception) { + } catch (ReflectiveOperationException | LinkageError e) { throw new IllegalStateException( - "An internal error occurred attempting to compile the expression (" + sourceText + ").", - exception); + "An internal error occurred attempting to compile the expression ('" + + sourceText + + "'). This may be caused by too complex code triggering limits inside the Java VM.", + e); } } @@ -261,6 +317,10 @@ private ParseTree getAntlrParseTree() { setupPicky(javascriptParser); } javascriptParser.setErrorHandler(new JavascriptParserErrorStrategy()); + // add listener to detect too deep nesting early, this may not catch all occurrences... (this + // will count root node, so add one recursion extra). + javascriptParser.addParseListener( + new JavascriptNestingDepthListener(sourceText, maxNestingDepth + 1)); return javascriptParser.compile(); } @@ -343,10 +403,18 @@ private JavascriptBaseVisitor newClassFileGeneratorVisitor( CodeBuilder gen, final Map externalsMap, final Map constantsMap) { + // we do an extra check on the tree to make sure the final structure is not too deeply nested: + final var nestingChecker = new JavascriptNestingDepthListener(sourceText, maxNestingDepth); // to completely hide the ANTLR visitor we use an anonymous impl: return new JavascriptBaseVisitor<>() { private final Deque typeStack = new ArrayDeque<>(); + private void visit(ParserRuleContext ctx) { + nestingChecker.enterEveryRule(ctx); + ctx.accept(this); + nestingChecker.exitEveryRule(ctx); + } + @Override public Void visitCompile(JavascriptParser.CompileContext ctx) { typeStack.push(TypeKind.DOUBLE); diff --git a/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptNestingDepthListener.java b/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptNestingDepthListener.java new file mode 100644 index 000000000000..cc74a32f6607 --- /dev/null +++ b/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptNestingDepthListener.java @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.expressions.js; + +import org.antlr.v4.runtime.ParserRuleContext; +import org.antlr.v4.runtime.tree.ErrorNode; +import org.antlr.v4.runtime.tree.ParseTreeListener; +import org.antlr.v4.runtime.tree.TerminalNode; + +final class JavascriptNestingDepthListener implements ParseTreeListener { + private final String sourceText; + private final int maxNestingDepth; + + private int depth = 0; + + public JavascriptNestingDepthListener(String sourceText, int maxNestingDepth) { + super(); + this.sourceText = sourceText; + this.maxNestingDepth = maxNestingDepth; + } + + @Override + public void visitTerminal(TerminalNode node) {} + + @Override + public void visitErrorNode(ErrorNode node) {} + + @Override + public void enterEveryRule(ParserRuleContext ctx) { + depth++; + if (depth > maxNestingDepth) { + throw JavascriptCompiler.newWrappedParseException( + "Invalid expression '" + + sourceText + + "': Nesting level too deep (>" + + maxNestingDepth + + ")", + ctx.start != null ? ctx.start.getStartIndex() : -1); + } + } + + @Override + public void exitEveryRule(ParserRuleContext ctx) { + depth--; + } +} diff --git a/lucene/expressions/src/test/org/apache/lucene/expressions/js/CompilerTestCase.java b/lucene/expressions/src/test/org/apache/lucene/expressions/js/CompilerTestCase.java index d7cc843efdac..b28eb480b197 100644 --- a/lucene/expressions/src/test/org/apache/lucene/expressions/js/CompilerTestCase.java +++ b/lucene/expressions/src/test/org/apache/lucene/expressions/js/CompilerTestCase.java @@ -34,6 +34,7 @@ protected Expression compile(String sourceText) throws ParseException { /** compiles expression for sourceText with custom functions list */ protected Expression compile(String sourceText, Map functions) throws ParseException { - return JavascriptCompiler.compile(sourceText, functions, true); + return JavascriptCompiler.compile( + sourceText, functions, JavascriptCompiler.DEFAULT_MAX_NESTING_DEPTH, true); } } diff --git a/lucene/expressions/src/test/org/apache/lucene/expressions/js/TestJavascriptCompiler.java b/lucene/expressions/src/test/org/apache/lucene/expressions/js/TestJavascriptCompiler.java index 94d180df5f9d..e9e9a3d093fa 100644 --- a/lucene/expressions/src/test/org/apache/lucene/expressions/js/TestJavascriptCompiler.java +++ b/lucene/expressions/src/test/org/apache/lucene/expressions/js/TestJavascriptCompiler.java @@ -17,6 +17,8 @@ package org.apache.lucene.expressions.js; import java.text.ParseException; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import org.apache.lucene.expressions.Expression; import org.junit.jupiter.api.Test; @@ -269,4 +271,53 @@ public void testVariableNormalization() throws Exception { x = compile("foo['\\\\'][\"\\\\\"]"); assertEquals("foo['\\\\']['\\\\']", x.variables[0]); } + + @Test + public void testRecursionDepth1() throws Exception { + int depth = 20000; + String src = "(".repeat(depth) + "1" + ")".repeat(depth); + assertEquals(JavascriptCompiler.DEFAULT_MAX_NESTING_DEPTH, assertRecursionLimit(src)); + } + + @Test + public void testRecursionDepth1a() throws Exception { + int depth = JavascriptCompiler.DEFAULT_MAX_NESTING_DEPTH; + String src = "(".repeat(depth) + "1" + ")".repeat(depth); + assertEquals(JavascriptCompiler.DEFAULT_MAX_NESTING_DEPTH, assertRecursionLimit(src)); + } + + @Test + public void testRecursionDepth2() throws Exception { + String src = "-".repeat(20000) + "1"; + assertEquals(JavascriptCompiler.DEFAULT_MAX_NESTING_DEPTH, assertRecursionLimit(src)); + } + + @Test + public void testRecursionDepth3() throws Exception { + String src = + IntStream.range(0, JavascriptCompiler.DEFAULT_MAX_NESTING_DEPTH + 1) + .mapToObj(Integer::toString) + .collect(Collectors.joining("+")); + assertEquals( + "error offset needs to be 0 due to recursion with depth first", + 0, + assertRecursionLimit(src)); + } + + @Test + public void testRecursionDepth3a() throws Exception { + String src = + IntStream.range(0, 20000).mapToObj(Integer::toString).collect(Collectors.joining("+")); + assertEquals( + "error offset needs to be 0 due to recursion with depth first", + 0, + assertRecursionLimit(src)); + } + + /** returns the error offset for further checks */ + private int assertRecursionLimit(String src) { + ParseException expected = expectThrows(ParseException.class, () -> compile(src)); + assertTrue(expected.getMessage(), expected.getMessage().contains("Nesting level too deep")); + return expected.getErrorOffset(); + } }