virtualbox: test VBoxVMI disconnect safety

doc/src/developer/tests.md

@@ -75,13 +75,15 @@ Run the generic tests the method supports, or the VirtualBox ones:
 MICROVMI_TEST_FILTER=virtualbox_ cargo test --features virtualbox-api --test integration
 MICROVMI_TEST_FILTER=virtualbox_ VBOX_TEST_MEMORY_MB=5120 cargo test --features virtualbox-api --test integration
 MICROVMI_TEST_FILTER=vcpu VBOX_TEST_CPUS=2 cargo test --features virtualbox-api --test integration
-for f in init vcpu_count pause double_resume virtualbox_read_ virtualbox_write_physical_ registers_match write_registers; do MICROVMI_TEST_FILTER=$f VBOX_TEST_METHOD=vboxvmi cargo test --features virtualbox-vmi --test integration; done
+for f in init vcpu_count pause double_resume virtualbox_read_ virtualbox_write_physical_ registers_match write_registers vboxvmi_disconnect_; do MICROVMI_TEST_FILTER=$f MICROVMI_TEST_TIMEOUT=120 VBOX_TEST_METHOD=vboxvmi cargo test --features virtualbox-vmi --test integration; done
 for f in vcpu pause registers_match write_registers; do MICROVMI_TEST_FILTER=$f VBOX_TEST_CPUS=2 VBOX_TEST_METHOD=vboxvmi cargo test --features virtualbox-vmi --test integration; done
 ~~~
 
 The VBoxVMI filters above run every test, each once; `virtualbox_write_physical_` checks random
 patterns against a fresh core dump, after resume/re-pause and after restoring the original pages.
-The other filters cover `pause`,
+`vboxvmi_disconnect_` covers normal close, a killed client process, both external-pause ownership
+cases, a repeated external resume/pause versus disconnect race, and immediate `Busy` refusal of a
+second client. The other filters cover `pause`,
 `double_pause`, `pause_resume`, `multiple_pause_resume`, `virtualbox_pause_*` and
 `virtualbox_registers_unchanged_while_paused`; `vcpu_count` covers `get_vcpu_count` and
 `virtualbox_vcpu_count_matches_vmm`. With `VBOX_TEST_CPUS=2`, the register tests read every vCPU

doc/src/reference/drivers/virtualbox.md

@@ -24,7 +24,7 @@ A physical write to a non-RAM page fails; other pages in the same batch may alre
 Pause is VirtualBox's own pause (the VM shows as Paused in the GUI and `VBoxManage`). It is synchronous, all vCPUs are stopped when it returns, and idempotent: pausing a paused VM succeeds, and one resume resumes it.
 Pauses are not counted, also across clients: resume resumes the VM even if the GUI, `VBoxManage` or another tool paused it too.
 Pause and resume fail while VirtualBox controls the VM's run state: powering on or off, saving or restoring its state, live snapshot, teleport, or a configuration change (medium or network attachment). Resume also fails while the host's power management has the VM paused, and after a failed teleport of a paused VM until it is resumed through VirtualBox.
-A VM paused through VBoxVMI stays paused if the tool exits or disconnects without resuming it ([#287](https://github.com/Wenzel/libmicrovmi/issues/287)).
+A client disconnect leaves no session state behind: the server removes its intercepts and synchronously resumes a pause the client created. A pause that already existed, or an external pause after the client's pause was externally resumed, remains paused.
 A register read or write is one request, served by the vCPU itself between two of its instructions: a read is a consistent snapshot of that vCPU and a write changes all its registers at once, also while the VM runs (pause first to read or write several vCPUs at the same point).
 The MSRs of `X86Registers` (EFER, STAR, LSTAR, SYSENTER_CS/ESP/EIP, APIC base) are included; an MSR the guest CPU doesn't have reads as 0.
 A write is all or nothing: a value the CPU itself would not load (RFLAGS with bit 1 clear or a reserved bit set, reserved bits in CR0, CR3, CR4 or EFER, an inconsistent long mode state, an MSR value `WRMSR` refuses) fails the whole write, and nothing is written. Registers keeping their value are not touched, so writing back registers read earlier always succeeds and has no side effects.
@@ -38,7 +38,7 @@ Segment selectors, bases and limits are written as raw state: no descriptor tabl
   The socket is `<VirtualBox user home>/vmi/<VM UUID>.sock` (e.g. `~/.config/VirtualBox/vmi/`), or the path set in `VBoxInternal/VMI/SocketPath`.
 - libmicrovmi runs as the user running the VM (the socket is mode 0600 and the server checks the peer's uid; root is refused too).
 - A custom `SocketPath` must be absolute, in a directory owned by that user and not writable by group or others (unless sticky); otherwise the VM refuses to start.
-- One client per VM at a time: a second client gets a "another VBoxVMI client is already connected" error.
+- One client per VM at a time: a second client immediately gets the error "another VBoxVMI client is already connected".
 
 ### Initialization parameters
 

docs/adr/0002-vboxvmi-unix-socket.md

@@ -16,11 +16,15 @@ matched by id. Bulk memory reads are batched to amortise the per-message cost.
 
 ## Consequences
 
-- The kernel tells the server when the client goes away (EOF), so it can resume a VM the client
-  paused and remove its intercepts. With shared memory this needs heartbeats or a side channel.
+- The kernel tells the server when the client goes away (EOF), so one session cleanup point removes
+  its intercepts and resumes only a pause that session created and nobody has since resumed. A
+  pre-existing pause, and a new external pause after the client's pause was resumed, remain paused.
+  With shared memory this needs heartbeats or a side channel.
 - Access control is the socket file's mode (0600) plus a peer uid check (`SO_PEERCRED`), with no
   protocol-level authentication.
 - One ordered channel carries replies and, in phase 2, events (request id 0).
+- While one client is active, the server accepts a second connection only to greet it with
+  `VERR_RESOURCE_BUSY` and close it, rather than leaving it waiting in the listen backlog.
 - `SOCK_SEQPACKET` keeps message boundaries, so neither side frames a byte stream.
 - Each request costs a syscall round trip and a copy. Page-by-page reads of whole guest RAM are
   expected to stay practical; batching covers bulk reads.

tests/common/virtualbox.rs

@@ -74,7 +74,7 @@ fn default_method() -> VBoxInitParams {
 }
 
 /// The configured or default VBoxVMI socket path.
-fn vboxvmi_socket() -> String {
+pub fn vboxvmi_socket() -> String {
     if let Some(path) = extradata(VMI_SOCKET) {
         return path;
     }

tests/integration.rs

@@ -14,6 +14,11 @@ mod common;
 mod tests;
 
 fn main() {
+    #[cfg(all(feature = "virtualbox-vmi", not(feature = "kvm")))]
+    if std::env::var_os("MICROVMI_VBOXVMI_PAUSE_CHILD").is_some() {
+        tests::virtualbox::vboxvmi_pause_child();
+    }
+
     // init logger
     env_logger::builder().is_test(true).init();
     // run only the tests whose name contains MICROVMI_TEST_FILTER

tests/tests/virtualbox.rs

@@ -2,8 +2,16 @@
 use std::convert::TryInto;
 use std::env;
 use std::fs::{self, File};
+#[cfg(feature = "virtualbox-vmi")]
+use std::io::{self, BufRead, BufReader, Write};
 use std::ops::Range;
 use std::os::unix::fs::FileExt;
+#[cfg(feature = "virtualbox-vmi")]
+use std::process::{Child, Command, Stdio};
+#[cfg(feature = "virtualbox-vmi")]
+use std::sync::mpsc;
+#[cfg(feature = "virtualbox-vmi")]
+use std::thread;
 use std::thread::sleep;
 use std::time::{Duration, Instant, SystemTime};
 
@@ -12,6 +20,8 @@ use microvmi::api::registers::{Registers, X86Registers};
 use microvmi::api::{Introspectable, PAGE_SIZE};
 
 use super::IntegrationTest;
+#[cfg(feature = "virtualbox-vmi")]
+use crate::common::virtualbox::vboxvmi_socket;
 use crate::common::virtualbox::{core_path, try_vboxmanage, vboxmanage, vm_info, vm_name};
 
 const PAGE: u64 = PAGE_SIZE as u64;
@@ -318,6 +328,222 @@ inventory::submit!(IntegrationTest {
     test_fn: pause_idempotent_and_visible
 });
 
+#[cfg(feature = "virtualbox-vmi")]
+fn connect_vboxvmi() -> vboxvmi::Client {
+    let deadline = Instant::now() + Duration::from_secs(10);
+    loop {
+        match vboxvmi::Client::connect(vboxvmi_socket()) {
+            Ok(client) => return client,
+            Err(error) if Instant::now() < deadline => {
+                debug!("waiting for the prior VBoxVMI session to close: {}", error);
+                sleep(Duration::from_millis(20));
+            }
+            Err(error) => panic!("failed to connect to VBoxVMI: {}", error),
+        }
+    }
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+fn wait_for_vm_state(expected: &str) {
+    let deadline = Instant::now() + Duration::from_secs(10);
+    loop {
+        let state = vm_info("VMState");
+        if state == expected {
+            return;
+        }
+        assert!(
+            Instant::now() < deadline,
+            "VM did not become {} after VBoxVMI disconnect; state is {}",
+            expected,
+            state
+        );
+        sleep(Duration::from_millis(20));
+    }
+}
+
+/// Entry point for the subprocess killed by `vboxvmi_killed_client_resumes`.
+#[cfg(feature = "virtualbox-vmi")]
+pub fn vboxvmi_pause_child() -> ! {
+    let mut client = connect_vboxvmi();
+    client
+        .pause()
+        .expect("child failed to pause through VBoxVMI");
+    println!("paused");
+    std::io::stdout()
+        .flush()
+        .expect("child failed to flush readiness");
+    loop {
+        sleep(Duration::from_secs(60));
+    }
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+fn vboxvmi_normal_close_resumes(mut drv: Box<dyn Introspectable>) {
+    drv.pause().unwrap();
+    assert_eq!(vm_info("VMState"), "paused");
+    drop(drv);
+    wait_for_vm_state("running");
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+inventory::submit!(IntegrationTest {
+    name: "vboxvmi_disconnect_normal_close_resumes",
+    test_fn: vboxvmi_normal_close_resumes
+});
+
+#[cfg(feature = "virtualbox-vmi")]
+struct ChildGuard(Option<Child>);
+
+#[cfg(feature = "virtualbox-vmi")]
+impl ChildGuard {
+    fn new(child: Child) -> Self {
+        Self(Some(child))
+    }
+
+    fn child_mut(&mut self) -> &mut Child {
+        self.0.as_mut().unwrap()
+    }
+
+    fn kill_and_wait(&mut self) -> io::Result<()> {
+        {
+            let child = self.0.as_mut().unwrap();
+            child.kill()?;
+            child.wait()?;
+        }
+        self.0 = None;
+        Ok(())
+    }
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+impl Drop for ChildGuard {
+    fn drop(&mut self) {
+        if let Some(mut child) = self.0.take() {
+            let _ = child.kill();
+            let _ = child.wait();
+        }
+    }
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+fn vboxvmi_killed_client_resumes(drv: Box<dyn Introspectable>) {
+    drop(drv);
+    let child = Command::new(std::env::current_exe().unwrap())
+        .env("MICROVMI_VBOXVMI_PAUSE_CHILD", "1")
+        .stdout(Stdio::piped())
+        .stderr(Stdio::inherit())
+        .spawn()
+        .expect("failed to start VBoxVMI pause child");
+    let mut child = ChildGuard::new(child);
+    let mut ready = String::new();
+    BufReader::new(child.child_mut().stdout.take().unwrap())
+        .read_line(&mut ready)
+        .expect("failed to read child readiness");
+    assert_eq!(
+        ready.trim_end(),
+        "paused",
+        "VBoxVMI pause child did not become ready"
+    );
+    assert_eq!(vm_info("VMState"), "paused");
+    child
+        .kill_and_wait()
+        .expect("failed to kill and reap VBoxVMI pause child");
+    wait_for_vm_state("running");
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+inventory::submit!(IntegrationTest {
+    name: "vboxvmi_disconnect_killed_client_resumes",
+    test_fn: vboxvmi_killed_client_resumes
+});
+
+#[cfg(feature = "virtualbox-vmi")]
+fn vboxvmi_external_pause_is_not_resumed(mut drv: Box<dyn Introspectable>) {
+    vboxmanage(&["controlvm", &vm_name(), "pause"]);
+    drv.pause().unwrap();
+    drop(drv);
+    drop(connect_vboxvmi()); // accepting the next client proves disconnect cleanup finished
+    assert_eq!(vm_info("VMState"), "paused");
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+inventory::submit!(IntegrationTest {
+    name: "vboxvmi_disconnect_preserves_preexisting_vm_state",
+    test_fn: vboxvmi_external_pause_is_not_resumed
+});
+
+#[cfg(feature = "virtualbox-vmi")]
+fn vboxvmi_resume_generation_preserves_external_pause(mut drv: Box<dyn Introspectable>) {
+    drv.pause().unwrap();
+    vboxmanage(&["controlvm", &vm_name(), "resume"]);
+    vboxmanage(&["controlvm", &vm_name(), "pause"]);
+    drop(drv);
+    drop(connect_vboxvmi()); // accepting the next client proves disconnect cleanup finished
+    assert_eq!(vm_info("VMState"), "paused");
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+inventory::submit!(IntegrationTest {
+    name: "vboxvmi_disconnect_preserves_new_external_vm_state",
+    test_fn: vboxvmi_resume_generation_preserves_external_pause
+});
+
+#[cfg(feature = "virtualbox-vmi")]
+fn vboxvmi_disconnect_race_preserves_external_state(drv: Box<dyn Introspectable>) {
+    drop(drv);
+    for iteration in 0..8 {
+        if iteration != 0 {
+            vboxmanage(&["controlvm", &vm_name(), "resume"]);
+        }
+        let mut client = connect_vboxvmi();
+        client.pause().unwrap();
+
+        let (resumed_tx, resumed_rx) = mpsc::channel();
+        let external = thread::spawn(move || {
+            let _ = try_vboxmanage(&["controlvm", &vm_name(), "resume"]);
+            resumed_tx.send(()).unwrap();
+            vboxmanage(&["controlvm", &vm_name(), "pause"]);
+        });
+        if iteration % 2 != 0 {
+            resumed_rx.recv().unwrap();
+        } else {
+            thread::yield_now();
+        }
+        drop(client);
+        external.join().expect("external resume/pause panicked");
+
+        drop(connect_vboxvmi()); // accepting the next client proves disconnect cleanup finished
+        assert_eq!(
+            vm_info("VMState"),
+            "paused",
+            "disconnect resumed external pause in race iteration {}",
+            iteration
+        );
+    }
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+inventory::submit!(IntegrationTest {
+    name: "vboxvmi_disconnect_race_preserves_external_state",
+    test_fn: vboxvmi_disconnect_race_preserves_external_state
+});
+
+#[cfg(feature = "virtualbox-vmi")]
+fn vboxvmi_second_client_is_busy(drv: Box<dyn Introspectable>) {
+    match vboxvmi::Client::connect(vboxvmi_socket()) {
+        Err(vboxvmi::Error::Busy) => {}
+        Err(error) => panic!("second VBoxVMI client got the wrong error: {}", error),
+        Ok(_) => panic!("second VBoxVMI client connected"),
+    }
+    drop(drv);
+}
+
+#[cfg(feature = "virtualbox-vmi")]
+inventory::submit!(IntegrationTest {
+    name: "vboxvmi_disconnect_second_client_busy",
+    test_fn: vboxvmi_second_client_is_busy
+});
+
 /// Resumes the VM even if selecting pages or dumping the first core fails.
 struct ResumeVm;