From 82ae2fb5ca0d5710b249619453ff45342f0b910a Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 21 Jul 2026 16:24:44 -0400 Subject: [PATCH] fix(config): reject the saturating i64 upper boundary in int_from_f64 Review round 1, finding 1. `i64::MAX as f64` rounds UP to 2^63 = 9223372036854775808.0, one greater than `i64::MAX`. The guard used `>`, so a value of exactly 2^63 passed validation and `v as i64` then saturated it to 9223372036854775807 --- the registry silently stored a different number than the caller wrote. The bounds are asymmetric on purpose, and the fix must not be applied to both ends: `i64::MIN as f64` IS exactly -2^63 and round-trips, so the lower comparison stays `<`. Tightening it in sympathy would wrongly reject a legitimate value. Three tests: the boundary is rejected (fails against the `>` form, bite-verified), `i64::MIN` is still accepted, and the largest representable integer below the boundary (2^63 - 1024) still converts. Also from review round 1, finding 2, which does NOT reproduce: the concern was that because `set_local` never calls `maybe_freeze_after_init`, a `set_local` as the first post-init operation would defer the lazy freeze and let a later `StartupOnly` write slip through. It cannot --- `set` and `reset` call `maybe_freeze_after_init` as their first statement, ahead of the definition lookup and the mutator, so the freeze always lands before the check within the same call. No code change; the ordering is now pinned by a test that drives exactly that sequence (post-init `set_local` on a Live key, then a `StartupOnly` write) and asserts the write is still refused and that the refused set is itself what triggered the freeze. Gates: fmt, clippy -D warnings, --lib (1687), --lib --features crdt (1861), lua54 backend, config_registry_acceptance (13), and the full workspace sweep (2799 tests, exit 0). git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/config_registry.rs | 49 +++++++++++++++++++++++++++++++++- src/lua_bindings/config.rs | 54 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/src/config_registry.rs b/src/config_registry.rs index 964dbd2..f089c6a 100644 --- a/src/config_registry.rs +++ b/src/config_registry.rs @@ -356,7 +356,13 @@ impl ConfigValue { value: v, }); } - if v.fract() != 0.0 || v < i64::MIN as f64 || v > i64::MAX as f64 { + // The bounds are deliberately ASYMMETRIC. `i64::MIN as f64` is + // -2^63, which round-trips exactly, so `<` correctly admits it. + // `i64::MAX as f64` rounds UP to 2^63 --- one more than + // `i64::MAX` --- so a `>` here would admit exactly 2^63 and then + // `v as i64` would saturate it to `i64::MAX`, silently storing a + // different number than the caller wrote. `>=` rejects it. + if v.fract() != 0.0 || v < i64::MIN as f64 || v >= i64::MAX as f64 { return Err(ConfigError::NonIntegral { name: name.to_owned(), value: v, @@ -2121,4 +2127,45 @@ mod tests { assert!(matches!(err, ConfigError::NonFiniteNumber { .. })); } } + + #[test] + fn int_from_f64_rejects_the_saturating_upper_boundary() { + // Review round 1, finding 1. `i64::MAX as f64` rounds UP to + // 2^63 = 9223372036854775808.0, one more than i64::MAX. With a + // `>` guard this value passes validation and `v as i64` then + // SATURATES to 9223372036854775807 --- the registry silently + // stores a different number than the caller asked for. This + // test fails against the `>` form. + let boundary = i64::MAX as f64; + let err = ConfigValue::int_from_f64("x", boundary).unwrap_err(); + assert!( + matches!(err, ConfigError::NonIntegral { .. }), + "2^63 must be rejected, not saturated to i64::MAX" + ); + // Anything beyond it too. + assert!(ConfigValue::int_from_f64("x", boundary * 2.0).is_err()); + } + + #[test] + fn int_from_f64_accepts_the_exact_lower_boundary() { + // The bounds are asymmetric on purpose: unlike the upper one, + // `i64::MIN as f64` IS exactly i64::MIN and round-trips, so + // tightening the lower comparison to `<=` alongside the upper + // `>=` would wrongly reject a legitimate value. + let lo = i64::MIN as f64; + assert_eq!( + ConfigValue::int_from_f64("x", lo).unwrap(), + ConfigValue::Int(i64::MIN), + "i64::MIN is representable and must still be accepted" + ); + } + + #[test] + fn int_from_f64_accepts_the_largest_representable_integer_below_the_boundary() { + // The next f64 below 2^63 is 2^63 - 1024, which is a valid i64. + // Pins that the `>=` fix did not over-reject the top of range. + let below = (i64::MAX as f64) - 1024.0; + let got = ConfigValue::int_from_f64("x", below).unwrap(); + assert_eq!(got, ConfigValue::Int(9_223_372_036_854_774_784)); + } } diff --git a/src/lua_bindings/config.rs b/src/lua_bindings/config.rs index 43e0423..960576e 100644 --- a/src/lua_bindings/config.rs +++ b/src/lua_bindings/config.rs @@ -1253,6 +1253,60 @@ mod tests { assert!(err.to_string().contains("startup-only"), "{err}"); } + #[test] + fn a_set_local_first_does_not_open_a_startup_only_write_window() { + // Review round 1, finding 2. `set_local` does not call + // `maybe_freeze_after_init`, so the concern was that a + // `set_local` as the first post-init operation defers the freeze + // and lets a subsequent `StartupOnly` write slip through. + // + // It cannot: `set` and `reset` call `maybe_freeze_after_init` as + // their FIRST statement, before the definition lookup and before + // the mutator runs, so the freeze always lands ahead of the + // check in the very same call. This test drives that exact + // ordering -- post-init `set_local` on a Live key, then a + // `StartupOnly` write -- and asserts the write is still refused. + let (lua, reg) = fresh(); + let flag = InitCompleteFlag::new(); + lua.set_app_data(flag.clone()); + run( + &lua, + r#"pmacs.config.define{ name="editing.live-one", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + run( + &lua, + r#"pmacs.config.define{ name="lsp.root-markers", description="d", type="boolean", default=true, mutability="startup" }"#, + ) + .unwrap(); + + flag.set_complete(); + assert!( + !reg.borrow().is_frozen(), + "precondition: nothing has triggered the lazy freeze yet" + ); + + // The first post-init operation is a set_local on a Live key. + let buf = BufferId::next(); + lua.globals().set("__buf", BufferIdLua(buf)).unwrap(); + run( + &lua, + "pmacs.config.set_local(__buf, 'editing.live-one', false)", + ) + .unwrap(); + + // The StartupOnly write must still be refused. + let err = run(&lua, "pmacs.config.set('lsp.root-markers', false)").unwrap_err(); + assert!( + err.to_string().contains("startup-only"), + "a set_local first must not open a write window: {err}" + ); + assert!( + reg.borrow().is_frozen(), + "the set that was refused is itself what triggered the freeze" + ); + } + #[test] fn define_startup_only_always_rejects_set_local() { let (lua, _reg) = fresh();