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
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index b4b2c8b..20fc104 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -71,6 +71,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
// 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;
}
}
diff --git a/zipfuse/Android.bp b/zipfuse/Android.bp
index 3aba94a..1bdc5fe 100644
--- a/zipfuse/Android.bp
+++ b/zipfuse/Android.bp
@@ -13,9 +13,10 @@
"libclap",
"libfuse_rust",
"liblibc",
- "libzip",
- "libscopeguard",
"liblog_rust",
+ "librustutils",
+ "libscopeguard",
+ "libzip",
],
// libfuse_rust, etc don't support 32-bit targets
multilib: {
diff --git a/zipfuse/src/main.rs b/zipfuse/src/main.rs
index 8400a72..9411759 100644
--- a/zipfuse/src/main.rs
+++ b/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 @@
.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 @@
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 @@
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 @@
}
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)?)?)