Fix zipfuse race condition

We mount the APK by running zipfuse (asynchronously), and then try to
run the payload from within it. There is a race condition - if the
mount hasn't happened, the payload binary won't appear.

This showed up when I switched to config-less VMs (so we no longer
read the JSON configuration between the two operations) - but only on
cuttlefish, and only in non-debug mode.

Fix it by using a property to signal when the mount has completed.

Bug: 243513572
Test: atest MicrodroidTests (locally & via acloud)
Change-Id: Ib777e7f28afafebd128f8e0c149d485ab9351273

microdroid_manager/src/main.rs

@@ -71,6 +71,7 @@ const VMADDR_CID_HOST: u32 = 2;
 
 const APEX_CONFIG_DONE_PROP: &str = "apex_config.done";
 const APP_DEBUGGABLE_PROP: &str = "ro.boot.microdroid.app_debuggable";
+const APK_MOUNT_DONE_PROP: &str = "microdroid_manager.apk.mounted";
 
 // SYNC WITH virtualizationservice/src/crosvm.rs
 const FAILURE_SERIAL_DEVICE: &str = "/dev/ttyS1";
@@ -344,12 +345,12 @@ fn try_run_payload(service: &Strong<dyn IVirtualMachineService>) -> Result<i32>
     dice_derivation(&verified_data, &payload_metadata)?;
 
     // Before reading a file from the APK, start zipfuse
-    let noexec = false;
     run_zipfuse(
-        noexec,
+        MountForExec::Allowed,
         "fscontext=u:object_r:zipfusefs:s0,context=u:object_r:system_file:s0",
         Path::new("/dev/block/mapper/microdroid-apk"),
         Path::new("/mnt/apk"),
+        Some(APK_MOUNT_DONE_PROP),
     )
     .context("Failed to run zipfuse")?;
 
@@ -385,6 +386,9 @@ fn try_run_payload(service: &Strong<dyn IVirtualMachineService>) -> Result<i32>
         control_service("start", "authfs_service")?;
     }
 
+    // Wait until zipfuse has mounted the APK so we can access the payload
+    wait_for_property_true(APK_MOUNT_DONE_PROP).context("Failed waiting for APK mount done")?;
+
     system_properties::write("dev.bootcomplete", "1").context("set dev.bootcomplete")?;
     exec_task(task, service)
 }
@@ -418,11 +422,25 @@ fn run_apkdmverity(args: &[ApkDmverityArgument]) -> Result<Child> {
     cmd.spawn().context("Spawn apkdmverity")
 }
 
-fn run_zipfuse(noexec: bool, option: &str, zip_path: &Path, mount_dir: &Path) -> Result<Child> {
+enum MountForExec {
+    Allowed,
+    Disallowed,
+}
+
+fn run_zipfuse(
+    noexec: MountForExec,
+    option: &str,
+    zip_path: &Path,
+    mount_dir: &Path,
+    ready_prop: Option<&str>,
+) -> Result<Child> {
     let mut cmd = Command::new(ZIPFUSE_BIN);
-    if noexec {
+    if let MountForExec::Disallowed = noexec {
         cmd.arg("--noexec");
     }
+    if let Some(property_name) = ready_prop {
+        cmd.args(["-p", property_name]);
+    }
     cmd.arg("-o")
         .arg(option)
         .arg(zip_path)
@@ -608,12 +626,12 @@ fn mount_extra_apks(config: &VmPayloadConfig) -> Result<()> {
         create_dir(Path::new(&mount_dir)).context("Failed to create mount dir for extra apks")?;
 
         // don't wait, just detach
-        let noexec = true;
         run_zipfuse(
-            noexec,
+            MountForExec::Disallowed,
             "fscontext=u:object_r:zipfusefs:s0,context=u:object_r:extra_apk_file:s0",
             Path::new(&format!("/dev/block/mapper/extra-apk-{}", i)),
             Path::new(&mount_dir),
+            None,
         )
         .context("Failed to zipfuse extra apks")?;
     }
@@ -623,10 +641,14 @@ fn mount_extra_apks(config: &VmPayloadConfig) -> Result<()> {
 
 // Waits until linker config is generated
 fn wait_for_apex_config_done() -> Result<()> {
-    let mut prop = PropertyWatcher::new(APEX_CONFIG_DONE_PROP)?;
+    wait_for_property_true(APEX_CONFIG_DONE_PROP).context("Failed waiting for apex config done")
+}
+
+fn wait_for_property_true(property_name: &str) -> Result<()> {
+    let mut prop = PropertyWatcher::new(property_name)?;
     loop {
         prop.wait()?;
-        if system_properties::read_bool(APEX_CONFIG_DONE_PROP, false)? {
+        if system_properties::read_bool(property_name, false)? {
             break;
         }
     }

zipfuse/Android.bp

@@ -13,9 +13,10 @@ rust_defaults {
         "libclap",
         "libfuse_rust",
         "liblibc",
-        "libzip",
-        "libscopeguard",
         "liblog_rust",
+        "librustutils",
+        "libscopeguard",
+        "libzip",
     ],
     // libfuse_rust, etc don't support 32-bit targets
     multilib: {

zipfuse/src/main.rs

@@ -20,10 +20,11 @@
 
 mod inode;
 
-use anyhow::Result;
+use anyhow::{Context as AnyhowContext, Result};
 use clap::{App, Arg};
 use fuse::filesystem::*;
 use fuse::mount::*;
+use rustutils::system_properties;
 use std::collections::HashMap;
 use std::convert::TryFrom;
 use std::ffi::{CStr, CString};
@@ -52,6 +53,12 @@ fn main() -> Result<()> {
                 .takes_value(false)
                 .help("Disallow the execution of binary files"),
         )
+        .arg(
+            Arg::with_name("readyprop")
+                .short('p')
+                .takes_value(true)
+                .help("Specify a property to be set when mount is ready"),
+        )
         .arg(Arg::with_name("ZIPFILE").required(true))
         .arg(Arg::with_name("MOUNTPOINT").required(true))
         .get_matches();
@@ -60,7 +67,8 @@ fn main() -> Result<()> {
     let mount_point = matches.value_of("MOUNTPOINT").unwrap().as_ref();
     let options = matches.value_of("options");
     let noexec = matches.is_present("noexec");
-    run_fuse(zip_file, mount_point, options, noexec)?;
+    let ready_prop = matches.value_of("readyprop");
+    run_fuse(zip_file, mount_point, options, noexec, ready_prop)?;
     Ok(())
 }
 
@@ -70,6 +78,7 @@ pub fn run_fuse(
     mount_point: &Path,
     extra_options: Option<&str>,
     noexec: bool,
+    ready_prop: Option<&str>,
 ) -> Result<()> {
     const MAX_READ: u32 = 1 << 20; // TODO(jiyong): tune this
     const MAX_WRITE: u32 = 1 << 13; // This is a read-only filesystem
@@ -94,6 +103,11 @@ pub fn run_fuse(
     }
 
     fuse::mount(mount_point, "zipfuse", mount_flags, &mount_options)?;
+
+    if let Some(property_name) = ready_prop {
+        system_properties::write(property_name, "1").context("Failed to set readyprop")?;
+    }
+
     let mut config = fuse::FuseConfig::new();
     config.dev_fuse(dev_fuse).max_write(MAX_WRITE).max_read(MAX_READ);
     Ok(config.enter_message_loop(ZipFuse::new(zip_file)?)?)