Add (configurable) guards for complexity of expressions

[uschindler]

...so we fail withParseException instead of StackOverflowError or LinkageErrors when the expression is too complex.

Description

Any application that compiles caller-influenced expressions through JavascriptCompiler inherits an undeclared crash path. Because StackOverflowError is a VirtualMachineError, generic try/catch (ParseException) or even catch (Exception) around compile() does not contain it; only an explicit catch (StackOverflowError) / catch (Error) does.

Concrete downstream example: Elasticsearch exposes the expressions language as a scripting engine. A single small request containing a nested-parenthesis expression reaches JavascriptCompiler.compile(), the StackOverflowError escapes Elasticsearch's ParseException-only handler, and Elasticsearch's fatal-error handler halts the JVM. This is partly an Elasticsearch defense-in-depth gap (it catches this same error for its Painless engine but not for expressions), and that is being addressed separately with Elasticsearch.

The stopped JVM is actually not a bug in Lucene (Elasticsearch kills itsself when the stack overflows or on any other error), but as this is partly caused by the ANTLR lexr/parser to work recursive, this should be handled.

This PR adds a configurable limit on the nesting in Lucene expressions (defaults to 250 1024) which is checked on parsing and building the ParseTree -- but also adds a "last safety": When a StackOverflowError happens during building the final class file, the parsing fails with a ParseException, too. If others think the additional listener is too much overhead, we can remove the configurable limit and only trigger on a stack overflow.

The configurable limit has the pro that it is not JVM dependent. An expression will fail consistently with a given limit and not depending on the call stack and config of the JVM. This may also be used by software like Elasticsearch to limit the complexity of expressions. We may also add a limit on how many method calls are allowed at a later stage.

The PR also adds another catch for LinkageError that is rethrown as (documented) IllegalStateException if the parser created a class file which does not load at all (e.g., due to the size of bytecode or a bug in our parser).

It also updates documentation to clarfiy which exceptions are thrown.

Thansk to @skraft9 for the hint about this problem!

[msokolov]

do we need to make the nestingDepth configurable? Given that this is a safety check, 250 really seems like plenty

[uschindler]

Actually if you read the whole thing, you may notice that this not only affects "nesting" of expressions. Also a long "1 + 2 + 3 + 4 + ..... + 10000" will trigger the failure. I am not sure if 250 is enough for all cases, so I'd like to have it configurable (at least for a while, so people can tune it).

[uschindler]

But I agree, if we can remove it and have a sane default - I am fine. But if we don't want to have it, catching "StackOverFlowException" might be enough, too.

[msokolov]

hm, I did not realize a single expression with lots of + would be included. You know at one time we attempted to use Lucene's expression language to encode machine-learned decision trees that had thousands and thousands of nodes. Ultimately we concluded this was bad idea and implemented a custom handling function,, but the the reason was not StackOverflow, but some internal limit in the JDK on byte code. I'm forgetting exactly what was limited - maybe the total size of byte code, number of instructions, or number of identifiers? Anyway there can be cases with very large expressions, so I get your point that 250 may not be enough. Still, this feels something like BooleanQuery's 1024 limit or vector search's 1024-dimension limit - can we pick a large default (maybe 1024 is our magic number)? But I don't fundamentally object to exposing the config parameter, just wonder if anyone would use it

[uschindler]

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!

[uschindler]

Anyways maybe 1024 is a better default limit! In my testing 2048 sometimes causes stack overflows already (especially in testing environment while the "picky" mode is enabled).

[uschindler]

I updated javadocs to clarify what nesting depth means.

[uschindler]

This can be backported if needed (it does not depend on the ASM vs. Classfile library used for compiling. It may just create minor merge conflicts.

[msokolov]

looks good to me!

[rmuir]

I think its fine.

alternative names might be "max depth" or "max tree depth" but "max nesting depth" isn't the worst and will work.

[uschindler]

Thanks for review, I added a small improvement to not traverse the tree two times. The depth check is now done inside the visitor!