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 |
314cb46 to
1789522
Compare
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>
|
/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
- 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. Signed-off-by: Andrew White <andrewh@cdw.com>
637f212 to
37a23ea
Compare
|
Rebased onto latest Two process fixes:
|
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: 37a23ea5a541cf17f49190474a02301d4062502e
Base SHA: fde96f0488d09d34472521d0dd59bdf20c5bdf61
Merge base SHA: fde96f0488d09d34472521d0dd59bdf20c5bdf61
Patch ID: c1a014332bcd32dce1c321b56609b666a08870cc
Gator payload: 2
Review mode: follow_up
Previous reviewed SHA: 1789522df14313e5062aacd46c322590850dd088
Thanks @andrewwhitecdw. I checked your August 3 update about rebasing onto current main, dropping the invalid repair-bot commit, restoring the non-positive format_age assertions, and fixing commit identity/sign-offs. GitHub now reports the head as mergeable, DCO is green, every PR commit is attributed to Andrew White with a sign-off, and the author-only range-diff preserves the previously reviewed fixes.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None. The two prior regression-test hardening notes remain non-blocking suggestions and do not require another commit.
Docs: No Fern update is needed; these remain representation-bound safety fixes rather than a new documented UX or configuration contract.
E2E: test:e2e remains required because this head changes Docker/Podman driver configuration behavior and server sandbox-session behavior. Gator will reconcile the runner mirror for this exact head and monitor the required checks.
Next state: gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
|
/ok to test 37a23ea |
johntmyers
added
gator:in-review
Author Follow-Up NudgeThis PR has been in @andrewwhitecdw, please run |
Signed-off-by: Andrew White <andrewh@cdw.com>
|
/ok to test 9f9714c Ran |
Review Convergence CheckpointHead SHA: Three finding-bearing review rounds have completed. Thanks @andrewwhitecdw. I checked your August 5 update that ran Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: accept the current scope and allow Gator to resume required-check/E2E monitoring, split any desired test hardening into follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
johntmyers
added
gator:blocked
|
/ok to test 9f9714c |
johntmyers
added
gator:in-review
Blocker Follow-Up NudgeThis item is still blocked by the review convergence checkpoint after more than 48 business hours. Next action: @NVIDIA/openshell-maintainers, please accept the current scope, split any desired follow-up work, waive an obligation, or explicitly authorize another autonomous review round. |
Blocker Follow-Up NudgeThis item is still blocked after more than 48 business hours because Next action: @andrewwhitecdw, please remove the trailing comma inside the |
The format! invocation in the docker cpu_limit error path carried an unnecessary trailing comma, which clippy's unnecessary_trailing_comma lint rejects on both Linux Rust jobs in Branch Checks. Removing it keeps the lint clean; no behavior change. Signed-off-by: Andrew White <andrewh@cdw.com>
|
Removed the unnecessary trailing comma inside the New head: |
|
/ok to test 5c47ec3 |
Review Convergence CheckpointHead SHA: Three finding-bearing review rounds have completed. Thanks @andrewwhitecdw. I checked your August 8 update that removes the Clippy-rejected trailing comma in Root-cause findings:
Scope growth:
Reviewer-quality signals:
The existing Maintainer action: accept the current scope and allow Gator to resume required-check/E2E monitoring, split any desired test hardening into follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
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