release: check generated vm ids before launch

Author: moradology

.beads/issues.jsonl

@@ -4,7 +4,7 @@ members = ["crates/*"]
 exclude = ["support/m80-close-range"]
 
 [workspace.package]
-version = "0.2.10"
+version = "0.2.11"
 edition = "2021"
 license = "Apache-2.0 OR MIT"
 repository = "https://github.com/nathanp/m80"

Cargo.lock

@@ -28,16 +28,16 @@ fn current_latest_version_cutover_names_real_repair_tag() {
             .expect("read installer layout fixture");
 
     assert!(
-        cargo_toml.contains("version = \"0.2.10\""),
+        cargo_toml.contains("version = \"0.2.11\""),
         "workspace package version must match the current repair tag"
     );
     assert!(
-        runbook.contains("workspace package version `0.2.10`")
-            && runbook.contains("expected release tag is `v0.2.10`"),
+        runbook.contains("workspace package version `0.2.11`")
+            && runbook.contains("expected release tag is `v0.2.11`"),
         "release runbook must document the real current repair version"
     );
     assert!(
-        behavior.contains("matching stable tag is `v0.2.10`")
+        behavior.contains("matching stable tag is `v0.2.11`")
             && behavior.contains("`v0.2.7` installer handoff"),
         "behavior doc must name the repaired tag and the superseded latest state"
     );

Cargo.toml

@@ -669,9 +669,10 @@ an invariant fails closed.
   `SharedPmemErofsLayoutProbeInvalid`, `VmIdPathBudgetExceeded`). It is not a
   fallback bucket: it is reserved for caller configuration, CLI flag, and config
   merge failures where no lower crate owns a more specific typed cause.
-- `ConfigError::VmIdPathBudgetExceeded` is raised at `Backend::admit()` when a
-  caller-supplied `vm_id` would produce an AF_UNIX socket path longer than the
-  kernel `sun_path` cap (107 usable bytes). The path layout is
+- `ConfigError::VmIdPathBudgetExceeded` is raised at `Backend::admit()` when
+  the selected `vm_id` would produce an AF_UNIX socket path longer than the
+  kernel `sun_path` cap (107 usable bytes). The selected id is either the
+  caller-supplied id or the generated `vm-{pid}-{unix_ms}` id. The path layout is
   `<run_root>/<vm_id>/<fc_basename>/<vm_id>/root/firecracker.sock`; vm_id
   appears twice because the jail layout inherits Firecracker's jailer
   convention. The check is pure arithmetic and runs before the admission

crates/m80-cli/tests/release/current_latest_version_cutover.rs

