uschindler commented on code in PR #16423:
URL: https://github.com/apache/lucene/pull/16423#discussion_r3657810879
##########
lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java:
##########
@@ -155,10 +162,35 @@ 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<String,
MethodHandle> 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.
+ *
+ * <p>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)
+ * @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<String, MethodHandle> functions, int
maxNestingDepth)
Review Comment:
Yeah, this is now also handled in this PR. If the JVM refuses the class file
due to internal limitations, an IllegalStateException is thrown, too (instead
of an error).
There may also be other limitations in the JVM that could cause a stack
overflow also at runtime. Basically those two "identical" expressions
(semantically), will cause a different bytecode. The first and second one have
same AST and are more optimal, but the third one consumes much more space
during execution:
- `a + b + c + d`: bytecode: push a, push b, add, push c, add, push d, add
- `((a + b) + c) +d`: bytecode: push a, push b, add, push c, add, push d, add
- `a + (b + (c + d))`: bytecode push a, push b, push c, push d, add, add, add
The last one pushes all arguments onto stack and calls three times "add".
This is consuming more stack than the first two variants (with same AST). But
as first and second are identical in the AST, you see why the first one has
implicit precedence included and therefor may cause a stack overflow during
parsing.
You see it is complicated and therefor a hardcoded nesting limit, but
possibly also a "number of tokens" limit should be enforced (configurable).
The number of tokens limit can be added, if we agree on it.
I hope this helps to understand what's going on!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
