feat(engine): add local functions and lambdas to Painless - #88
Conversation
OpenSearch's UBI sample dashboards filter via a Supplier-style boolean
helper, which needs two Painless features xerj didn't support:
- Top-level local function declarations: `<type> name(<type> arg, ...) { ... }`
- Lambda literals: `(a, b) -> expr` / `(a, b) -> { stmts }`, stored as
a closure value and invoked either by calling the function/variable
name directly, or via any `.method(args)` call on a closure value
(`s.get()`, `fn.apply(x)`, `pred.test(x)`, ...) — the method name is
ignored, only positional args matter, covering Supplier/Function/
BiFunction/Predicate etc. without hard-coding each functional
interface. Closures run in a fresh scope seeded only with their bound
parameters, with no access to the caller's other locals.
Adapted to the current `ParsedExpr`/`eval_access_chain` parser
structure: a lambda literal is a parse-time leaf (its own body is
depth-guarded independently via the normal statement-parsing path), and
closure invocation is wired into `eval_member_value` (the `.method()`
dispatch point) and the `Expr::Call` case.
The new PainlessValue::Closure variant made two existing conversions
(aggs.rs's painless_value_to_json, es_compat.rs's painless_to_json)
non-exhaustive; both now map a closure to Value::Null, the same
fail-closed treatment already used for script errors — a script's
top-level result is never meaningfully a function value.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PRCbyt7Y2HDyhG1tbQTeL
|
Reviewed for inclusion in v1.0.0-rc.9. Holding it — there are two shapes here that abort the process, and I reproduced both in the real crate under the release profile. Detail below so it's actionable rather than just a "no". Method note: every finding was sent to an independent verifier told to refute it, and I re-ran the two most serious ones myself against The design is closer than it looks
What it doesn't prevent is self-application: a closure passed as an argument is bound in the callee's scope, so Blocker 1 — a 156-byte script aborts the process
Root cause: The guard's own doc comment (:938-941) says it is "Incremented on entry to Worth knowing how narrowly this hides: the simple shape Blocker 2 — no step or time budget; depth bounds the call path, not the call tree89 bytes, Before this PR no Painless construct could loop or recurse at all, so "there is no step budget" was fine. Adding closures changes that premise. Also confirmed
What would make this shippableA dedicated call-depth counter in For context on why this got the scrutiny: this repo has already shipped and fixed an unauthenticated stack-overflow DoS from nested SQL |
Addresses maintainer review findings on the original closures PR — the
two crash-class findings verified directly before fixing:
- Confirmed by diffing against upstream: EvalDepthGuard::enter is
called in exactly one place, eval_expr. exec_stmt, exec_body and
call_closure charge nothing. Statement nesting was previously bounded
once by MAX_PARSE_DEPTH at parse time and could never be re-entered
at eval time — closures change that by re-entering exec_stmt from
inside eval_expr on every call, turning MAX_PARSE_DEPTH into a
per-call-level multiplier instead of a total.
Fixes both shapes the review demonstrated:
- Self-application with a nested-statement body (`f(f, n)` where f's
body has several nested `if`s) can re-enter exec_stmt far more
times than the expression-eval budget accounts for. Fixed with a
new call_depth counter (MAX_CALL_DEPTH = 32), independent of
eval_depth, charged once per call_closure invocation.
- An exponential call tree (`g(g,n-1) + g(g,n-1) + g(g,n-1) +
g(g,n-1)`) never exceeds a call *depth* of ~n, so a depth counter
alone doesn't bound it — measured at 262,144 invocations for n=9.
Fixed with a call_count budget (MAX_CALL_COUNT = 10,000) across
the whole script evaluation.
- try_parse_lambda swallowed every error from parsing a lambda body,
including the MAX_PARSE_DEPTH sentinel, and silently backtracked —
defeating check_script_limits's up-front 400 for a script whose
lambda body is what makes it too deep. Once `(params) ->` is
consumed there is no other valid parse in this grammar, so any
error from the body onward is now propagated instead of swallowed.
- Every closure value deep-cloned its entire body AST on construction
(`body.clone()` on a `Vec<Stmt>`) — with the call-count fix above
already bounding the invocation count, this is no longer a path to
unbounded memory, but it's still needless allocation on every call
that passes a closure as an argument. Switched closure bodies to
Rc<Vec<Stmt>>, shared from the AST node at parse time, so cloning a
closure value is a refcount bump instead of an AST copy.
- Also added: argument-count checking in call_closure (a wrong-arity
call now errors instead of silently defaulting missing args to
Null / dropping extras).
Not addressed here (agreed lower-severity follow-ups): no capture, no
hoisting, no recursion by name — a script relying on lexical capture
still silently sees an undefined-identifier error rather than the
value, which is at least fail-closed rather than fail-open, but isn't
the ES-correct behavior either.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PRCbyt7Y2HDyhG1tbQTeL
|
Both blockers confirmed before fixing — diffed
Not addressed (agree, lower severity, tracking separately): no capture/hoisting/recursion-by-name — a script relying on lexical capture still errors on the undefined identifier rather than seeing the value, which is fail-closed but not ES-correct. Full suite green: 🤖 Generated with Claude Code |
|
Housekeeping, not a review comment. We've just added a Contributor License Agreement to the project (#93). Until now contributions here were covered only by Apache-2.0 §5's inbound=outbound clause, which gives no explicit patent grant — worth tightening for a project this size. Once #93 merges, a
That PR is the signature — it comes from your own account, so the commit history is the record. To be explicit about something: we did not add anyone to the signed list on their behalf, including you. That file asserts a person has signed, and that's not ours to assert for someone else — hence the ask rather than a quiet edit. Sorry for the extra step on work that's already in flight. Thanks for the contributions. |
… score (#116) A Painless script that recursed past MAX_CALL_DEPTH did not fail — it returned a WRONG SCORE. PR #88 bounded the recursion correctly and both process-abort repros stay dead; the defect is the failure mode. The scoring and matching paths (apply_function_score, apply_rescore, terms_set min_should_match, script-bucketed terms aggs) return bare f32/Value with no error channel, so they mapped the limit error to a neutral value and served it as a success. Captured verbatim from the pre-fix tree: `{"max_score":0.0,"hits":[{"_score":0.0,...}]}` with a 200 status, from a scoring script that never ran. Two failure classes must not collapse, and the error type now distinguishes them: - Unsupported or unparseable script syntax keeps degrading quietly. Seven of the nine eval_painless call sites swallow that error on purpose, because the ES-compat surface depends on an out-of-subset script scoring neutrally rather than 400-ing the request. None of those fallbacks changed. - A resource limit (call depth, eval depth, invocation count, source size) means the script was understood and we refused to finish it. Publishing 0.0 / [] there is an answer that looks real and is not. `is_resource_limit_error` marks exactly this class, and the call-depth sentinel is split from the invocation-count one so the two remedies (recurse less vs. call less) are distinguishable. The transport is a task-local fault sink (`with_script_fault_capture`). Threading a sink through doc_matches_query/score_query_against_doc would touch ~90 call sites; a thread-local would mis-attribute a fault across an await. Index::search installs the sink and reports the fault on SearchResult.script_failure, alongside timed_out — and re-raises it into any enclosing scope on the way out, so a sub-search cannot hide a fault from the request that contains it. Faulted results are never cached, so degraded scores are not replayed. _search, _msearch, _search/template, _msearch/template, scroll and async_search return an ES-shaped script_exception 400 instead of the degraded page. _count, _reindex, _delete_by_query and _update_by_query refuse rather than act on a selection a fail-closed script truncated — under-deleting and calling it a completed run is the same defect wearing a different hat. The _search handler additionally wraps its own script evaluation (script_fields, runtime_mappings emit scripts run during response assembly, outside the search task), which previously dropped a failed field silently. MAX_CALL_DEPTH stays at 32, and that is measured rather than assumed. Probing the stack address at each call level in a release build, with the recursive call nested inside the closure body's blocks so every level also pays that body's exec_stmt frames: 2,272 bytes/level at one nested block, 40,720 at the parser's 90-block maximum — 1,262,320 bytes, 1.20 MiB of the 2 MiB tokio worker stack, at depth 32. Raising the ceiling would overflow, so the issue's "make it high enough" option is not available; lifting it safely needs body nesting charged against one stack budget, not a larger constant. The harness ships behind the off-by-default `stackprobe` feature. Rc<Vec<Stmt>> becomes Arc, making PainlessValue Send/Sync. A/B'd on the same benchmark, three runs each, reported as the closure/closure-free ratio because wall-clock on a shared host swings ±40%: Arc 3.328, 3.334, 3.224 against Rc 3.367, 3.344, 2.360. The ~1% gap is an order of magnitude below Rc's own spread — a closure call allocates a fresh HashMap scope, which dominates the refcount either way. Regression coverage is at the wire, in tests that compile against the unfixed engine: script_score, script_fields, script-bucketed aggs, delete_by_query and cache non-poisoning all fail on the pre-fix tree with the 200s quoted above, while the out-of-subset-script test passes before and after — the guard against fixing this by making every unparseable script a 400. Engine-level tests add the multi-thread block_in_place path, rescore, terms_set, Send/Sync, and both abort repros on a 2 MiB stack including the 1.20 MiB worst case. Closes #97
Implements the Elasticsearch `script` query: a Painless predicate evaluated per document, wired into the doc-scan path. MISSING FIELDS FAIL CLOSED, THE WAY MODERN ELASTICSEARCH DOES. `doc['missing'].value` errors rather than coercing to a default. Since ES 7.0 reading `.value` off an empty `ScriptDocValues` throws IllegalStateException; 6.x returned a type default with a deprecation warning and 7.0 removed that fallback. Coercing would mean a script filter with a typo'd field name matches every document, which is the defect this shape exists to avoid. The guard idiom ES documents still works, because that idiom is `doc['x'].size() == 0 ? <default> : doc['x'].value`, not a null comparison: `.size()`, `.length` and `.empty` are total on a missing field and the ternary only evaluates the branch it takes. `==` and `!=` are null-aware, since `params` is a real map in Painless and an unsupplied key reads as null. COMPILED SCRIPTS ARE CACHED AND THE CACHE IS BOUNDED. A script is evaluated once per document, so re-parsing per document made the per-document cost scale with the script's SIZE rather than its complexity. The cache key is attacker-supplied source, so the bound is a security property rather than an optimisation detail: 128 entries and 512 KiB of source, per thread. It is thread-local rather than global because the AST is not Send. THE DOC-SCAN TIMEOUT POLL IS TIGHTER. `scan_stored_section_into` polled its deadline every 4 096 documents on the assumption that per-document work is a few microseconds. Measured in release, a benign 64 KiB script is 385 us/doc, so that interval was a ~1.6 s stretch in which a `timeout` could not take effect. It now polls every 256, bringing that to ~98 ms. That is an improvement, not a bound, and the comment says so. A deliberately adversarial script of legal size measures 1.27 s/doc, ~325 s per poll interval, and nothing bounds it because there is no per-evaluation CPU budget. Tracked separately. PR #88's closure guards are untouched: MAX_CALL_DEPTH, MAX_CALL_COUNT and the string-allocation ceiling all still trip closed, and the cached AST does not leak the per-evaluation call budget across evaluations.
Summary
OpenSearch's UBI sample dashboards filter via a Supplier-style boolean helper, which needs two Painless features xerj didn't support:
<type> name(<type> arg, ...) { ... }(a, b) -> expr/(a, b) -> { stmts }, stored as a closure value and invoked either by calling the function/variable name directly, or via any.method(args)call on a closure value (s.get(),fn.apply(x),pred.test(x), ...) — the method name is ignored, only positional args matter, covering Supplier/Function/BiFunction/Predicate etc. without hard-coding each functional interface. Closures run in a fresh scope seeded only with their bound parameters, with no access to the caller's other locals.Adapted to the current
ParsedExpr/eval_access_chainparser structure: a lambda literal is a parse-time leaf (its own body is depth-guarded independently via the normal statement-parsing path), and closure invocation is wired intoeval_member_value(the.method()dispatch point) and theExpr::Callcase.The new
PainlessValue::Closurevariant made two existing conversions (aggs.rs'spainless_value_to_json,es_compat.rs'spainless_to_json) non-exhaustive; both now map a closure toValue::Null, the same fail-closed treatment already used for script errors — a script's top-level result is never meaningfully a function value.This is one of three independent PRs from the same investigation (the others: #86
flat_objectalias, #87scriptquery support). Combined with #87, this unblocks the "Top Searches Without Results" UBI dashboard panel, which filters via exactly this shape of script.Test plan
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warningscargo test -p xerj-engine -p xerj-query -p xerj-api --lib(351 passed; the 2 failures insnapshot_path_security_testsare pre-existing onmain, unrelated to this change)🤖 Generated with Claude Code
https://claude.ai/code/session_014PRCbyt7Y2HDyhG1tbQTeL