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) <noreply@anthropic.com>
This commit is contained in:
Levi Neuwirth 2026-07-21 16:24:44 -04:00
parent 6844262495
commit 82ae2fb5ca
2 changed files with 102 additions and 1 deletions

View File

@ -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));
}
}

View File

@ -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();