fix: assorted numeric overflow fixes#2462
Conversation
The SSH session expiry calculation cast a u64 TTL to i64 and multiplied by 1000 without bounds checking. A configured TTL larger than i64::MAX / 1000 overflows and panics in debug builds. Use i64::try_from with saturating_mul and saturating_add so any out-of-range TTL clamps to i64::MAX milliseconds. Signed-off-by: Andrew White <andrewh@cdw.com>
The Podman health-check interval multiplied a u64 seconds value by 1_000_000_000 to produce nanoseconds. A configured interval above u64::MAX / 1e9 overflows. Use saturating_mul so huge values clamp to u64::MAX nanoseconds instead of panicking. Signed-off-by: Andrew White <andrewh@cdw.com>
parse_duration_to_ms parsed the numeric part as i64 and multiplied it by a fixed multiplier, which can overflow for large user inputs such as '100000000000000h'. Use checked_mul and return a clear error instead of panicking. Add a regression test. Signed-off-by: Andrew White <andrewh@cdw.com>
After parsing a duration, the CLI subtracted it from the current Unix epoch milliseconds. A duration larger than now_ms underflows. Use saturating_sub so the start timestamp clamps to 0 instead of panicking in debug builds. Signed-off-by: Andrew White <andrewh@cdw.com>
format_age subtracted created_secs from now before checking whether the timestamp was in the future, so a future or skewed timestamp underflowed before the guard could return '-'. Check created_secs > now before subtracting, and add regression tests for future, zero, and negative timestamps. Signed-off-by: Andrew White <andrewh@cdw.com>
|
I have read the DCO document and I hereby sign the DCO. |
parse_cpu_to_microseconds checked that the input cores value was finite, but the product cores * 100_000 could still overflow to infinity (e.g. '1e300'). An infinite product cast to u64 saturates to u64::MAX, producing a bogus quota. Reject the value when the product is non-finite or exceeds u64::MAX. Add a regression test. Signed-off-by: Andrew White <andrewh@cdw.com>
parse_cpu_limit and parse_memory_limit parsed user-supplied f64 values and multiplied them by large constants before casting to i64. A huge input could make the product infinite, silently saturating to i64::MAX. Check that the rounded product is finite and within i64 range before casting, and return a clear failed-precondition error. Add regression tests. Signed-off-by: Andrew White <andrewh@cdw.com>
andrewwhitecdw
changed the title
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This is project-valid small, concentrated numeric-safety work across existing CLI, TUI, server, Docker, and Podman paths.
Head SHA: 260664b542b829e612093b7ed7e617192daed804
Review findings:
- The Docker millicore path still uses
saturating_mul(1_000_000), so oversized millicore input silently clamps while the equivalent core input errors. Please use checked multiplication, return the same “too large” error, and add a regression test. - Four line-specific boundary findings are included inline.
Docs: No Fern update is needed for the current arithmetic-only changes. If remediation introduces a documented maximum Podman health-check interval, update docs/reference/gateway-config.mdx.
E2E: Deferred until review findings are resolved; the current patch is arithmetic-only and focused boundary unit tests are sufficient.
Next state: gator:in-review
| Ok(Some((cores * 1_000_000_000.0).round() as i64)) | ||
| let nano_cpus = (cores * 1_000_000_000.0).round(); | ||
| #[allow(clippy::cast_precision_loss)] | ||
| if !nano_cpus.is_finite() || nano_cpus < i64::MIN as f64 || nano_cpus > i64::MAX as f64 { |
There was a problem hiding this comment.
gator-agent
Warning (CWE-190): i64::MAX as f64 rounds to 2^63, so an intermediate equal to 2^63 passes this > check and the cast silently saturates to i64::MAX. Reject this boundary (for example with >=) or use an exact checked conversion, and add a 2^63 regression test.
| Ok(Some((amount * multiplier).round() as i64)) | ||
| let bytes = (amount * multiplier).round(); | ||
| #[allow(clippy::cast_precision_loss)] | ||
| if !bytes.is_finite() || bytes < i64::MIN as f64 || bytes > i64::MAX as f64 { |
There was a problem hiding this comment.
gator-agent
Warning (CWE-190): The same 2^63 boundary issue applies here: a quantity producing exactly 2^63 bytes passes validation and clamps to i64::MAX. Reject equality or use an exact checked conversion, with a boundary regression test.
| } | ||
| let micros_f = cores * 100_000.0; | ||
| #[allow(clippy::cast_precision_loss)] | ||
| if !micros_f.is_finite() || micros_f > u64::MAX as f64 { |
There was a problem hiding this comment.
gator-agent
Warning (CWE-190): u64::MAX as f64 rounds to 2^64, so an intermediate equal to 2^64 passes this check and the cast saturates to u64::MAX. Reject this boundary (for example with >=) or use checked exact parsing, and test the 2^64 boundary.
| interval: config.health_check_interval_secs * 1_000_000_000, | ||
| interval: config | ||
| .health_check_interval_secs | ||
| .saturating_mul(1_000_000_000), |
There was a problem hiding this comment.
gator-agent
Warning (CWE-190): saturating_mul converts overflow to u64::MAX, but Podman’s inherited health-check interval uses a signed Go time.Duration in nanoseconds. Invalid configuration can therefore survive startup and fail later during sandbox creation. Validate the interval against i64::MAX / 1_000_000_000 during driver initialization, use checked multiplication, and add a boundary test.
johntmyers
added
the
gator:in-review
|
/ok to test 260664b |
2 participants
Summary
Eight small, independent numeric overflow/underflow fixes in production code paths that handle user or external configuration values:
u64toi64and multiplied by 1000, overflowing for large configured TTLs.u64seconds value by1_000_000_000, overflowing for huge intervals.parse_duration_to_msmultiplied the parsedi64by a fixed multiplier, overflowing for inputs like100000000000000h.openshell logs --since <duration>subtracted the parsed duration fromnow_ms, underflowing when the duration exceeded the current Unix epoch.format_agesubtractedcreated_secsfromnowbefore checking for a future timestamp, so clock-skewed/future values underflowed before the guard could return"-".parse_cpu_to_microsecondschecked thatcoreswas finite, butcores * 100_000could overflow to infinity and saturate tou64::MAX.parse_cpu_limitmultiplied parsedcoresby1_000_000_000; huge inputs overflowed to infinity and saturated toi64::MAX.parse_memory_limitmultiplied parsedamountby a suffix multiplier; huge inputs overflowed to infinity and saturated toi64::MAX.Related Issue
N/A — small fixes found during code review.
Changes
sandbox.rs:i64::try_from(...).unwrap_or(i64::MAX).saturating_mul(1000), thensaturating_addcontainer.rs(podman):saturating_mul(1_000_000_000)for health-check interval; reject non-finite or oversized CPU quota productscommands/common.rs:checked_mulinparse_duration_to_ms; regression testrun.rs:saturating_subfor--sincestart timestamplib.rs(tui): guardcreated_secs > nowbefore subtracting informat_age; regression testslib.rs+tests.rs(docker): reject non-finite or out-of-range CPU/memory limit products; regression testsTesting
mise run pre-commitpasses (mise unavailable in this environment; ran equivalentcargo fmt+cargo clippy --all-targetson all touched crates — clean)cargo test -p openshell-cli parse_duration_to_ms_rejects_overflow→ passedcargo test -p openshell-tui format_age→ 2 passedcargo test -p openshell-driver-podman parse_cpu→ 5 passedcargo test -p openshell-driver-docker parse_cpu_limit→ 2 passedcargo test -p openshell-driver-docker parse_memory_limit→ 2 passedChecklist