From b8639130d9b8b62b1b9f5da5d409e75d8186dc22 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 6 Aug 2026 23:08:10 +0200 Subject: [PATCH] fix(protocol): saturating_mul undercounted the percentage silently byte_pos.saturating_mul(100) does not merely lose precision at the top of the range --- it collapses the numerator to a constant. u64::MAX times 100 saturates to u64::MAX, and u64::MAX / u64::MAX is 1, so a cursor at the very end of a maximal buffer read 1%. Wrong in the worst way available: in range, plausible, and passing every test. percent_is_always_in_range asserted only p <= 100, which Percent(1) satisfies perfectly. The sweep even included the exact (u64::MAX, u64::MAX) pair and reported success, because it never asked what the answer should be. Computes in u128 now. u64::MAX * 100 fits with room to spare, so the product is exact and the only remaining clamp is the genuine one --- a caller reporting a cursor past the end still gets 100%, never above. large_byte_counts_stay_accurate is the correctness witness the range sweep could not be. It bites: against the old arithmetic it fails with left: Percent(1) right: Percent(100) while percent_is_always_in_range keeps passing, which is the point of adding it rather than extending that one. It also pins u64::MAX/2 at 49% and u64::MAX/4 at 24%, and includes u64::MAX/100 + 1 --- the smallest position whose scaling overflows u64, and therefore the first input the old code got wrong. percent_is_always_in_range keeps its sweep and gains a note about what it does not prove, so the next reader does not mistake bounded for correct. Gates: fmt, workspace clippy -D warnings, diff --check, pmacs-protocol --lib 25/0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- pmacs-protocol/src/scroll.rs | 57 +++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 4 deletions(-) diff --git a/pmacs-protocol/src/scroll.rs b/pmacs-protocol/src/scroll.rs index d56df81..bab0689 100644 --- a/pmacs-protocol/src/scroll.rs +++ b/pmacs-protocol/src/scroll.rs @@ -85,10 +85,18 @@ pub fn classify( if byte_len == 0 { return ScrollPosition::All; } - // Saturating, then clamped: a caller that reports a cursor - // past the end (a stale readout mid-edit) gets 100%, not a - // wrapped or panicking percent. - let pct = byte_pos.saturating_mul(100) / byte_len; + // Widen to u128 before scaling. `saturating_mul` was wrong + // here, not merely inelegant: it *silently undercounts*. + // `u64::MAX * 100` saturates to `u64::MAX`, so a cursor at + // the end of a maximal buffer divided out to 1% — a wrong + // answer that looked safe because it stayed in range. + // + // `u64::MAX * 100` fits in u128 with room to spare, so the + // product is exact and the only clamp left is the genuine + // one below. + let pct = u128::from(byte_pos) * 100 / u128::from(byte_len); + // Clamped for a caller that reports a cursor past the end + // (a stale readout mid-edit): 100%, never above. ScrollPosition::Percent(u8::try_from(pct.min(100)).unwrap_or(100)) } } @@ -156,6 +164,12 @@ mod tests { } /// Percent never leaves `0..=100`, for any input. + /// + /// **In range is not the same as correct**, which is why + /// [`large_byte_counts_stay_accurate`] exists beside this. This + /// sweep passed against a `saturating_mul` that silently reported + /// 1% for a cursor at the end of a maximal buffer — a wrong answer + /// that satisfies every assertion here. #[test] fn percent_is_always_in_range() { for pos in [0_u64, 1, 7, 99, 100, 1_000, u64::MAX / 2, u64::MAX] { @@ -166,4 +180,39 @@ mod tests { } } } + + /// The percentage stays *accurate* where `u64` arithmetic would + /// overflow, not merely bounded. + /// + /// `byte_pos * 100` exceeds `u64::MAX` for any position above + /// `u64::MAX / 100`. Saturating there collapses the numerator to a + /// constant, so the quotient stops tracking the position at all: + /// `u64::MAX / u64::MAX` is 1, and the readout said **1%** at the + /// very end of the buffer. + #[test] + fn large_byte_counts_stay_accurate() { + assert_eq!( + classify(false, false, u64::MAX, u64::MAX), + ScrollPosition::Percent(100), + "the end of a maximal buffer is 100%, not 1%" + ); + assert_eq!( + classify(false, false, u64::MAX / 2, u64::MAX), + ScrollPosition::Percent(49), + "halfway through a maximal buffer, floored" + ); + assert_eq!( + classify(false, false, u64::MAX / 4, u64::MAX), + ScrollPosition::Percent(24), + "a quarter through, floored" + ); + // The smallest position whose scaling overflows u64 — the first + // input the old implementation got wrong. + let first_overflowing = u64::MAX / 100 + 1; + assert_eq!( + classify(false, false, first_overflowing, u64::MAX), + ScrollPosition::Percent(1), + "correct by arithmetic here, not by saturation" + ); + } }