fix: reject end bounds beyond right.len() in compare_start_end by samukweku · Pull Request #55 · pyjanitor-devs/janitor-rs

samukweku · 2026-08-24T08:11:12Z

Summary

Fixes #53. compare_start_end_core validates negative and inverted start/end ranges (#52), but never checked a positive end against right.len(). A row with end > right.len() survives every existing check (non-negative, not the -1 sentinel, start < end) and then the inner loop indexes right[nn] out of bounds once nn == right.len() -- panicking (an unrecoverable PanicException on the Python side) instead of raising a normal ValueError or being silently skipped like every other malformed range.

Repro (from the issue): left=[1], right=[1], starts=[0], ends=[2], matches=[1,1] -- right has length 1, ends=[2] asks the loop to walk 0..2 and index right[1].

Fix

Adds one more condition alongside the existing per-row checks: *end as usize > right_len (safe -- by the time this runs, end is already proven non-negative by the earlier checks). Applied in both:

compare_start_end_core's per-row loop (the actual out-of-bounds site).
The #[pyfunction] wrapper's expected_matches_width precheck, which must reject the same rows the core will -- otherwise it demands a needlessly large matches tape for rows the core now silently skips.

A row that fails the check contributes zero ticks, matching how every other malformed range in this function already behaves (not an error) -- consistent with #52's precedent and the reporter's own "accepted or clean ValueError, never a panic" framing.

Why not `checked_range`?

The crate already has crate::aggs::checked_range for exactly this 0 <= start <= end <= len contract, used elsewhere (index_builder, aggs). I used it first, but benches/kernels.rs's compare_start_end group showed a real regression on this specific hot per-row-comparison loop:

n	before (plain checks)	with `checked_range`	regression
100	~278 ns	~347 ns	~25%
100,000	~246 µs	~321 µs	~30%

Swapping back to a plain comparison (same style the function already used for its other three checks) recovered the baseline exactly:

n	final (plain checks + end bound)
100	~272 ns (no regression vs. original)
100,000	~234 µs (no regression vs. original)

This tracks compare::op's own doc comment on indirection cost in this same hot path (25-46% slower from function-pointer dispatch, per #51) -- checked_range's Option-returning usize::try_from calls aren't free at this call frequency (once per row, inside the crate's tightest comparison loop), even though the logic is otherwise identical.

Test plan

cargo test --no-default-features -- 176 passed, 0 failed. New: end_equal_to_right_len_is_a_valid_inclusive_bound (boundary stays accepted), end_beyond_right_len_contributes_nothing_not_a_panic (core-level repro), end_beyond_right_len_does_not_panic_through_the_python_wrapper (wrapper-level repro, matching fix: reject non-sentinel negative starts in compare_start_end's width precheck #52's test style)
cargo clippy --all-targets --all-features -- -D warnings -- clean
cargo fmt --check -- clean
cargo bench --no-default-features -- compare_start_end -- before/after comparison above; final version shows no regression vs. pre-fix baseline
Built the wheel via maturin develop --release and ran the issue's exact repro end-to-end through Python: previously would panic, now returns (array([0, 0]), array([0]), 0) cleanly; confirmed the valid end == right.len() boundary still compares correctly

Note (not in scope here)

comp_first.rs and several other sibling files under src/compare/ have their own independent core loops with the same unguarded-end pattern -- and some lack even the negative-start check #52 added to comp.rs. Filing a follow-up issue for that separately; #53 was scoped to compare_start_end specifically.

Base branch is fix-compare-width-precheck-underflow (#52), not main -- this fix is additive on top of #52's negative-bound validation and would conflict/duplicate logic if applied to unpatched main.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S1gZKDRiZoBLZnXXXjW3gt

compare_start_end_core validated negative and inverted start/end ranges (PR #52) but never checked a positive `end` against `right.len()`, so a call with `ends[i] > right.len()` walked `right[nn]` out of bounds and panicked instead of raising a normal ValueError -- reported in issue #53. Adds the missing `*end as usize > right_len` check, in both the core loop and the pyfunction wrapper's matches-tape width precheck (which must reject the same rows, or it demands a needlessly large tape for rows the core now silently skips). Deliberately kept as a plain comparison rather than routed through the crate's existing `checked_range` helper: benchmarking `compare_start_end` (a hot, per-row comparison kernel) showed checked_range's Option-returning usize::try_from calls cost ~25-30% more wall time here than the equivalent plain comparisons, consistent with compare::op's own doc comment on indirection being measurably costly in this same hot path. No regression in the final version (benches/kernels.rs's compare_start_end group, n=100 and n=100,000, before vs after). Adds regression tests for end == right.len() (valid boundary), end == right.len() + 1 (issue #53's exact repro, at both the core and Python wrapper level), and confirms via a built wheel that the previously-panicking call now returns cleanly.

Continues #56 (see the previous commit for comp_ends.rs/comp_starts.rs/ comp_first*.rs): the 6 `!=`-comparison files -- comp_ne.rs, comp_ne_ends.rs, comp_ne_starts.rs ("matches already exist") and comp_ne_1st.rs, comp_ne_ends_1st.rs, comp_ne_starts_1st.rs ("matches does not exist yet") -- share the exact same two shapes and get the exact same two fixes: - "matches already exist" files take an externally-sized matches tape, so an invalid row is rejected with a PyValueError up front (silently skipping it would desynchronize the shared tape index for every subsequent row). - "_1st" files own their output array end to end, so an invalid row is silently skipped, consistent between the length precomputation and the main loop's skip condition -- matching comp.rs/comp_first.rs precedent. Same plain-comparison style throughout (not checked_range), per #55's measured perf finding. 17 new tests (Python-wrapper level, 2-3 per file), same coverage pattern as the previous commit. Verified via a rebuilt wheel that all 6 previously panicking reproductions now return cleanly (3 as a ValueError, 3 as an accepted empty result). Still remaining under #56: comp_posns.rs/comp_posns_ne.rs (indirection through `positions`, needs its own look).

Completes #56 -- the last 2 of the 13 files identified there. These have an extra layer of indirection the other 11 don't: the row's start..end range indexes into positions (not right), and positions[nn] is itself an index into right, so there are two independent bounds to fix: - start/end must be checked against positions.len(), not right.len(). result is presized to positions.len() directly (not derived from a width sum), so an invalid row is silently skipped, same convention as comp.rs/comp_first.rs. - positions[nn] (the indexer) must be checked against right.len() before being used to index right[indexer]/right_booleans[indexer]. The crate already treats indexer == -1 as a "no match" sentinel; broadened that same handling to any out-of-bounds indexer (negative-but-not-1, or >= right.len()), mirroring comp_no_range.rs's checked_index in spirit -- but as a plain comparison, since this check runs once per candidate position (the same hot-loop frequency #55 found checked_range costly at). 7 new tests (Python-wrapper level): out-of-bounds indexer, a negative-but-not-sentinel indexer, end beyond positions.len(), and a valid case comparing normally. Verified via a rebuilt wheel that all 3 reproductions return cleanly now instead of panicking. This closes out #56: all 13 files it flagged now validate their ranges.

Review finding on PR #58: every bound check added across the 13 files used the shape `(*end as usize) > right_len` -- casting the i64 value down to usize *before* comparing. On a 32-bit target (this crate's release matrix includes x86 and armv7, per .github/workflows/release.yml) usize is 32 bits, so a genuinely oversized end (e.g. 2**32 + 1) truncates to a small value before the check ever sees it. In most of the 11 non-posns files this produced silently wrong/truncated results rather than a panic, since the same truncated value was used consistently for both the check and the actual loop bound (self- consistent but wrong). Two places were worse: - comp_first_starts.rs / comp_ne_starts_1st.rs had no explicit upper-bound check at all -- they relied on "a Range with start > end is naturally empty" as the safety net, which breaks under wraparound: a huge start can truncate to something *smaller* than end, turning an out-of-range row into a seemingly valid one. - comp_posns.rs / comp_posns_ne.rs's indexer check could let a large positive indexer wrap to an in-range value and silently select the wrong row from `right` -- the reviewer's specific example. Fix: compare in i64 space (`right_len as i64`) instead of casting the i64 value to usize first -- a lossless widening cast on the length side, rather than a lossy narrowing cast on the value side. Same cost as before (one comparison either way), so this doesn't reopen #55's checked_range perf finding. comp_first_starts.rs/comp_ne_starts_1st.rs additionally gained the explicit upper-bound check they were missing. Not independently regression-tested: this class of bug only manifests when usize is narrower than i64, and this repo's CI only runs `cargo test` on ubuntu-latest (x86_64) -- the 32-bit release targets are cross-compiled but never test-executed. Verified instead by re-running the full suite (212 passed) and a rebuilt wheel end-to-end on this (64-bit) machine to confirm no regression in ordinary behavior.

Part of #56: compare_start_end_core (comp.rs) is not the only file in src/compare/ with unguarded start/end range indexing -- 13 sibling files share the pattern, none with any bound validation at all (not even the negative-bound check #52 added to comp.rs). This fixes the first 5: - comp_ends.rs / comp_starts.rs ("matches already exist"): these take a matches/counts tape already sized by the caller, so an invalid row can't be silently skipped the way comp.rs skips its own self-owned tape -- doing so would desynchronize the shared tape index for every row after it. Reject the whole call with a PyValueError up front instead. - comp_first_ends.rs / comp_first_starts.rs / comp_first.rs ("matches does not exist yet"): these own their output array end to end, so they follow comp.rs's precedent exactly -- an invalid row silently contributes zero ticks, computed consistently between the output-size precomputation and the main loop. comp_first_starts.rs needed the least: its start_..end loop was already safe for a bad start (an empty Range, not a panic) since end is always right.len(); only the length-sum precomputation (end - start_) could underflow. All bound checks are plain comparisons, not the crate's checked_range helper -- PR #55 measured checked_range costing ~25-30% more wall time than plain comparisons in this same per-row-comparison hot path, so that lesson is applied here preemptively rather than rediscovered per file. No dedicated benchmark exists for these 5 (unlike comp.rs, they have no extracted _core function benches/kernels.rs can call without a Python interpreter), so this is a qualitative call, not a measured one. 12 new tests (Python-wrapper level, matching each file's exported function directly), covering: the out-of-bounds repro, a negative-bound variant, and the valid boundary (start/end == right.len()) staying accepted. Verified via a built wheel that every one of the 5 previously- panicking calls now returns cleanly. Remaining files under #56: comp_ne.rs and its 5 siblings (comp_ne_1st, comp_ne_ends, comp_ne_ends_1st, comp_ne_starts, comp_ne_starts_1st), plus comp_posns.rs/comp_posns_ne.rs (double indirection through `positions`, needs its own look at what bound `nn`/`indexer` should each check against).

Continues #56 (see the previous commit for comp_ends.rs/comp_starts.rs/ comp_first*.rs): the 6 `!=`-comparison files -- comp_ne.rs, comp_ne_ends.rs, comp_ne_starts.rs ("matches already exist") and comp_ne_1st.rs, comp_ne_ends_1st.rs, comp_ne_starts_1st.rs ("matches does not exist yet") -- share the exact same two shapes and get the exact same two fixes: - "matches already exist" files take an externally-sized matches tape, so an invalid row is rejected with a PyValueError up front (silently skipping it would desynchronize the shared tape index for every subsequent row). - "_1st" files own their output array end to end, so an invalid row is silently skipped, consistent between the length precomputation and the main loop's skip condition -- matching comp.rs/comp_first.rs precedent. Same plain-comparison style throughout (not checked_range), per #55's measured perf finding. 17 new tests (Python-wrapper level, 2-3 per file), same coverage pattern as the previous commit. Verified via a rebuilt wheel that all 6 previously panicking reproductions now return cleanly (3 as a ValueError, 3 as an accepted empty result). Still remaining under #56: comp_posns.rs/comp_posns_ne.rs (indirection through `positions`, needs its own look).

Completes #56 -- the last 2 of the 13 files identified there. These have an extra layer of indirection the other 11 don't: the row's start..end range indexes into positions (not right), and positions[nn] is itself an index into right, so there are two independent bounds to fix: - start/end must be checked against positions.len(), not right.len(). result is presized to positions.len() directly (not derived from a width sum), so an invalid row is silently skipped, same convention as comp.rs/comp_first.rs. - positions[nn] (the indexer) must be checked against right.len() before being used to index right[indexer]/right_booleans[indexer]. The crate already treats indexer == -1 as a "no match" sentinel; broadened that same handling to any out-of-bounds indexer (negative-but-not-1, or >= right.len()), mirroring comp_no_range.rs's checked_index in spirit -- but as a plain comparison, since this check runs once per candidate position (the same hot-loop frequency #55 found checked_range costly at). 7 new tests (Python-wrapper level): out-of-bounds indexer, a negative-but-not-sentinel indexer, end beyond positions.len(), and a valid case comparing normally. Verified via a rebuilt wheel that all 3 reproductions return cleanly now instead of panicking. This closes out #56: all 13 files it flagged now validate their ranges.

Review finding on PR #58: every bound check added across the 13 files used the shape `(*end as usize) > right_len` -- casting the i64 value down to usize *before* comparing. On a 32-bit target (this crate's release matrix includes x86 and armv7, per .github/workflows/release.yml) usize is 32 bits, so a genuinely oversized end (e.g. 2**32 + 1) truncates to a small value before the check ever sees it. In most of the 11 non-posns files this produced silently wrong/truncated results rather than a panic, since the same truncated value was used consistently for both the check and the actual loop bound (self- consistent but wrong). Two places were worse: - comp_first_starts.rs / comp_ne_starts_1st.rs had no explicit upper-bound check at all -- they relied on "a Range with start > end is naturally empty" as the safety net, which breaks under wraparound: a huge start can truncate to something *smaller* than end, turning an out-of-range row into a seemingly valid one. - comp_posns.rs / comp_posns_ne.rs's indexer check could let a large positive indexer wrap to an in-range value and silently select the wrong row from `right` -- the reviewer's specific example. Fix: compare in i64 space (`right_len as i64`) instead of casting the i64 value to usize first -- a lossless widening cast on the length side, rather than a lossy narrowing cast on the value side. Same cost as before (one comparison either way), so this doesn't reopen #55's checked_range perf finding. comp_first_starts.rs/comp_ne_starts_1st.rs additionally gained the explicit upper-bound check they were missing. Not independently regression-tested: this class of bug only manifests when usize is narrower than i64, and this repo's CI only runs `cargo test` on ubuntu-latest (x86_64) -- the 32-bit release targets are cross-compiled but never test-executed. Verified instead by re-running the full suite (212 passed) and a rebuilt wheel end-to-end on this (64-bit) machine to confirm no regression in ordinary behavior.

samukweku mentioned this pull request Aug 24, 2026

[BUG] compare/ family: 13 files share compare_start_end's pre-#52/#53 unguarded range bug #56

Closed

samukweku force-pushed the issue-53-compare-end-upper-bound branch from d1e12bc to 7603087 Compare August 24, 2026 09:27

samukweku changed the base branch from fix-compare-width-precheck-underflow to main August 24, 2026 09:27

samukweku self-assigned this Aug 24, 2026

samukweku mentioned this pull request Aug 24, 2026

fix: reject/skip out-of-bounds ranges across all 13 compare/ files (closes #56) #58

Merged

4 tasks

samukweku merged commit 1a59297 into main Aug 24, 2026
17 checks passed

samukweku mentioned this pull request Aug 24, 2026

[BUG] compare_start_end's end-bound check truncates on 32-bit targets instead of rejecting oversized end #61

Closed

samukweku mentioned this pull request Aug 24, 2026

fix: compare end bound in i64 space, not after casting, in comp.rs (closes #61) #62

Merged

4 tasks

Navigation Menu

Uh oh!

fix: reject end bounds beyond right.len() in compare_start_end - #55

fix: reject end bounds beyond right.len() in compare_start_end#55
samukweku merged 1 commit into
mainfrom
issue-53-compare-end-upper-bound

samukweku commented Aug 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

samukweku commented Aug 24, 2026

Summary

Fix

Why not checked_range?

Test plan

Note (not in scope here)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Why not `checked_range`?