[LXC] (3/5) Harden concurrency and cleanup - #1329
Soham Das (SohamDas2021) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
e3cadab to
54b54b0
Compare
Soham Das (SohamDas2021)
changed the title
54b54b0 to
96b0a80
Compare
Soham Das (SohamDas2021)
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
96b0a80 to
6e320e5
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A successful non-consuming wait() retains the container-name claim and incorrectly blocks subsequent sandboxes.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Hardens LXC concurrency by isolating executor-only signal cleanup and preventing concurrent reuse of container names.
Changes:
- Gates watchdog registration to
lxc-exec. - Adds process-local container-name claims with collision retries.
- Adds regression tests for cleanup and collision behavior.
| File | Description |
|---|---|
signal_cleanup.rs |
Restricts watchdog state publication to installed executors. |
lxc_runner.rs |
Adds container-name ownership and concurrency tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Keeps this sandbox's container out of reach of a second one in this | ||
| /// process. | ||
| _name_claim: ContainerNameClaim, |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 959d4b60-0a77-4533-b88a-20a5be80703f
6e320e5 to
9893b85
Compare
| start_directory: start_directory(request).unwrap_or_default(), | ||
| exec_env, | ||
| timeout, | ||
| _name_claim: name_claim, |
There was a problem hiding this comment.
Confirmed, and pre-existing on the shared tear_down that lxc-exec also uses, the fix lands in (4/5) with the engine arm that makes the path reachable.
| struct ContainerNameClaim { | ||
| name: String, | ||
| } | ||
|
|
||
| impl ContainerNameClaim { |
There was a problem hiding this comment.
question: I'm slightly confused on what this is for/ it get us tbh. Is this something to do with already running containers lxc created containers and their names?
| }) | ||
| } | ||
|
|
||
| fn claim_generated_name(&self) -> Result<ContainerNameClaim, ScriptResponse> { |
There was a problem hiding this comment.
note: same about this function, if lxc is creating containers and we're just a wrapper for it, what's the generated name stuff for? I assumed it was simple and we're just calling the lxc executables, but seems like its more complex than I thought?
is this about two containers not being allowed the same name?
| if let Some(overridden) = WATCHDOG_OVERRIDE.with(std::cell::Cell::get) { | ||
| return overridden; | ||
| } | ||
| WATCHDOG_INSTALLED.load(Ordering::Acquire) |
There was a problem hiding this comment.
note: slightly confused on the watchdog usecase tbh.
Stacked on #1326 (2/5). Review only the top commit; the rest is that PR.
PR 2 made two in-process LXC sandboxes able to live at once in one host process. Nothing in the backend was written for that, so this PR closes the two holes before the engine arm makes the path reachable.
The signal watchdog is the executor binary's alone
On a fatal signal the executor destroys the container it has on record. There is one record, so a second concurrent sandbox overwrote the first's — leaving that container and its firewall chains to leak. The watchdog also ends the process, which a library must not do to its caller.
Nothing registers now unless the executor armed the watchdog.
lxc-execarms it before it does anything else, so the binary behaves exactly as before. A library host arms nothing, and there each sandbox handle already tears itself down.Bubblewrap runs through this same code and never registers a container, so it is unaffected. A test pins that.
A container a live sandbox holds is refused, not taken
A container applies its network policy only when it starts. So a second sandbox naming one that is already running would restart it to apply its own policy — killing the first sandbox's workload. Handles are bound to a container name rather than to an instance, so dropping one handle could stop another's container.
The second sandbox is now refused, and told which container it collided with and which field to change. The name frees as soon as the first sandbox finishes or its handle drops. Generated names retry on collision. Excluding sandboxes in other processes is out of scope and documented as a limitation.
Validation
fmt, clippy on the host,x86_64-unknown-linux-gnu, andaarch64-apple-darwin, plusbwrap_commonand thelxcbinaries. 377 tests on Windows and 398 on Linux, the latter through the exact release command #1326 added to CI. The three behaviors this PR adds were each confirmed by mutation — reverting a guard fails its own test and no other.Still unreachable: the engine arm lands in 4/5.