Skip to content

Fix crack_row_jet4 overflow panic when var-offset table wraps#8

Closed
Bradyok wants to merge 1 commit into
dominion525:mainfrom
Bradyok:fix/crack-row-jet4-overflow-on-wrap
Closed

Fix crack_row_jet4 overflow panic when var-offset table wraps#8
Bradyok wants to merge 1 commit into
dominion525:mainfrom
Bradyok:fix/crack-row-jet4-overflow-on-wrap

Conversation

@Bradyok

@Bradyok Bradyok commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

What

crack_row_jet4 (crates/jetdb/src/data.rs:243) panics in debug builds with attempt to add with overflow on rows where the declared var_col_count claims more variable columns than physically fit before vcc_pos. pos is already computed with wrapping_sub (correct), but the very next line — if pos + 2 > len — does a plain usize + 2, and when pos has wrapped to near usize::MAX that add itself panics before the length check can reject it.

Reproducer

Any .mdb whose variable-length offset table would extend past the start of the row. I hit it in the wild on Hy-Tek Meet Manager 8 .mdb exports — specifically the ScoringImprovement and ScoreLanes tables of a 2025 meet file.

The included regression test (crack_row_jet4_bogus_var_col_count_does_not_overflow) builds a 7-byte synthetic row with var_col_count = 100 which forces the loop well past vcc_pos. Without the fix it panics on the third iteration; with the fix it cleanly breaks out of the loop and returns Ok with the two valid offsets that actually fit.

Fix

Swap pos + 2 > len for pos.checked_add(2) so the bounds check itself is panic-safe:

match pos.checked_add(2) {
    Some(end) if end <= len => {}
    _ => break,
}

Semantically equivalent for every non-wrapped pos; the only difference is that a wrapped pos now breaks out of the loop instead of overflow-panicking in debug.

Verification

  • cargo test -p jetdb --lib — 620 tests pass (was 619, +1 for the new regression test)
  • cargo test -p jetdb --lib data::tests::crack_row_jet4 — 6/6 pass

Happy to adjust the test fixture, split the test into its own #[test], or add a fuzz-target-style input if you'd prefer.

When a row's declared var_col_count claims more variable columns
than physically fit before vcc_pos, the backward-walking offset
table loop computes `pos = vcc_pos.wrapping_sub(2 + i * 2)` which
wraps to a near-`usize::MAX` value. The bounds check that follows
was written as `if pos + 2 > len` — the plain `pos + 2` overflow-
panics in debug before the `> len` comparison can reject the
wrapped value.

Swap the unchecked add for `checked_add(2)` so the bounds check
itself is panic-safe; a wrapped `pos` now breaks cleanly out of
the loop.

Triggered in the wild by Hy-Tek Meet Manager 8 .mdb files —
specifically the `ScoringImprovement` and `ScoreLanes` tables.

Adds a targeted regression test against a synthetic row that
declares `var_col_count = 100` in a 7-byte buffer; the old code
panicked with `attempt to add with overflow`, the new code
returns `Ok` with as many offsets as actually fit.
@dominion525

Copy link
Copy Markdown
Owner

Hi @Bradyok — sorry for the long delay on this, almost two months is far too long.

The underlying overflow fix ended up landing in #9 with a different approach (checked_sub on the subtraction itself rather than checked_add on the bounds check, to avoid keeping a wrapped value in pos). Your regression test (crack_row_jet4_bogus_var_col_count_does_not_overflow) was so on-point that I cherry-picked it on top of main (commit c923116, on top of #9's merge 4e2c68b), with your authorship preserved.

Closing this PR as the underlying issue is now resolved on main. Really appreciate the detailed reproducer — the var_col_count=100 synthetic row is the kind of compact, targeted test that future-me will thank you for.

dominion525 pushed a commit that referenced this pull request Jun 20, 2026
Cherry-picked the regression test from #8. The underlying overflow itself was addressed in #9 via a different approach (checked_sub), but this var_col_count=100 / 7-byte reproducer is independently useful for guarding against future regressions.

(cherry picked from commit 2d924b5)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants