Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion crates/openshell-cli/src/commands/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,11 @@ pub fn parse_duration_to_ms(s: &str) -> Result<i64> {
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,
Expand All @@ -736,7 +741,8 @@ pub fn parse_duration_to_ms(s: &str) -> Result<i64> {
));
}
};
Ok(num * multiplier)
num.checked_mul(multiplier)
.ok_or_else(|| miette::miette!("duration value is too large: {s}"))
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -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);
}
}
5 changes: 4 additions & 1 deletion crates/openshell-cli/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
0
};
Expand Down
29 changes: 26 additions & 3 deletions crates/openshell-driver-docker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2738,7 +2738,14 @@ fn parse_cpu_limit(value: &str) -> Result<Option<i64>, 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::<f64>().map_err(|_| {
Expand All @@ -2752,7 +2759,15 @@ fn parse_cpu_limit(value: &str) -> Result<Option<i64>, 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)]
Expand Down Expand Up @@ -2798,7 +2813,15 @@ fn parse_memory_limit(value: &str) -> Result<Option<i64>, 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<DriverSandbox> {
Expand Down
56 changes: 56 additions & 0 deletions crates/openshell-driver-docker/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
);
}
77 changes: 75 additions & 2 deletions crates/openshell-driver-podman/src/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1211,8 +1229,13 @@ fn parse_cpu_to_microseconds(quantity: &str) -> Option<u64> {
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.
Expand Down Expand Up @@ -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));
Expand Down
37 changes: 36 additions & 1 deletion crates/openshell-server/src/grpc/sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down Expand Up @@ -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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

Non-blocking test hardening: u64::MAX exercises the u64i64 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.


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"));
}
}
35 changes: 32 additions & 3 deletions crates/openshell-tui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -2649,3 +2648,33 @@ 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_past_epoch() {
// Epoch 0 and slightly negative ms are in the past, not the future.
// They should return a positive age, not "-".
assert_ne!(format_age(0), "-");
assert_ne!(format_age(-1), "-");
}
}
Loading