feat(expr)!: bounded range values, budgeted range equality, exact int/float comparison by mwiebe · Pull Request #276 · OpenJobDescription/openjd-rs

mwiebe · 2026-07-22T18:47:10Z

What was the problem/requirement? (What/Why)

Range expressions (range_expr('1-100')) describe potentially huge integer
sequences symbolically — a billion-frame range is stored as just three
numbers (start, end, step). That's what makes huge frame ranges cheap, but
it means every operation on a range — length, indexing, equality, slicing —
is arithmetic on those three numbers, and near the edges of the 64-bit
domain that arithmetic could produce incorrect results. Since expressions
come from untrusted job templates, a template could obtain wrong answers
or consume operations and memory in excess of the evaluator's specified
limits:

Wrong answers near the 64-bit limits. A range of 2^63 elements
overflowed the length field, so len(R) reported 0, an empty list
compared equal to it, and max(R) returned a mid-range element.
Equality expanded the range. [1, 2] == R for a 100M-element range
allocated an 800 MB vector to say "false", outside every budget.
Budget gaps. Comprehensions, list(range_expr(...)), and reverse
slices allocated their full result before the memory-limit check —
and a comprehension transiently held both its input and its result,
roughly doubling its footprint — an OOM on a constrained worker
instead of a clean error.
Incoherent int/float comparison. 9223372036854775807.0 (really
the float 2^63) compared equal to i64::MAX (2^63 − 1) — two different
numbers; Python says False. Equality and ordering also disagreed.
Unvalidated construction. IntRange's public fields and
non-validating deserialization allowed step: 0, making the length
computation divide by zero.

What was the solution? (How)

The key design decision: bound range values to |v| < 2^62. Rather than
patching each overflow with wider 128-bit arithmetic (earlier iterations
tried; edge cases kept leaking), endpoints and steps of magnitude ≥ 2^62
are rejected at construction with an integer-overflow error. In that
domain every derived quantity — spans, counts, index products — fits
exactly in plain 64-bit arithmetic, making the whole class of boundary
bugs unrepresentable. 2^62 ≈ 4.6e18 is nine orders of magnitude beyond any
realistic frame range; the divergence from Python's arbitrary-precision
endpoints is documented in specs/expr/range-expr.md (Value Bounds).

On that foundation:

