docs(engine): correct the record on why the bucket_script expression bound exists - #117
Merged
Conversation
The bound landed in #115 with three comments asserting that this evaluator is recursive and can be made to abort the process. It is not, and it cannot. The commit message was corrected before that PR merged; the source comments were not, so main currently documents a crash that no released version had. What is actually there: `eval_script_expr` is `tokenize_script` -> `shunting_yard` -> `evaluate_rpn`, three flat loops over a `Vec`, and `?` is not a token at all (`tokenize_script` returns `None` on it). Measured with the bound removed on a 2 MiB stack: the 80,001-byte / 20,000-ternary source returns `None`, and a 1,000,000-deep parenthesised expression returns `Some(1.0)`. Nothing overflows. The recursion arrives with the `eval_tokens` / `find_ternary_split` evaluator, which re-enters once per `?`. The bound is real and worth keeping; it is simply pre-emptive, landing ahead of that evaluator rather than behind a live defect. The comments now say so. No functional change. Only doc comments move.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No functional change. Doc comments only.
The bound landed in #115 with three comments asserting that the
bucket_scriptexpression evaluator is consumed recursively and that an unauthenticated request can exhaust the stack and abort the process. That claim was corrected in the commit message before #115 merged, but not in the source, somaincurrently documents a crash no released version had.What is actually on this tree:
eval_script_expristokenize_script→shunting_yard→evaluate_rpn, three flat loops over aVec, and?is not a token at all (tokenize_scriptreturnsNoneon it). Measured with the bound removed, on a 2 MiB stack:Ok(None)Ok(None)Ok(Some(1.0))The million-paren case is a 2 MB expression, 30×
MAX_SCRIPT_LEN. Nothing overflowed.The recursion arrives with the
eval_tokens/find_ternary_splitevaluator (PR #95), which re-enters once per?. The bound is correct and stays — it is simply pre-emptive, landing ahead of that evaluator rather than behind a live defect. The three comments now say that.Worth doing rather than leaving: this repo has already had to spend a commit correcting one overclaim (
64d0d55, "the path-traversal finding was a stale-binary false alarm"). A comment in shipped source that names a process-kill which never existed is the same mistake in a more durable place.