Skip to content

Commit c973134

Browse files
author
Roy Lin
committed
fix(cri): StopPodSandbox state invariant + non-running container stats
Two confirmed bugs from the adversarial CRI audit, hand-verified: - StopPodSandbox left the sandbox Ready if VM teardown failed. It marks all containers Exited + removes their stream handles, then destroy_sandbox_vm(...)? propagated an error BEFORE update_sandbox_state(NotReady) ran — so the sandbox stayed Ready with Exited containers and a gone VM. A later CreateContainer (which only checks the sandbox is Ready) would then attach to a sandbox whose VM no longer exists. Mark the sandbox NotReady regardless of the destroy result (state made consistent first), then surface the destroy error. - container_stats / list_container_stats reported VM usage for non-running containers. They split the pod VM's usage across containers without checking the container is Running, so a Created/Exited container reported a full share of CPU/memory. list_container_stats was worse: it counted ALL containers (any state) as the divisor. Now only running containers get a usage share; others report zero, and the divisor counts running containers only. Also REJECTED from the same audit (hand-verified false positives): the 5 'CPU cpu_period/cpu_quota nanosecond' findings — CRI defines these as CFS period/quota in MICROSECONDS (matching ResourceLimits + the guest cpu.max), and critest already measures a 0.5-core limit at ~0.53; adding /1000 would have broken working CPU limits. clippy -D warnings clean; 237 cri unit tests pass.
1 parent 6b706ca commit c973134

1 file changed

Lines changed: 39 additions & 19 deletions

File tree

  • src/cri/src/runtime_service

src/cri/src/runtime_service/mod.rs

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -697,12 +697,17 @@ impl RuntimeService for BoxRuntimeService {
697697
}
698698
}
699699

700-
self.destroy_sandbox_vm(sandbox_id, None).await?;
700+
// Mark the sandbox NotReady regardless of whether VM teardown succeeds:
701+
// its containers are already Exited and their streams removed, so it must
702+
// not remain Ready — otherwise a later CreateContainer (which only checks
703+
// the sandbox is Ready) would attach to a sandbox whose VM is gone. A
704+
// VM-destroy error is surfaced after the state is made consistent.
705+
let destroy_result = self.destroy_sandbox_vm(sandbox_id, None).await;
701706
self.disconnect_sandbox_network(&sandbox).await;
702-
703707
self.store
704708
.update_sandbox_state(sandbox_id, SandboxState::NotReady)
705709
.await;
710+
destroy_result?;
706711

707712
Ok(Response::new(StopPodSandboxResponse {}))
708713
}
@@ -2383,18 +2388,23 @@ impl RuntimeService for BoxRuntimeService {
23832388
.await
23842389
.ok_or_else(|| Status::not_found(format!("Container not found: {}", container_id)))?;
23852390

2386-
let running = self
2387-
.store
2388-
.containers
2389-
.list(Some(&container.sandbox_id), None)
2390-
.await
2391-
.into_iter()
2392-
.filter(|c| c.state == ContainerState::Running)
2393-
.count();
2394-
let usage = self
2395-
.sandbox_vm_usage(&container.sandbox_id)
2396-
.await
2397-
.per_container(running);
2391+
// Only a running container consumes VM resources; a Created/Exited one
2392+
// reports zero rather than a share of the pod VM's usage.
2393+
let usage = if container.state == ContainerState::Running {
2394+
let running = self
2395+
.store
2396+
.containers
2397+
.list(Some(&container.sandbox_id), None)
2398+
.await
2399+
.into_iter()
2400+
.filter(|c| c.state == ContainerState::Running)
2401+
.count();
2402+
self.sandbox_vm_usage(&container.sandbox_id)
2403+
.await
2404+
.per_container(running)
2405+
} else {
2406+
VmUsage::default()
2407+
};
23982408
Ok(Response::new(ContainerStatsResponse {
23992409
stats: Some(container_stats(&container, usage).await),
24002410
}))
@@ -2440,9 +2450,14 @@ impl RuntimeService for BoxRuntimeService {
24402450
std::collections::HashMap::new();
24412451
for container in &containers {
24422452
if !usage_by_sandbox.contains_key(&container.sandbox_id) {
2453+
// Split across RUNNING containers only (a non-running one consumes
2454+
// nothing) so the running ones get an accurate share.
24432455
let running = containers
24442456
.iter()
2445-
.filter(|c| c.sandbox_id == container.sandbox_id)
2457+
.filter(|c| {
2458+
c.sandbox_id == container.sandbox_id
2459+
&& c.state == ContainerState::Running
2460+
})
24462461
.count();
24472462
let usage = self
24482463
.sandbox_vm_usage(&container.sandbox_id)
@@ -2452,10 +2467,15 @@ impl RuntimeService for BoxRuntimeService {
24522467
}
24532468
}
24542469
let stats = join_all(containers.iter().map(|c| {
2455-
let usage = usage_by_sandbox
2456-
.get(&c.sandbox_id)
2457-
.copied()
2458-
.unwrap_or_default();
2470+
// Only running containers report a share of the VM usage; others zero.
2471+
let usage = if c.state == ContainerState::Running {
2472+
usage_by_sandbox
2473+
.get(&c.sandbox_id)
2474+
.copied()
2475+
.unwrap_or_default()
2476+
} else {
2477+
VmUsage::default()
2478+
};
24592479
container_stats(c, usage)
24602480
}))
24612481
.await;

0 commit comments

Comments
 (0)