@@ -92,8 +92,8 @@ impl Backend {
     /// Acquire one admission permit and return a [`Sandbox`] in Created state.
     ///
     /// Fails fast with [`FcError::AdmissionRefused`] when no permits are
-    /// available, or with [`ConfigError::VmIdPathBudgetExceeded`] when a
-    /// caller-supplied `vm_id` would overflow the AF_UNIX `sun_path` cap.
+    /// available, or with [`ConfigError::VmIdPathBudgetExceeded`] when the
+    /// selected `vm_id` would overflow the AF_UNIX `sun_path` cap.
     /// Does not block.
     pub fn admit(self: &Arc<Self>, mut config: SandboxConfig) -> Result<Sandbox, FcError> {
         validate_caller_boot_args_if_present(config.boot_args.as_deref())?;
@@ -102,16 +102,13 @@ impl Backend {
         }
         validate_network_policy(&config.network)?;
 
-        // Caller-supplied vm_ids are validated up front so the failure
-        // surfaces as a typed config error rather than as an opaque
-        // `bind() AF_UNIX path too long` deep inside launch. The
-        // auto-generated `vm-{pid}-{ts}` form (resolve_vm_id) is bounded by
-        // construction and does not need a check here.
-        if let Some(vm_id) = config.vm_id.as_deref() {
-            check_vm_id_name(vm_id)?;
-            check_vm_id_reserved_name(vm_id)?;
-            check_vm_id_path_budget(&self.config, vm_id)?;
-        }
+        let vm_id = config.vm_id.get_or_insert_with(auto_vm_id);
+        // Validate the selected vm_id up front so the failure surfaces as a
+        // typed config error rather than as an opaque AF_UNIX failure after
+        // launch has partially materialized a jail.
+        check_vm_id_name(vm_id)?;
+        check_vm_id_reserved_name(vm_id)?;
+        check_vm_id_path_budget(&self.config, vm_id)?;
 
         let mut available = self.semaphore.lock().unwrap_or_else(|p| p.into_inner());
 
@@ -312,10 +309,15 @@ fn live_run_dir_names(run_root: &Path) -> Result<Vec<String>, FcError> {
     Ok(names)
 }
 
-/// Reject a caller-supplied `vm_id` whose constructed AF_UNIX socket path
-/// would exceed the kernel's `sun_path` cap. The check is purely arithmetic
-/// (no filesystem access) so it can run before the admission permit is
-/// acquired.
+fn auto_vm_id() -> String {
+    let pid = std::process::id();
+    let ts = crate::runroot::unix_ms_now();
+    format!("vm-{pid}-{ts}")
+}
+
+/// Reject a selected `vm_id` whose constructed AF_UNIX socket path would
+/// exceed the kernel's `sun_path` cap. The check is purely arithmetic (no
+/// filesystem access) so it can run before the admission permit is acquired.
 fn check_vm_id_path_budget(config: &BackendConfig, vm_id: &str) -> Result<(), FcError> {
     let fc_basename = config
         .discovery

crates/m80-firecracker/README.md

@@ -453,15 +453,15 @@ pub enum ConfigError {
         /// Maximum accepted artifact byte length.
         max: u64,
     },
-    /// The caller-supplied `vm_id` would produce an AF_UNIX socket path that
-    /// exceeds the kernel's `sun_path` cap. Surfaces at admission time so the
-    /// failure cannot reach `bind()` / `connect()` as an opaque IO error.
+    /// The selected `vm_id` would produce an AF_UNIX socket path that exceeds
+    /// the kernel's `sun_path` cap. Surfaces at admission time so the failure
+    /// cannot reach `bind()` / `connect()` as an opaque IO error.
     #[error(
         "vm_id {vm_id:?} would produce a {path_len}-byte AF_UNIX socket path under run_root {} (cap {budget})",
         run_root.display()
     )]
     VmIdPathBudgetExceeded {
-        /// Caller-supplied vm_id whose path would overflow.
+        /// Selected vm_id whose path would overflow.
         vm_id: String,
         /// Configured run-root.
         run_root: PathBuf,

crates/m80-firecracker/src/backend.rs

@@ -109,11 +109,10 @@ impl Sandbox {
 
     /// Resolve or auto-generate the VM identifier.
     fn resolve_vm_id(&self) -> String {
-        self.config.vm_id.clone().unwrap_or_else(|| {
-            let pid = std::process::id();
-            let ts = crate::runroot::unix_ms_now();
-            format!("vm-{pid}-{ts}")
-        })
+        self.config
+            .vm_id
+            .clone()
+            .expect("Backend::admit stores the selected vm_id")
     }
 
     /// Delete the partial run directory if launch fails.

crates/m80-firecracker/src/error.rs

@@ -1,5 +1,5 @@
-//! Admission-time validation that a caller-supplied `vm_id` would not
-//! produce an AF_UNIX socket path longer than the kernel's `sun_path` cap.
+//! Admission-time validation that the selected `vm_id` would not produce an
+//! AF_UNIX socket path longer than the kernel's `sun_path` cap.
 //!
 //! These tests run without KVM or root: they only exercise the arithmetic
 //! check inside `Backend::admit()`. The structural fix complements the
@@ -64,13 +64,34 @@ fn admit_just_under_budget_succeeds() {
 }
 
 #[test]
-fn admit_with_no_vm_id_skips_check() {
-    // Auto-generated `vm-{pid}-{ts}` is bounded by construction; admit must
-    // succeed without checking the budget.
-    let backend = common::make_fake_backend(4, Path::new("/var/lib/m80-run"));
+fn admit_with_no_vm_id_uses_checked_generated_id() {
+    let backend = common::make_fake_backend(4, Path::new("/tmp/m80-test"));
     backend
         .admit(config_with_vm_id(None))
-        .expect("None vm_id must admit (auto-generated form is bounded)");
+        .expect("None vm_id must admit when generated id fits");
+}
+
+#[test]
+fn admit_with_no_vm_id_rejects_generated_id_over_budget() {
+    let backend = common::make_fake_backend(
+        4,
+        Path::new("/tmp/m80-run-root-that-is-intentionally-too-long-for-auto-vm-ids"),
+    );
+    let err = backend
+        .admit(config_with_vm_id(None))
+        .expect_err("generated vm_id must be checked against the path budget");
+
+    let FcError::Config(ConfigError::VmIdPathBudgetExceeded {
+        vm_id,
+        path_len,
+        budget,
+        ..
+    }) = err
+    else {
+        panic!("expected ConfigError::VmIdPathBudgetExceeded, got {err:?}");
+    };
+    assert!(vm_id.starts_with("vm-"), "unexpected generated id: {vm_id}");
+    assert!(path_len > budget);
 }
 
 #[test]