fix: assorted numeric overflow fixes - #2462
Conversation
|
I have read the DCO document and I hereby sign the DCO. |
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 |
Addresses gator-agent review feedback on NVIDIA#2462: - Docker parse_cpu_limit / parse_memory_limit: use >= i64::MAX as f64 so inputs rounding to 2^63 are rejected instead of saturating. - Podman parse_cpu_to_microseconds: use >= u64::MAX as f64 so inputs rounding to 2^64 are rejected instead of saturating. - Podman health-check interval: replace saturating_mul with checked_mul and reject values exceeding i64::MAX nanoseconds. - Add regression tests for the i64::MAX, u64::MAX, and health-check interval boundaries. Signed-off-by: Andrew White <andrewh@cdw.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid, concentrated numeric-safety work across existing CLI, TUI, server, Docker, and Podman paths.
Head SHA: 04139ebeb6c91f7ef3b69b9fc384bb8d57f2e8ea
Thanks @andrewwhitecdw. I checked the latest commit against each prior finding. The Docker memory boundary and Podman health-check bound are resolved, and the Docker/Podman floating-point guards now reject equality. However, two new tests do not exercise the equality values they claim, and the earlier Docker millicore finding remains unresolved at crates/openshell-driver-docker/src/lib.rs:2678: saturating_mul(1_000_000) still silently maps oversized input to i64::MAX. Replace it with checked multiplication, return the existing “too large” error, and test 9223372036855m as rejected while 9223372036854m remains accepted.
Four additional line-specific findings are included inline. No Fern docs update is needed for these arithmetic-only changes. E2E is not required; focused boundary unit coverage is appropriate.
Next state: gator:in-review
| ) | ||
| .into_diagnostic()?; | ||
| now_ms - dur_ms | ||
| now_ms.saturating_sub(dur_ms) |
There was a problem hiding this comment.
gator-agent
Warning: saturating_sub does not clamp a duration older than the Unix epoch to zero; it returns a negative timestamp whenever the subtraction remains representable. Negative durations are also accepted and produce future timestamps. Reject negative durations, then clamp an overlong duration to zero (for example, now_ms.checked_sub(dur_ms).unwrap_or(0).max(0)) and add regression tests for both cases.
| fn parse_cpu_limit_rejects_i64_max_boundary() { | ||
| // 9_223_372_037 cores * 1e9 rounds to 2^63, which used to pass the > check | ||
| // and silently saturate to i64::MAX. It must now be rejected. | ||
| let err = parse_cpu_limit("9223372037").unwrap_err(); |
There was a problem hiding this comment.
gator-agent
Warning: This does not test the equality boundary: 9_223_372_037 * 1e9 is already above 2^63, so changing the guard back from >= to > would still pass. Test 9223372036.854776, whose floating-point product rounds exactly to 2^63.
| // 184_467_440_737_095_520 cores * 100_000 rounds to 2^64, which used | ||
| // to pass the > check and silently saturate to u64::MAX. It must now | ||
| // be rejected. | ||
| assert_eq!(parse_cpu_to_microseconds("184467440737095520"), None); |
There was a problem hiding this comment.
gator-agent
Warning: This input is approximately 1,000 times the 2^64 boundary, contrary to the comment, so it does not protect the >= regression. Use 184467440737095.51616; multiplying that parsed value by 100_000 rounds to exactly 2^64.
| let now_ms = current_time_ms(); | ||
| let expires_at_ms = if state.config.ssh_session_ttl_secs > 0 { | ||
| now_ms + (state.config.ssh_session_ttl_secs as i64 * 1000) | ||
| let ttl_ms = i64::try_from(state.config.ssh_session_ttl_secs) |
There was a problem hiding this comment.
gator-agent
Warning (CWE-20/CWE-613): Saturating an unrepresentable TTL to i64::MAX silently turns invalid configuration into a practically non-expiring SSH credential. Validate the TTL with checked conversion, multiplication, and addition, return an error when it cannot be represented, and add a boundary test.
johntmyers
added
gator:blocked
|
/ok to test 04139eb |
|
/ok to test 314cb46 |
BlockedHead SHA: Thanks @andrewwhitecdw. The latest commit says it addresses the prior numeric-boundary findings, including the Docker millicore overflow, exact floating-point boundary tests, Podman interval validation, negative CLI durations, and SSH-session TTL validation. Gator is blocked because GitHub currently reports this PR as conflicting with Next action: please rebase on or merge the latest |
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>
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>
Addresses gator-agent review feedback on NVIDIA#2462: - Docker parse_cpu_limit / parse_memory_limit: use >= i64::MAX as f64 so inputs rounding to 2^63 are rejected instead of saturating. - Podman parse_cpu_to_microseconds: use >= u64::MAX as f64 so inputs rounding to 2^64 are rejected instead of saturating. - Podman health-check interval: replace saturating_mul with checked_mul and reject values exceeding i64::MAX nanoseconds. - Add regression tests for the i64::MAX, u64::MAX, and health-check interval boundaries. Signed-off-by: Andrew White <andrewh@cdw.com>
- Docker CPU millicores: use checked multiplication and reject overflow. - Docker CPU/memory f64 boundaries: use exact equality tests (9223372036.854776 cores, 8192Pi memory) and add millicore boundary test. - Podman CPU: correct boundary test value to 184467440737095.51616 for exact 2^64 microseconds. - Podman health-check interval: pre-validate against i64::MAX / 1_000_000_000 seconds and use checked multiplication; add boundary acceptance test. - CLI duration parsing: reject negative durations; clamp overlong --since durations to zero instead of producing future timestamps. - Server SSH TTL: use checked conversion, multiplication, and addition, and return InvalidArgument instead of silently creating non-expiring sessions.
314cb46 to
1789522
Compare
|
/ok to test 1789522 Rebased onto the latest upstream |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid, concentrated numeric-safety work across existing CLI, TUI, server, Docker, and Podman paths.
Head SHA: 1789522df14313e5062aacd46c322590850dd088
Thanks @andrewwhitecdw. I checked the rebased head against the earlier boundary findings. The exact 2^63/2^64 floating-point guards, Docker millicore overflow, Podman signed health-check interval limit, negative and overlong CLI durations, and checked SSH TTL arithmetic are resolved. The independent review found no blocking correctness or security issue. Two non-blocking regression-test hardening notes are included inline.
Docs: No Fern update is needed; these are representation-bound safety fixes rather than new user-facing product constraints.
E2E: test:e2e is required because this head changes Docker/Podman driver configuration behavior and server sandbox-session behavior.
Next state: gator:watch-pipeline
| // Negative durations are rejected by parse_duration_to_ms. Overlong | ||
| // durations are clamped to zero (show all logs since the epoch) rather | ||
| // than silently producing a future timestamp. | ||
| now_ms.checked_sub(dur_ms).unwrap_or(0).max(0) |
There was a problem hiding this comment.
gator-agent
Non-blocking test hardening: The overlong-range fix has no direct regression test. Please consider testing dur_ms > now_ms and asserting the result is 0; extracting this arithmetic into a small pure helper would make the boundary easy to cover without exercising sandbox_logs.
| Arc::get_mut(&mut state) | ||
| .expect("fresh test state is uniquely held") | ||
| .config | ||
| .ssh_session_ttl_secs = u64::MAX; |
There was a problem hiding this comment.
gator-agent
Non-blocking test hardening: u64::MAX exercises the u64→i64 conversion failure, but not the new checked_mul or checked_add paths. Please consider cases around (i64::MAX as u64 / 1000) + 1 and i64::MAX as u64 / 1000, or a helper accepting now_ms, to protect the exact largest-accepted and first-rejected TTL boundaries.
johntmyers
added
gator:watch-pipeline
|
Label |
|
/ok to test 1789522 |
johntmyers
added
gator:in-review
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