One equality definition, charged as it goes (equals_charged backs
Rust == and the language's ==/!=/in): list↔range equality walks
the range at most one element past the list's length, and every list
comparison charges the operation budget per element actually compared.
Exact int↔float comparison matching Python, applied consistently to
equality, ordering, and hashing, so ==/< always agree.
Budgets enforced while building, not after. Comprehensions iterate
ranges lazily — never materializing them — and check the growing result
against the memory limit each iteration; list() and reverse slices
pre-check their projected allocation; reverse-slice bounds clamp the way
CPython clamps them.
Invariants enforced at construction: IntRange fields are private
(accessors added); deserialization and from_ranges re-validate.
CHUNK[INT] list ranges reject out-of-bound values with a clear error —
at job creation and again in the iterator constructor (the parameter
space is publicly constructible) — instead of panicking during chunk
iteration.

What is the impact of this change?

Range operations now return the mathematically correct answer or a clean,
budget-respecting error: len/R[-1]/max are exact for every accepted
range on every target (including 32-bit wasm); [1, 2] == range_expr(...)
answers in two comparisons instead of 800 MB; boundary int/float
comparisons match Python; out-of-bound range values fail at parse time
where they previously produced wrong lengths, wrong elements, or panics.

How was this change tested?

Have you run the unit tests? Yes — cargo test --workspace: openjd-expr
3,114 integration + 301 unit tests (~65 new regression tests),
openjd-model 1,502 + 341. Clippy (-D warnings) and rustfmt clean.
Full OpenJD conformance suite: 350/350 EXPR tests pass (the wider
suite's only failures are wrap-* tests being fixed on another branch).

Was this change documented?

Are relevant docstrings in the code base updated? Yes —
specs/expr/range-expr.md gains a Value Bounds section; values.md,
function-library.md, and public-api.md updated to match. Quality report
items 1, 6, 16, PA2 struck through; a follow-up (canonical range↔range
equality) recorded under item 9.

Is this a breaking change?

Yes (pre-1.0, conventional-commit ! + BREAKING CHANGE footer):
RangeExpr::from_values returns Result; IntRange fields are private
(use the new accessors) and deserialization/from_ranges now reject
invalid input; cumulative_lengths() returns &[u64]; range values of
magnitude ≥ 2^62 are parse errors — a deliberate, documented divergence
from the Python reference that no realistic template hits (conformance
suite unchanged).

Does this change impact security?

Defensive hardening of resource budgeting against untrusted templates:
closes paths that allocated memory in excess of the configured limit
(including a comprehension that transiently held roughly double its
checked footprint), an unbudgeted ~9e18-iteration slice walk,
overflow-driven wrong-answer paths, a deserialization-reachable
divide-by-zero, and an iteration-time panic reachable through publicly
constructible parameter spaces. No new files, permissions, or trust boundaries.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

leongdl

Reviewed the full diff with the call stacks traced (range construction → bound check, ==/in → equals_charged, comprehension → lazy iteration + incremental memory checks). Verified locally on the PR head: cargo test -p openjd-expr -p openjd-model — all 5,264 tests pass.

The bound-the-domain-instead-of-widening-the-arithmetic decision is the right call, and the capacity-slack projection in the comprehension (checking the post-doubling Vec footprint before push allocates it) is unusually careful budget work. I found no correctness bugs — I tried and failed to break the debug_assert_eq!(count, 2) slice fallback (spacing ≥ 2^62 in a domain of width ≤ 2^63−2 forces count ≤ 2), int_float_cmp at exactly −2^63, empty-range max(), and the 32-bit saturation ordering in list_from_range.

Four small non-blocking cleanups inline below.

leongdl

One follow-up from the review: a maintainability measurement on the largest function this PR grows.

leongdl · 2026-07-23T03:10:13Z

-        for item in &items {
+        for item in iter {
            self.count_op()?;
+            let memory_baseline = self.current_memory;


Maintainability data point: with this change, eval_listcomp is now 182 lines (145 code, 32 comment) with a cyclomatic complexity of ~40 (measured by decision-point count: if/while/for/match arms/&&/||/?). Even discounting the ? early-returns as low-cognitive-load, that's well above clippy's cognitive-complexity default of 25 — this function is getting hard to maintain.

The comments documenting the memory-accounting invariants help a lot, but the budget bookkeeping (baseline save/restore, result_bytes, the capacity-slack projection before push) is interleaved with the iteration logic — and the same projected-capacity pre-check pattern now appears in three places (here, slice_range in comparison.rs, and list_from_range in list.rs).

Suggested follow-up (non-blocking): extract a small BudgetedVec-style helper that owns the result-bytes tracking and post-doubling capacity projection. It would shrink this function by ~30 lines, deduplicate the three sites, and put the trickiest invariant behind one independently-testable abstraction.

Follow-up 5bb85b7 created the suggested budgeted vec, centralizing some of this tricky accounting from a bunch of places.

leongdl

Nit comments, did a pass for the dead code, comments etc.

I'm getting really concerned some of the code has cyclomatic complexity up to 30+ and function lengths of 500+.

…/float comparison Hardening for symbolic ranges (quality report items 1, 6, 16, PA2), refined through ten independent review passes. Range value bounds (the load-bearing design choice): endpoints and steps are restricted to magnitudes strictly below 2^62 (MAX_RANGE_VALUE_MAGNITUDE, re-exported at the crate root). Inputs at or beyond the bound — via parsing, IntRange::new, from_values, or from_ranges — raise an integer overflow error at construction. Bounding the domain makes every derived quantity exactly representable in 64 bits: the widest endpoint span is below 2^63, so element counts fit i64/u64 exactly, len() never saturates, index products and normalization intermediates stay in i64, and the packed length field cannot collide with the contiguous-display flag. The cumulative-length bisect table (mirroring the Python reference's _range_length_indices) is always exact, so indexed access is O(log m) unconditionally, with no widened (i128/u128) arithmetic anywhere. from_values keeps far-apart values (gap >= 2^62) as singleton chunks and uses checked run extrapolation. Python accepts arbitrary-precision endpoints; the divergence is documented in specs/expr/range-expr.md (Value Bounds). The full 2023-09 EXPR conformance suite passes. Construction invariants enforced everywhere: IntRange's fields are private with start()/end()/step() accessors; deserialization and RangeExpr::from_ranges re-validate through IntRange::new, so a zero step (which made len_u64() divide by zero) or a descending range cannot exist at runtime. CHUNK[INT] list ranges reject out-of-bound values with a path-annotated validation error — at job creation and re-validated in StepParameterSpaceIterator::new (the parameter space is publicly constructible and deserializable) — instead of panicking when generated chunks are built during iteration; plain INT list ranges still accept full i64. List<->range equality decides length mismatches in O(1) with no charge (range lengths are exact arithmetic under the bounded domain), then walks the range lazily, charged per element comparison. One budgeted equality definition: ExprValue::equals_charged is the single core backing equals()/PartialEq (infallible no-op charge) and the evaluator's ==/!=/in operators (operation budget). Charges are levied per element comparison actually performed, after the O(1) length check, for every arm — generic mixed-variant lists, same-variant typed lists, and the lazy list<->range walk (O(list_len + 1), no materialization) — so comparisons decidable by length alone are free and early mismatches cost only the steps taken. This replaces the arm that collected the entire range into a Vec (1.78 s / 800 MB for a 100M-element range) on every equals()/PartialEq path. Exact int<->float comparison, matching Python: equality requires the float to be an integer exactly representable in i64 whose converted value equals the int (float_as_exact_i64); ordering uses the same exact semantics (int_float_cmp) so ==/</> agree at the 2^63 boundary (9223372036854775807.0 == i64::MAX is false AND i64::MAX < 9223372036854775807.0 is true). The same rule drives Hash's integral-float handling in both the Float and ListFloat arms, preserving the Eq/Hash contract. Containment by value equality: __contains__/__not_contains__ accept any item type for lists ((list[T1], T2)) and ranges ((range_expr, T1)), so '1' in [1, 2, 3] is false rather than a type error and 1.0 in range_expr('1-3') is true. Budget completeness: comprehensions iterate both lists (borrowing ListIter, no wholesale copy of the iterable) and ranges (lazily, never materialized) in place, save/restore each iteration's memory baseline so per-item transients cannot erode accounting for still-live values, and check the growing result against the memory limit each iteration, accounting for the result Vec's projected post-growth capacity (not just logical element bytes) so push's doubling allocation is charged before it happens; list(range_expr(...)) and reverse slices preallocate to their exact pre-checked size so buffer growth can never overshoot the checked projection; list(range_expr(...)) and reverse range slices charge operations and pre-check the projected allocation before materializing; reverse slices clamp resolved negative start/stop to the -1 sentinel (matching CPython's PySlice_AdjustIndices) in both the range walk and the shared compute_slice_indices helper used by list and string slices; ExprValue::coerce to list[int] rejects ranges larger than the default operation limit (it runs outside any evaluation budget); openjd-for-js parseRangeExpr rejects ranges too large to materialize. BREAKING CHANGE: RangeExpr::from_values now returns Result (it validates the value bound); IntRange's start/end/step fields are private (use the new accessor methods); IntRange::Deserialize and RangeExpr::from_ranges reject invariant-violating input that was previously accepted silently; cumulative_lengths() returns &[u64]; range expressions with endpoints or steps of magnitude >= 2^62 are rejected with an integer overflow error where extreme values were previously accepted. specs: range-expr.md gains the Value Bounds section (rationale, exactness guarantees, Python divergence, singleton-chunk slicing note) and an accurate struct sketch; values.md documents the budgeted equality core and exact int<->float rules; function-library.md documents the containment signatures and their static-validation trade-off; public-api.md documents MAX_RANGE_VALUE_MAGNITUDE, the accessors, and the changed signatures. Quality report: items 1, 6, 16, and PA2 struck through; the RangeExpr canonical-equality recommendation recorded under item 9 for future work alongside expr-eq-hash. A related openjd-model fix (wrap-hook timing fields validating with the runtime's scope) ships separately on fix/model-wrap-hook-timing-scope. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Inline the range slicing implementation, improve defensive containment diagnostics, and document non-empty range invariants. Centralize result-vector memory accounting in a tested BudgetedVec helper shared by comprehensions and range materialization. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Correct the stale IntRange visibility rationale and retain direct coverage for both Deserialize validation and from_ranges defense in depth. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Adds a coverage-guided libFuzzer suite (via cargo-fuzz) over the crates that parse, decode, or evaluate untrusted input, a CI workflow that runs it on PRs and merges to main, and a hard "all untrusted inputs are fuzzed" gate. Motivation: the quality-evaluation reports kept re-discovering the same class of defect by hand — arithmetic overflow and char-boundary panics reachable from attacker-controlled expression text, templates, and manifests. Encoding that class as executable checks stops it from being re-litigated every review cycle. Twelve fuzz targets, each asserting "any input returns Ok or Err — never panic, abort, or hang": expr_parse, expr_evaluate (both POSIX and Windows path formats), range_expr, int_range_new, range_expr_slice, expr_type_parse, format_string, copy_symbol_value, model_decode, model_create_job, snapshot_decode, snapshot_ops. The fuzz crate is outside the root workspace (its own empty [workspace] table) so the stable build/test/clippy/MSRV jobs are untouched; it builds only under a pinned nightly with AddressSanitizer, overflow-checks, and debug-assertions on. Curated seed corpora (mined from the crates' own tests, sample templates, and i64/multibyte edge cases) live in fuzz/seeds/ and are committed. Fuzz-coverage gate (scripts/check_fuzz_coverage.py + fuzz/fuzz_coverage.toml, runs on stable in the Compliance CI job — no nightly, no fuzz build): every public function taking a &str / &[u8] / serde_json::Value / &Path parameter must be classified in the registry as either fuzzed-by-a-target or explicitly not-untrusted-with-a-reason. The check enforces no orphan/phantom targets, no dangling classifications, and — the ratchet — that a newly-added untrusted-input entry point fails CI until triaged. 108 input-shaped public functions are classified today (69 fuzzed, 39 not-untrusted). Bug found and fixed (with a regression test): path_starts_with under the Windows path format sliced a path by the base string's byte length, panicking when that offset fell inside a multibyte char (expr report finding X8) — found once expr_evaluate exercised the Windows path format. Fixed with a byte-slice comparison (equivalent for ASCII case-folding, boundary-safe). Note: earlier revisions of this branch also fixed RangeExpr/IntRange integer overflows the fuzzer found; those are now superseded by upstream OpenJobDescription#276 (bounded range values), so this branch rebases onto that and drops the redundant range changes, keeping the fuzz targets that guard the area. All openjd-expr tests pass; cargo clippy -D warnings is clean workspace-wide. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>

Adds a coverage-guided libFuzzer suite (via cargo-fuzz) over the crates that parse, decode, or evaluate untrusted input, a CI workflow that runs it on PRs and merges to main, and a hard "all untrusted inputs are fuzzed" gate. Motivation: the quality-evaluation reports kept re-discovering the same class of defect by hand — arithmetic overflow and char-boundary panics reachable from attacker-controlled expression text, templates, and manifests. Encoding that class as executable checks stops it from being re-litigated every review cycle. Twelve fuzz targets, each asserting "any input returns Ok or Err — never panic, abort, or hang": expr_parse, expr_evaluate (both POSIX and Windows path formats), range_expr, int_range_new, range_expr_slice, expr_type_parse, format_string, copy_symbol_value, model_decode, model_create_job, snapshot_decode, snapshot_ops. The fuzz crate is outside the root workspace (its own empty [workspace] table) so the stable build/test/clippy/MSRV jobs are untouched; it builds only under a pinned nightly with AddressSanitizer, overflow-checks, and debug-assertions on. Curated seed corpora (mined from the crates' own tests, sample templates, and i64/multibyte edge cases) live in fuzz/seeds/ and are committed. scripts/run_fuzz.sh derives the target list from `cargo fuzz list`, so new targets are picked up by CI automatically with nothing to maintain. Fuzz-coverage gate (scripts/check_fuzz_coverage.py + fuzz/fuzz_coverage.toml, runs on stable in the Compliance CI job — no nightly, no fuzz build): every public function taking a &str / &[u8] / serde_json::Value / &Path parameter must be classified in the registry as either fuzzed-by-a-target or explicitly not-untrusted-with-a-reason. The check enforces no orphan/phantom targets, no dangling classifications, and — the ratchet — that a newly-added untrusted-input entry point fails CI until triaged. 108 input-shaped public functions are classified today (69 fuzzed, 39 not-untrusted). Also fixes one bug the suite found, with a regression test: path_starts_with under the Windows path format sliced a path by the base string's byte length, panicking when that offset fell inside a multibyte char (expr report finding X8), surfaced once expr_evaluate exercised the Windows path format. Fixed with a byte-slice comparison (equivalent for ASCII case-folding, boundary-safe). Note: earlier revisions of this branch also fixed RangeExpr/IntRange integer overflows the fuzzer found; those are now superseded by upstream OpenJobDescription#276 (bounded range values), so this branch rebases onto that and drops the redundant range changes, keeping the fuzz targets that guard the area. All openjd-expr tests pass; cargo clippy -D warnings is clean workspace-wide. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>

mwiebe requested a review from a team as a code owner July 22, 2026 18:47

github-actions Bot added the waiting-on-maintainers label Jul 22, 2026

mwiebe force-pushed the fix/expr-symbolic-ranges-equality branch from 84e28f3 to 312486d Compare July 22, 2026 18:51

mwiebe mentioned this pull request Jul 22, 2026

fix: Close remaining RFC 0008 wrap-action gaps in hook scoping, cleanup, and preflight #277

Merged

leongdl reviewed Jul 23, 2026

View reviewed changes

Comment thread crates/openjd-expr/src/range_expr.rs Outdated

Comment thread crates/openjd-expr/src/range_expr.rs Outdated

Comment thread crates/openjd-expr/src/functions/comparison.rs Outdated

Comment thread crates/openjd-expr/src/functions/math.rs Outdated

leongdl reviewed Jul 23, 2026

View reviewed changes

mwiebe force-pushed the fix/expr-symbolic-ranges-equality branch from 312486d to db97473 Compare July 23, 2026 19:12

mwiebe added 2 commits July 23, 2026 12:37

test(expr): clarify range invariant validation

2ff841d

Correct the stale IntRange visibility rationale and retain direct coverage for both Deserialize validation and from_ranges defense in depth. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

mwiebe changed the title ~~feat(expr)!: bounded range values, budgeted range equality, exact int/float comparison~~ feat(expr)!: bounded range values, budgeted range equality, exact int/float comparison Jul 23, 2026

leongdl approved these changes Jul 23, 2026

View reviewed changes

mwiebe enabled auto-merge (squash) July 24, 2026 17:33

crowecawcaw approved these changes Jul 24, 2026

View reviewed changes

mwiebe merged commit d021564 into OpenJobDescription:main Jul 24, 2026
22 checks passed

github-actions Bot mentioned this pull request Jul 24, 2026

chore: release #278

Open

mwiebe deleted the fix/expr-symbolic-ranges-equality branch July 24, 2026 17:58

crowecawcaw mentioned this pull request Jul 24, 2026

chore(fuzz): add cargo-fuzz suite and untrusted-input coverage gate #279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(expr)!: bounded range values, budgeted range equality, exact int/float comparison - #276

feat(expr)!: bounded range values, budgeted range equality, exact int/float comparison #276
mwiebe merged 3 commits into
OpenJobDescription:mainfrom
mwiebe:fix/expr-symbolic-ranges-equality

mwiebe commented Jul 22, 2026

Uh oh!

leongdl left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leongdl left a comment

Uh oh!

leongdl Jul 23, 2026

Uh oh!

mwiebe Jul 23, 2026

Uh oh!

leongdl left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mwiebe commented Jul 22, 2026

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

Does this change impact security?

Uh oh!

leongdl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leongdl left a comment

Choose a reason for hiding this comment

Uh oh!

leongdl Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

mwiebe Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

leongdl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants