Stop leaking muvm configuration into vm environment

Signed-off-by: Sasha Finkelstein <[email protected]>

Author: WhatAmISupposedToPutHere

crates/muvm/src/bin/muvm.rs

@@ -1,3 +1,4 @@
+use std::collections::HashMap;
 use std::env;
 use std::ffi::{c_char, CString};
 use std::io::Write;
@@ -21,23 +22,24 @@ use muvm::launch::{launch_or_lock, LaunchResult, DYNAMIC_PORT_RANGE};
 use muvm::monitor::spawn_monitor;
 use muvm::net::{connect_to_passt, start_passt};
 use muvm::types::MiB;
+use muvm::utils::launch::{GuestConfiguration, Launch};
 use nix::sys::sysinfo::sysinfo;
 use nix::unistd::User;
 use rustix::io::Errno;
 use rustix::process::{
     geteuid, getgid, getrlimit, getuid, sched_setaffinity, setrlimit, CpuSet, Resource,
 };
-use serde::{Deserialize, Serialize};
+use serde::Serialize;
 use tempfile::NamedTempFile;
 
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize)]
 pub struct KrunConfig {
     #[serde(rename = "Cmd")]
     args: Vec<String>,
     #[serde(rename = "Env")]
     envs: Vec<String>,
 }
-#[derive(Serialize, Deserialize)]
+#[derive(Serialize)]
 pub struct KrunBaseConfig {
     #[serde(rename = "Config")]
     config: KrunConfig,
@@ -223,13 +225,10 @@ fn main() -> Result<ExitCode> {
             .collect()
     };
 
-    if options.merged_rootfs {
-        if disks.is_empty() {
-            return Err(anyhow!(
-                "Merged RootFS mode requires one or more RootFS images"
-            ));
-        }
-        env.insert("FEX_MERGEDROOTFS".to_owned(), "1".to_owned());
+    if options.merged_rootfs && disks.is_empty() {
+        return Err(anyhow!(
+            "Merged RootFS mode requires one or more RootFS images"
+        ));
     }
 
     for path in disks {
@@ -385,55 +384,67 @@ fn main() -> Result<ExitCode> {
 
     let muvm_guest_path = find_muvm_exec("muvm-guest")?;
 
-    let mut muvm_guest_args: Vec<String> = vec![
+    let display = env::var("DISPLAY").ok();
+    let guest_config = GuestConfiguration {
+        command: Launch {
+            cookie,
+            command,
+            command_args,
+            env: HashMap::new(),
+            vsock_port: 0,
+            tty: false,
+            privileged: false,
+        },
+        server_port: options.server_port,
+        username,
+        uid: getuid().as_raw(),
+        gid: getgid().as_raw(),
+        host_display: display,
+        server_cookie: cookie,
+        merged_rootfs: options.merged_rootfs,
+    };
+    let mut muvm_config_file = NamedTempFile::new()
+        .context("Failed to create a temporary file to store the muvm guest config")?;
+    write!(
+        muvm_config_file,
+        "{}",
+        serde_json::to_string(&guest_config)
+            .context("Failed to transform GuestConfiguration into a JSON string")?
+    )
+    .context("Failed to write to temporary config file")?;
+
+    let muvm_config_path = muvm_config_file
+        .path()
+        .to_str()
+        .context("Temporary directory path contains invalid UTF-8")?
+        .to_owned();
+    let muvm_guest_args = vec![
         muvm_guest_path
             .to_str()
             .context("Failed to process `muvm-guest` path as it contains invalid UTF-8")?
             .to_owned(),
-        username,
-        format!("{uid}", uid = getuid().as_raw()),
-        format!("{gid}", gid = getgid().as_raw()),
-        command
-            .to_str()
-            .context("Failed to process command as it contains invalid UTF-8")?
-            .to_string(),
+        muvm_config_path,
     ];
-    for arg in command_args {
-        muvm_guest_args.push(arg);
-    }
-
-    env.insert(
-        "MUVM_SERVER_PORT".to_owned(),
-        options.server_port.to_string(),
-    );
-    env.insert("MUVM_SERVER_COOKIE".to_owned(), cookie.to_string());
-
-    let display = env::var("DISPLAY").context("X11 forwarding requested but DISPLAY is unset")?;
-    env.insert("HOST_DISPLAY".to_string(), display);
 
     // And forward XAUTHORITY. This will be modified to fix the
     // display name in muvm-guest.
     if let Ok(xauthority) = env::var("XAUTHORITY") {
-        env.insert("XAUTHORITY".to_string(), xauthority);
+        env.insert("XAUTHORITY".to_owned(), xauthority);
     }
 
-    let mut krun_config = KrunConfig {
-        args: Vec::new(),
-        envs: Vec::new(),
-    };
-    for arg in muvm_guest_args {
-        krun_config.args.push(arg);
-    }
-    for (key, value) in env {
-        krun_config.envs.push(format!("{}={}", key, value));
-    }
     let krun_config = KrunBaseConfig {
-        config: krun_config,
+        config: KrunConfig {
+            args: muvm_guest_args,
+            envs: env
+                .into_iter()
+                .map(|(key, value)| format!("{key}={value}"))
+                .collect(),
+        },
     };
 
     // SAFETY: `config_file` lifetime needs to exceed krun_start_enter's
     let mut config_file = NamedTempFile::new()
-        .context("Failed to create a temporary file to store the guest config")?;
+        .context("Failed to create a temporary file to store the krun config")?;
     write!(
         config_file,
         "{}",

crates/muvm/src/guest/bin/muvm-guest.rs

@@ -1,10 +1,11 @@
+use std::fs::File;
+use std::io::Read;
 use std::os::fd::AsFd;
 use std::panic::catch_unwind;
 use std::process::Command;
-use std::{cmp, env, thread};
+use std::{cmp, env, fs, thread};
 
 use anyhow::{Context, Result};
-use muvm::guest::cli_options::options;
 use muvm::guest::fex::setup_fex;
 use muvm::guest::hidpipe::start_hidpipe;
 use muvm::guest::mount::mount_filesystems;
@@ -14,8 +15,12 @@ use muvm::guest::socket::setup_socket_proxy;
 use muvm::guest::user::setup_user;
 use muvm::guest::x11::setup_x11_forwarding;
 use muvm::guest::x11bridge::start_x11bridge;
+use muvm::utils::launch::GuestConfiguration;
+use nix::unistd::{Gid, Uid};
 use rustix::process::{getrlimit, setrlimit, Resource};
 
+const KRUN_CONFIG: &str = "KRUN_CONFIG";
+
 fn main() -> Result<()> {
     env_logger::init();
 
@@ -24,7 +29,22 @@ fn main() -> Result<()> {
         return Ok(());
     }
 
-    let options = options().run();
+    let config_path = env::args()
+        .nth(1)
+        .context("expected configuration file path")?;
+    let mut config_file = File::open(&config_path)?;
+    let mut config_buf = Vec::new();
+    config_file.read_to_end(&mut config_buf)?;
+    fs::remove_file(config_path).context("Unable to delete temporary muvm configuration file")?;
+    if let Ok(krun_config_path) = env::var(KRUN_CONFIG) {
+        fs::remove_file(krun_config_path)
+            .context("Unable to delete temporary krun configuration file")?;
+        // SAFETY: We are single-threaded at this point
+        env::remove_var(KRUN_CONFIG);
+    }
+    // SAFETY: We are single-threaded at this point
+    env::remove_var("KRUN_WORKDIR");
+    let options = serde_json::from_slice::<GuestConfiguration>(&config_buf)?;
 
     {
         const ESYNC_RLIMIT_NOFILE: u64 = 524288;
@@ -41,7 +61,7 @@ fn main() -> Result<()> {
         setrlimit(Resource::Nofile, rlim).context("Failed to raise `RLIMIT_NOFILE`")?;
     }
 
-    if let Err(err) = mount_filesystems() {
+    if let Err(err) = mount_filesystems(options.merged_rootfs) {
         return Err(err).context("Failed to mount filesystems, bailing out");
     }
 
@@ -61,7 +81,11 @@ fn main() -> Result<()> {
 
     configure_network()?;
 
-    let run_path = match setup_user(options.username, options.uid, options.gid) {
+    let run_path = match setup_user(
+        options.username,
+        Uid::from(options.uid),
+        Gid::from(options.gid),
+    ) {
         Ok(p) => p,
         Err(err) => return Err(err).context("Failed to set up user, bailing out"),
     };
@@ -72,9 +96,11 @@ fn main() -> Result<()> {
     let pulse_path = pulse_path.join("native");
     setup_socket_proxy(pulse_path, 3333)?;
 
-    setup_x11_forwarding(run_path)?;
+    if let Some(host_display) = options.host_display {
+        setup_x11_forwarding(run_path, &host_display)?;
+    }
 
-    let uid = options.uid.as_raw();
+    let uid = options.uid;
     thread::spawn(move || {
         if catch_unwind(|| start_hidpipe(uid)).is_err() {
             eprintln!("hidpipe thread crashed, input device passthrough will no longer function");
@@ -83,6 +109,12 @@ fn main() -> Result<()> {
 
     let rt = tokio::runtime::Runtime::new().unwrap();
     rt.block_on(async {
-        server_main(options.server_port, options.command, options.command_args).await
+        server_main(
+            options.server_port,
+            options.server_cookie,
+            options.command.command,
+            options.command.command_args,
+        )
+        .await
     })
 }

crates/muvm/src/guest/cli_options.rs

@@ -1,4 +1,3 @@
-pub mod cli_options;
 pub mod fex;
 pub mod hidpipe;
 pub mod mount;

crates/muvm/src/guest/mod.rs

@@ -1,5 +1,4 @@
 use std::collections::HashSet;
-use std::env;
 use std::ffi::CString;
 use std::fs::{read_dir, read_link, File};
 use std::io::Write;
@@ -50,7 +49,7 @@ fn do_mount_recursive_bind(source: &str, target: PathBuf) -> Result<()> {
     Ok(())
 }
 
-fn mount_fex_rootfs() -> Result<()> {
+fn mount_fex_rootfs(merged_rootfs: bool) -> Result<()> {
     let dir = "/run/fex-emu/";
     let dir_rootfs = dir.to_string() + "rootfs";
 
@@ -60,10 +59,6 @@ fn mount_fex_rootfs() -> Result<()> {
     let flags = MountFlags::RDONLY;
     let mut images = Vec::new();
 
-    let merged_rootfs = env::var("FEX_MERGEDROOTFS")
-        .map(|a| a != "0")
-        .unwrap_or(false);
-
     // In merged RootFS mode, make /run/fex-emu a tmpfs.
     // This ensures that once /run is bind-mounted into the
     // rootfs, /run/fex-emu/* isn't itself visible within the
@@ -95,11 +90,7 @@ fn mount_fex_rootfs() -> Result<()> {
     if images.is_empty() {
         // If no images were passed, FEX is either managed by the host os
         // or is not installed at all. Avoid clobbering the config in that case.
-        // merged_rootfs is ignored in this case, and we unset the env var so
-        // the state of MergedRootFS is strictly managed by the host config.
-        // TODO: Remove once #134 is merged, move merged_rootfs to config.
-        // SAFETY: muvm-guest is single-threaded.
-        unsafe { env::remove_var("FEX_MERGEDROOTFS") };
+        // merged_rootfs is ignored in this case.
         return Ok(());
     }
 
@@ -285,7 +276,7 @@ pub fn place_file(backing: &str, dest: &str, contents: Option<&str>) -> Result<(
     overlay_file(backing, dest)
 }
 
-pub fn mount_filesystems() -> Result<()> {
+pub fn mount_filesystems(merged_rootfs: bool) -> Result<()> {
     make_tmpfs("/var/run")?;
 
     place_file("/run/resolv.conf", "/etc/resolv.conf", None)?;
@@ -323,7 +314,7 @@ pub fn mount_filesystems() -> Result<()> {
     .context("Failed to mount `/dev/shm`")?;
 
     // Do this last so it can pick up all the submounts made above.
-    if let Err(e) = mount_fex_rootfs() {
+    if let Err(e) = mount_fex_rootfs(merged_rootfs) {
         println!(
             "Failed to mount FEX rootfs, carrying on without. Error: {}",
             e