Conversation
|
Review summary: I left 2 inline findings on Firecracker TAP cleanup: a failed TAP deletion is reported as a successful stop, and an already-exited Firecracker instance bypasses the new cleanup path. I reviewed correctness, security, design, conventions, tests, documentation, comments, and API usage. |
hbrodin
left a comment
hbrodin
left a comment
There was a problem hiding this comment.
Follow-up review using the updated review, lifecycle, and path/trust guidance on current main. That guidance prompted a fresh check of failure paths and claims after the recent PR fixes, which surfaced the three inline issues below. I checked the new PID/socket paths against the filesystem trust guidance and found no supported guest-derived path escape. git diff --check and bash -n passed; I did not run the Firecracker or Lima VM integration suites.
| // curl exit 7 means it could not connect. With the probe running as | ||
| // root, this covers a missing listener without conflating it with the | ||
| // ordinary user's lack of write permission on the socket. | ||
| Some(7) => Ok(()), |
There was a problem hiding this comment.
Could we avoid treating curl exit 7 as proof that Firecracker has exited? I reproduced exit 7 with these curl flags against a live Unix listener whose accept queue was full. If this instance's PID file is missing or stale, both liveness probes can therefore return stopped, and coop stop removes its proxy credentials and TAP while the VM is still running.
A privileged probe that preserves the connect(2) error would distinguish a refused or missing socket from a busy listener: accept only ECONNREFUSED/ENOENT as confirmed exit, and leave EAGAIN, timeouts, and other failures uncertain. Please add a regression with a live listener and full accept queue that verifies stop retains the proxy and TAP; the current live-socket test has an empty queue, while the integration case kills Firecracker first.
|
I left one inline finding on the Firecracker socket probe: curl exit 7 can also come from a live Unix listener with a full accept queue. I reproduced that result with the probe's curl flags. The proposed fix is to preserve the underlying connection error and keep uncertain states from authorizing TAP and proxy teardown. I reviewed correctness, security, API usage, tests, design, conventions, docs, and comments on head |
2 participants
Summary
coop stopnow removes the instance’s TAP device after Firecracker exits. If Firecracker survives SIGKILL, stop returns an error and leaves the network in place rather than reporting a successful shutdown.Testing