diff --git a/crates/openshell-cli/src/commands/common.rs b/crates/openshell-cli/src/commands/common.rs index e6edb4d33..a134ff6f0 100644 --- a/crates/openshell-cli/src/commands/common.rs +++ b/crates/openshell-cli/src/commands/common.rs @@ -726,6 +726,11 @@ pub fn parse_duration_to_ms(s: &str) -> Result { let num: i64 = num_str .parse() .map_err(|_| miette::miette!("invalid duration: {s} (expected e.g. 5m, 1h, 30s)"))?; + if num < 0 { + return Err(miette::miette!( + "duration must not be negative: {s}" + )); + } let multiplier = match unit { "s" => 1_000, "m" => 60_000, @@ -736,7 +741,8 @@ pub fn parse_duration_to_ms(s: &str) -> Result { )); } }; - Ok(num * multiplier) + num.checked_mul(multiplier) + .ok_or_else(|| miette::miette!("duration value is too large: {s}")) } // --------------------------------------------------------------------------- @@ -975,4 +981,24 @@ mod tests { let err = parse_duration_to_ms("\u{20ac}").expect_err("missing number should error"); assert!(err.to_string().contains("invalid duration")); } + + #[test] + fn parse_duration_to_ms_rejects_overflow() { + let err = parse_duration_to_ms("100000000000000h").expect_err("overflow should error"); + assert!(err.to_string().contains("too large")); + } + + #[test] + fn parse_duration_to_ms_rejects_negative() { + let err = parse_duration_to_ms("-5m").expect_err("negative duration should error"); + assert!(err.to_string().contains("negative")); + } + + #[test] + fn parse_duration_to_ms_accepts_zero_and_valid() { + assert_eq!(parse_duration_to_ms("0s").unwrap(), 0); + assert_eq!(parse_duration_to_ms("30s").unwrap(), 30_000); + assert_eq!(parse_duration_to_ms("5m").unwrap(), 300_000); + assert_eq!(parse_duration_to_ms("2h").unwrap(), 7_200_000); + } } diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 4843475e5..8622735bd 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -6508,7 +6508,10 @@ pub async fn sandbox_logs( .as_millis(), ) .into_diagnostic()?; - now_ms - dur_ms + // 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) } else { 0 }; diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index a196ba6ad..60b04ea2c 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -2738,7 +2738,14 @@ fn parse_cpu_limit(value: &str) -> Result, Status> { "docker cpu_limit must be greater than zero", )); } - return Ok(Some(millicores.saturating_mul(1_000_000))); + return millicores + .checked_mul(1_000_000) + .map(Some) + .ok_or_else(|| { + Status::failed_precondition(format!( + "docker cpu_limit '{value}' is too large", + )) + }); } let cores = value.parse::().map_err(|_| { @@ -2752,7 +2759,15 @@ fn parse_cpu_limit(value: &str) -> Result, Status> { )); } - 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 { + return Err(Status::failed_precondition(format!( + "docker cpu_limit '{value}' is too large", + ))); + } + + Ok(Some(nano_cpus as i64)) } #[allow(clippy::cast_possible_truncation)] @@ -2798,7 +2813,15 @@ fn parse_memory_limit(value: &str) -> Result, Status> { } }; - 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 { + return Err(Status::failed_precondition(format!( + "docker memory_limit '{value}' is too large", + ))); + } + + Ok(Some(bytes as i64)) } fn sandbox_from_container_summary(summary: &ContainerSummary) -> Option { diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index a86a9936f..0e752d92d 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -2250,3 +2250,59 @@ fn container_state_needs_resume_matches_startable_states() { ); } } + +#[test] +fn parse_cpu_limit_rejects_overflow() { + let err = parse_cpu_limit("1e300").unwrap_err(); + assert!(err.message().contains("too large")); +} + +#[test] +fn parse_memory_limit_rejects_overflow() { + // 308 nines is finite as f64 (~1e308), but multiplying by Gi overflows to inf. + let huge = "9".repeat(308) + "Gi"; + let err = parse_memory_limit(&huge).unwrap_err(); + assert!(err.message().contains("too large")); +} + +#[test] +fn parse_cpu_limit_rejects_i64_max_boundary() { + // 9_223_372_036.854776 cores * 1e9 rounds exactly to 2^63, which used to + // pass the > check and silently saturate to i64::MAX. It must now be + // rejected. (9_223_372_037 is already above the boundary and would also + // pass a > guard, so it does not test the equality case.) + let err = parse_cpu_limit("9223372036.854776").unwrap_err(); + assert!(err.message().contains("too large")); + + // One core below the boundary is still valid. + assert_eq!( + parse_cpu_limit("9223372036").unwrap(), + Some(9_223_372_036_000_000_000) + ); +} + +#[test] +fn parse_cpu_limit_rejects_millicore_overflow() { + // 9_223_372_036_854 millicores * 1_000_000 == 9_223_372_036_854_000_000, + // which fits in i64. One millicore more overflows and must be rejected. + assert_eq!( + parse_cpu_limit("9223372036854m").unwrap(), + Some(9_223_372_036_854_000_000) + ); + let err = parse_cpu_limit("9223372036855m").unwrap_err(); + assert!(err.message().contains("too large")); +} + +#[test] +fn parse_memory_limit_rejects_i64_max_boundary() { + // 8192 PiB = 2^63 bytes, which used to pass the > check and silently + // saturate to i64::MAX. It must now be rejected. + let err = parse_memory_limit("8192Pi").unwrap_err(); + assert!(err.message().contains("too large")); + + // One PiB below the boundary is still valid. + assert_eq!( + parse_memory_limit("8191Pi").unwrap(), + Some(8191 * 1024_i64.pow(5)) + ); +} diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 90ef0fec2..eceaa3177 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -1074,7 +1074,25 @@ pub fn build_container_spec_for_image( openshell_core::config::DEFAULT_SSH_PORT ), ], - interval: config.health_check_interval_secs * 1_000_000_000, + interval: { + const NS_PER_S: u64 = 1_000_000_000; + let max_secs = (i64::MAX as u64) / NS_PER_S; + if config.health_check_interval_secs > max_secs { + return Err(ComputeDriverError::InvalidArgument(format!( + "health_check_interval_secs {} exceeds maximum allowed nanoseconds", + config.health_check_interval_secs + ))); + } + config + .health_check_interval_secs + .checked_mul(NS_PER_S) + .ok_or_else(|| { + ComputeDriverError::InvalidArgument(format!( + "health_check_interval_secs {} exceeds maximum allowed nanoseconds", + config.health_check_interval_secs + )) + })? + }, timeout: 2_000_000_000, retries: 10, start_period: 5_000_000_000, @@ -1211,8 +1229,13 @@ fn parse_cpu_to_microseconds(quantity: &str) -> Option { if cores <= 0.0 || cores.is_nan() || cores.is_infinite() { return None; } + let micros_f = cores * 100_000.0; + #[allow(clippy::cast_precision_loss)] + if !micros_f.is_finite() || micros_f >= u64::MAX as f64 { + return None; + } #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] - let val = (cores * 100_000.0) as u64; + let val = micros_f as u64; val }; // A quota of 0 microseconds is invalid — treat as no limit. @@ -1287,6 +1310,56 @@ mod tests { assert_eq!(parse_cpu_to_microseconds("0.5"), Some(50_000)); } + #[test] + fn parse_cpu_huge_value_returns_none_instead_of_overflow() { + // A finite f64 whose product with 100_000 overflows to infinity. + assert_eq!(parse_cpu_to_microseconds("1e300"), None); + } + + #[test] + fn parse_cpu_rejects_u64_max_boundary() { + // 184_467_440_737_095.51616 cores * 100_000 rounds exactly to 2^64, + // which used to pass the > check and silently saturate to u64::MAX. + // It must now be rejected. The previous value (184467440737095520) + // was ~1000x larger and did not exercise the equality boundary. + assert_eq!(parse_cpu_to_microseconds("184467440737095.51616"), None); + + // Just below the boundary is still valid. + assert_eq!( + parse_cpu_to_microseconds("184467440737095"), + Some(18_446_744_073_709_500_416) + ); + } + + #[test] + fn container_spec_rejects_health_check_interval_overflow() { + let sandbox = test_sandbox("test-id", "test-name"); + let mut config = test_config(); + // i64::MAX nanoseconds / 1_000_000_000 ns/s = 9_223_372_036 seconds. + // One second over must be rejected before it can saturate. + config.health_check_interval_secs = 9_223_372_037; + let err = try_build_container_spec_with_token(&sandbox, &config, None).unwrap_err(); + assert!( + matches!(err, ComputeDriverError::InvalidArgument(_)), + "expected InvalidArgument, got {err:?}" + ); + assert!(format!("{err}").contains("health_check_interval_secs")); + } + + #[test] + fn container_spec_accepts_health_check_interval_at_boundary() { + let sandbox = test_sandbox("test-id", "test-name"); + let mut config = test_config(); + // i64::MAX nanoseconds / 1_000_000_000 ns/s = 9_223_372_036 seconds. + const NS_PER_S: u64 = 1_000_000_000; + config.health_check_interval_secs = (i64::MAX as u64) / NS_PER_S; + let spec = try_build_container_spec_with_token(&sandbox, &config, None).unwrap(); + let interval = spec["healthconfig"]["Interval"] + .as_u64() + .expect("healthcheck interval should be a u64"); + assert_eq!(interval, config.health_check_interval_secs * NS_PER_S); + } + #[test] fn parse_memory_binary_suffixes() { assert_eq!(parse_memory_to_bytes("256Mi"), Some(256 * 1024 * 1024)); diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 0d6a4e277..06a0362d6 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -1422,7 +1422,14 @@ pub(super) async fn handle_create_ssh_session( let token = uuid::Uuid::new_v4().to_string(); 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_secs = state.config.ssh_session_ttl_secs; + let ttl_ms = i64::try_from(ttl_secs) + .map_err(|_| Status::invalid_argument("ssh_session_ttl_secs is too large"))? + .checked_mul(1000) + .ok_or_else(|| Status::invalid_argument("ssh_session_ttl_secs is too large"))?; + now_ms + .checked_add(ttl_ms) + .ok_or_else(|| Status::invalid_argument("ssh_session_ttl_secs is too large"))? } else { 0 }; @@ -4016,4 +4023,32 @@ mod tests { assert!(session.revoked); assert_eq!(session.object_workspace(), "default"); } + + #[tokio::test] + async fn create_ssh_session_rejects_too_large_ttl() { + let mut state = test_server_state().await; + state + .store + .put_message(&test_sandbox("ttl-test", Vec::new())) + .await + .unwrap(); + + // Any value larger than i64::MAX seconds cannot be converted to + // milliseconds without overflowing. + Arc::get_mut(&mut state) + .expect("fresh test state is uniquely held") + .config + .ssh_session_ttl_secs = u64::MAX; + + let err = handle_create_ssh_session( + &state, + Request::new(CreateSshSessionRequest { + sandbox_id: "sandbox-ttl-test".to_string(), + }), + ) + .await + .unwrap_err(); + assert_eq!(err.code(), tonic::Code::InvalidArgument); + assert!(err.message().contains("ssh_session_ttl_secs is too large")); + } } diff --git a/crates/openshell-tui/src/lib.rs b/crates/openshell-tui/src/lib.rs index b3937e2b9..cd8693178 100644 --- a/crates/openshell-tui/src/lib.rs +++ b/crates/openshell-tui/src/lib.rs @@ -2603,11 +2603,10 @@ fn format_age(epoch_ms: i64) -> String { let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map_or(0, |d| d.as_secs().cast_signed()); - let diff = now - created_secs; - if diff < 0 { + if created_secs > now { return String::from("-"); } - let diff = diff.cast_unsigned(); + let diff = (now - created_secs).cast_unsigned(); if diff < 60 { format!("{diff}s") } else if diff < 3600 { @@ -2649,3 +2648,31 @@ fn days_to_ymd(days: i64) -> (i64, i64, i64) { let y = if m <= 2 { y + 1 } else { y }; (y, m, d) } + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn format_age_handles_future_timestamp() { + let future_ms = i64::try_from( + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis(), + ) + .unwrap() + + 10_000; + assert_eq!(format_age(future_ms), "-"); + } + + #[test] + fn format_age_handles_zero_and_negative() { + assert_eq!(format_age(0), "-"); + assert_eq!(format_age(-1), "-"); + } +}