Use new OdrefreshArgs in AIDL to simplify arguments
Using the parcelable directly simplies the arguments. Although it does
have two problems in the current form:
* Naming convension (unless we can break the rule of AIDL's)
* No Option<>. Optional argument requires the old-school check for
reserved/invalid value.
Bug: 248528901
Test: atest ComposHostTestCases
Change-Id: Ib87e9b1f998e803756d5a06aadcb92b8a3e2b8f4
diff --git a/compos/aidl/com/android/compos/ICompOsService.aidl b/compos/aidl/com/android/compos/ICompOsService.aidl
index bfde6b6..df8c91e 100644
--- a/compos/aidl/com/android/compos/ICompOsService.aidl
+++ b/compos/aidl/com/android/compos/ICompOsService.aidl
@@ -42,28 +42,41 @@
TEST_COMPILE = 1,
}
+ /** Arguments to run odrefresh */
+ parcelable OdrefreshArgs {
+ /** The type of compilation to be performed */
+ CompilationMode compilationMode = CompilationMode.NORMAL_COMPILE;
+ /** An fd referring to /system */
+ int systemDirFd = -1;
+ /** An optional fd referring to /system_ext. Negative number means none. */
+ int systemExtDirFd = -1;
+ /** An fd referring to the output directory, ART_APEX_DATA */
+ int outputDirFd = -1;
+ /** An fd referring to the staging directory, e.g. ART_APEX_DATA/staging */
+ int stagingDirFd = -1;
+ /**
+ * The sub-directory of the output directory to which artifacts are to be written (e.g.
+ * dalvik-cache)
+ */
+ String targetDirName;
+ /** The zygote architecture (ro.zygote) */
+ String zygoteArch;
+ /** The compiler filter used to compile system server */
+ String systemServerCompilerFilter;
+ }
+
/**
* Run odrefresh in the VM context.
*
* The execution is based on the VM's APEX mounts, files on Android's /system and optionally
- * /system_ext (by accessing through systemDirFd and systemExtDirFd over AuthFS), and
- * *CLASSPATH derived in the VM, to generate the same odrefresh output artifacts to the output
- * directory (through outputDirFd).
+ * /system_ext (by accessing through OdrefreshArgs.systemDirFd and OdrefreshArgs.systemExtDirFd
+ * over AuthFS), and *CLASSPATH derived in the VM, to generate the same odrefresh output
+ * artifacts to the output directory (through OdrefreshArgs.outputDirFd).
*
- * @param compilationMode The type of compilation to be performed
- * @param systemDirFd An fd referring to /system
- * @param systemExtDirFd An optional fd referring to /system_ext. Negative number means none.
- * @param outputDirFd An fd referring to the output directory, ART_APEX_DATA
- * @param stagingDirFd An fd referring to the staging directory, e.g. ART_APEX_DATA/staging
- * @param targetDirName The sub-directory of the output directory to which artifacts are to be
- * written (e.g. dalvik-cache)
- * @param zygoteArch The zygote architecture (ro.zygote)
- * @param systemServerCompilerFilter The compiler filter used to compile system server
+ * @param args Arguments to configure the odrefresh context
* @return odrefresh exit code
*/
- byte odrefresh(CompilationMode compilation_mode, int systemDirFd, int systemExtDirFd,
- int outputDirFd, int stagingDirFd, String targetDirName, String zygoteArch,
- String systemServerCompilerFilter);
+ byte odrefresh(in OdrefreshArgs args);
/**
* Returns the current VM's signing key, as an Ed25519 public key
diff --git a/compos/composd/src/odrefresh_task.rs b/compos/composd/src/odrefresh_task.rs
index 9276fb1..3a699ab 100644
--- a/compos/composd/src/odrefresh_task.rs
+++ b/compos/composd/src/odrefresh_task.rs
@@ -25,7 +25,7 @@
use anyhow::{Context, Result};
use binder::{Interface, Result as BinderResult, Strong};
use compos_aidl_interface::aidl::com::android::compos::ICompOsService::{
- CompilationMode::CompilationMode, ICompOsService,
+ CompilationMode::CompilationMode, ICompOsService, OdrefreshArgs::OdrefreshArgs,
};
use compos_common::odrefresh::{
is_system_property_interesting, ExitCode, ODREFRESH_OUTPUT_ROOT_DIR,
@@ -180,16 +180,18 @@
let zygote_arch = system_properties::read("ro.zygote")?.context("ro.zygote not set")?;
let system_server_compiler_filter =
system_properties::read("dalvik.vm.systemservercompilerfilter")?.unwrap_or_default();
- let exit_code = service.odrefresh(
- compilation_mode,
- system_dir_raw_fd,
- system_ext_dir_raw_fd,
- output_dir_raw_fd,
- staging_dir_raw_fd,
- target_dir_name,
- &zygote_arch,
- &system_server_compiler_filter,
- )?;
+
+ let args = OdrefreshArgs {
+ compilationMode: compilation_mode,
+ systemDirFd: system_dir_raw_fd,
+ systemExtDirFd: system_ext_dir_raw_fd,
+ outputDirFd: output_dir_raw_fd,
+ stagingDirFd: staging_dir_raw_fd,
+ targetDirName: target_dir_name.to_string(),
+ zygoteArch: zygote_arch,
+ systemServerCompilerFilter: system_server_compiler_filter,
+ };
+ let exit_code = service.odrefresh(&args)?;
drop(fd_server_raii);
ExitCode::from_i32(exit_code.into())
diff --git a/compos/src/compilation.rs b/compos/src/compilation.rs
index d165599..2872d95 100644
--- a/compos/src/compilation.rs
+++ b/compos/src/compilation.rs
@@ -33,95 +33,65 @@
IAuthFsService::IAuthFsService,
};
use binder::Strong;
-use compos_aidl_interface::aidl::com::android::compos::ICompOsService::CompilationMode::CompilationMode;
+use compos_aidl_interface::aidl::com::android::compos::ICompOsService::{
+ CompilationMode::CompilationMode, OdrefreshArgs::OdrefreshArgs,
+};
use compos_common::odrefresh::ExitCode;
const FD_SERVER_PORT: i32 = 3264; // TODO: support dynamic port
-pub struct OdrefreshContext<'a> {
- compilation_mode: CompilationMode,
- system_dir_fd: i32,
- system_ext_dir_fd: Option<i32>,
- output_dir_fd: i32,
- staging_dir_fd: i32,
- target_dir_name: &'a str,
- zygote_arch: &'a str,
- system_server_compiler_filter: &'a str,
-}
-
-impl<'a> OdrefreshContext<'a> {
- #[allow(clippy::too_many_arguments)]
- pub fn new(
- compilation_mode: CompilationMode,
- system_dir_fd: i32,
- system_ext_dir_fd: Option<i32>,
- output_dir_fd: i32,
- staging_dir_fd: i32,
- target_dir_name: &'a str,
- zygote_arch: &'a str,
- system_server_compiler_filter: &'a str,
- ) -> Result<Self> {
- if compilation_mode != CompilationMode::NORMAL_COMPILE {
- // Conservatively check debuggability.
- let debuggable =
- system_properties::read_bool("ro.boot.microdroid.app_debuggable", false)
- .unwrap_or(false);
- if !debuggable {
- bail!("Requested compilation mode only available in debuggable VMs");
- }
+fn validate_args(args: &OdrefreshArgs) -> Result<()> {
+ if args.compilationMode != CompilationMode::NORMAL_COMPILE {
+ // Conservatively check debuggability.
+ let debuggable = system_properties::read_bool("ro.boot.microdroid.app_debuggable", false)
+ .unwrap_or(false);
+ if !debuggable {
+ bail!("Requested compilation mode only available in debuggable VMs");
}
-
- if system_dir_fd < 0 || output_dir_fd < 0 || staging_dir_fd < 0 {
- bail!("The remote FDs are expected to be non-negative");
- }
- if !matches!(zygote_arch, "zygote64" | "zygote64_32") {
- bail!("Invalid zygote arch");
- }
- // Disallow any sort of path traversal
- if target_dir_name.contains(path::MAIN_SEPARATOR) {
- bail!("Invalid target directory {}", target_dir_name);
- }
-
- // We're not validating/allowlisting the compiler filter, and just assume the compiler will
- // reject an invalid string. We need to accept "verify" filter anyway, and potential
- // performance degration by the attacker is not currently in scope. This also allows ART to
- // specify new compiler filter and configure through system property without change to
- // CompOS.
-
- Ok(Self {
- compilation_mode,
- system_dir_fd,
- system_ext_dir_fd,
- output_dir_fd,
- staging_dir_fd,
- target_dir_name,
- zygote_arch,
- system_server_compiler_filter,
- })
}
+
+ if args.systemDirFd < 0 || args.outputDirFd < 0 || args.stagingDirFd < 0 {
+ bail!("The remote FDs are expected to be non-negative");
+ }
+ if !matches!(&args.zygoteArch[..], "zygote64" | "zygote64_32") {
+ bail!("Invalid zygote arch");
+ }
+ // Disallow any sort of path traversal
+ if args.targetDirName.contains(path::MAIN_SEPARATOR) {
+ bail!("Invalid target directory {}", args.targetDirName);
+ }
+
+ // We're not validating/allowlisting the compiler filter, and just assume the compiler will
+ // reject an invalid string. We need to accept "verify" filter anyway, and potential
+ // performance degration by the attacker is not currently in scope. This also allows ART to
+ // specify new compiler filter and configure through system property without change to
+ // CompOS.
+ Ok(())
}
pub fn odrefresh<F>(
odrefresh_path: &Path,
- context: OdrefreshContext,
+ args: &OdrefreshArgs,
authfs_service: Strong<dyn IAuthFsService>,
success_fn: F,
) -> Result<ExitCode>
where
F: FnOnce(PathBuf) -> Result<()>,
{
+ validate_args(args)?;
+
// Mount authfs (via authfs_service). The authfs instance unmounts once the `authfs` variable
// is out of scope.
let mut input_dir_fd_annotations = vec![InputDirFdAnnotation {
- fd: context.system_dir_fd,
+ fd: args.systemDirFd,
// Use the 0th APK of the extra_apks in compos/apk/assets/vm_config*.json
manifestPath: "/mnt/extra-apk/0/assets/build_manifest.pb".to_string(),
prefix: "system/".to_string(),
}];
- if let Some(fd) = context.system_ext_dir_fd {
+ if args.systemExtDirFd >= 0 {
input_dir_fd_annotations.push(InputDirFdAnnotation {
- fd,
+ fd: args.systemExtDirFd,
// Use the 1st APK of the extra_apks in compos/apk/assets/vm_config_system_ext_*.json
manifestPath: "/mnt/extra-apk/1/assets/build_manifest.pb".to_string(),
prefix: "system_ext/".to_string(),
@@ -132,8 +102,8 @@
port: FD_SERVER_PORT,
inputDirFdAnnotations: input_dir_fd_annotations,
outputDirFdAnnotations: vec![
- OutputDirFdAnnotation { fd: context.output_dir_fd },
- OutputDirFdAnnotation { fd: context.staging_dir_fd },
+ OutputDirFdAnnotation { fd: args.outputDirFd },
+ OutputDirFdAnnotation { fd: args.stagingDirFd },
],
..Default::default()
};
@@ -144,52 +114,50 @@
let mut odrefresh_vars = EnvMap::from_current_env();
let mut android_root = mountpoint.clone();
- android_root.push(context.system_dir_fd.to_string());
+ android_root.push(args.systemDirFd.to_string());
android_root.push("system");
odrefresh_vars.set("ANDROID_ROOT", path_to_str(&android_root)?);
debug!("ANDROID_ROOT={:?}", &android_root);
- if let Some(fd) = context.system_ext_dir_fd {
+ if args.systemExtDirFd >= 0 {
let mut system_ext_root = mountpoint.clone();
- system_ext_root.push(fd.to_string());
+ system_ext_root.push(args.systemExtDirFd.to_string());
system_ext_root.push("system_ext");
odrefresh_vars.set("SYSTEM_EXT_ROOT", path_to_str(&system_ext_root)?);
debug!("SYSTEM_EXT_ROOT={:?}", &system_ext_root);
}
- let art_apex_data = mountpoint.join(context.output_dir_fd.to_string());
+ let art_apex_data = mountpoint.join(args.outputDirFd.to_string());
odrefresh_vars.set("ART_APEX_DATA", path_to_str(&art_apex_data)?);
debug!("ART_APEX_DATA={:?}", &art_apex_data);
- let staging_dir = mountpoint.join(context.staging_dir_fd.to_string());
+ let staging_dir = mountpoint.join(args.stagingDirFd.to_string());
set_classpaths(&mut odrefresh_vars, &android_root)?;
- let mut args = vec![
+ let mut command_line_args = vec![
"odrefresh".to_string(),
"--compilation-os-mode".to_string(),
- format!("--zygote-arch={}", context.zygote_arch),
- format!("--dalvik-cache={}", context.target_dir_name),
+ format!("--zygote-arch={}", args.zygoteArch),
+ format!("--dalvik-cache={}", args.targetDirName),
format!("--staging-dir={}", staging_dir.display()),
"--no-refresh".to_string(),
];
- if !context.system_server_compiler_filter.is_empty() {
- args.push(format!(
- "--system-server-compiler-filter={}",
- context.system_server_compiler_filter
- ));
+ if !args.systemServerCompilerFilter.is_empty() {
+ command_line_args
+ .push(format!("--system-server-compiler-filter={}", args.systemServerCompilerFilter));
}
- let compile_flag = match context.compilation_mode {
+ let compile_flag = match args.compilationMode {
CompilationMode::NORMAL_COMPILE => "--compile",
CompilationMode::TEST_COMPILE => "--force-compile",
other => bail!("Unknown compilation mode {:?}", other),
};
- args.push(compile_flag.to_string());
+ command_line_args.push(compile_flag.to_string());
- debug!("Running odrefresh with args: {:?}", &args);
- let jail = spawn_jailed_task(odrefresh_path, &args, &odrefresh_vars.into_env())
+ debug!("Running odrefresh with args: {:?}", &command_line_args);
+ let jail = spawn_jailed_task(odrefresh_path, &command_line_args, &odrefresh_vars.into_env())
.context("Spawn odrefresh")?;
let exit_code = match jail.wait() {
Ok(_) => 0,
@@ -201,7 +169,7 @@
info!("odrefresh exited with {:?}", exit_code);
if exit_code == ExitCode::CompilationSuccess {
- let target_dir = art_apex_data.join(context.target_dir_name);
+ let target_dir = art_apex_data.join(&args.targetDirName);
success_fn(target_dir)?;
}
diff --git a/compos/src/compsvc.rs b/compos/src/compsvc.rs
index 3dbb4da..4330bbf 100644
--- a/compos/src/compsvc.rs
+++ b/compos/src/compsvc.rs
@@ -28,11 +28,11 @@
use std::sync::RwLock;
use crate::artifact_signer::ArtifactSigner;
-use crate::compilation::{odrefresh, OdrefreshContext};
+use crate::compilation::odrefresh;
use crate::compos_key;
use binder::{BinderFeatures, ExceptionCode, Interface, Result as BinderResult, Status, Strong};
use compos_aidl_interface::aidl::com::android::compos::ICompOsService::{
- BnCompOsService, CompilationMode::CompilationMode, ICompOsService,
+ BnCompOsService, ICompOsService, OdrefreshArgs::OdrefreshArgs,
};
use compos_common::binder::to_binder_result;
use compos_common::odrefresh::{is_system_property_interesting, ODREFRESH_PATH};
@@ -98,17 +98,7 @@
Ok(())
}
- fn odrefresh(
- &self,
- compilation_mode: CompilationMode,
- system_dir_fd: i32,
- system_ext_dir_fd: i32,
- output_dir_fd: i32,
- staging_dir_fd: i32,
- target_dir_name: &str,
- zygote_arch: &str,
- system_server_compiler_filter: &str,
- ) -> BinderResult<i8> {
+ fn odrefresh(&self, args: &OdrefreshArgs) -> BinderResult<i8> {
let initialized = *self.initialized.read().unwrap();
if !initialized.unwrap_or(false) {
return Err(Status::new_exception_str(
@@ -117,18 +107,7 @@
));
}
- let context = OdrefreshContext::new(
- compilation_mode,
- system_dir_fd,
- if system_ext_dir_fd >= 0 { Some(system_ext_dir_fd) } else { None },
- output_dir_fd,
- staging_dir_fd,
- target_dir_name,
- zygote_arch,
- system_server_compiler_filter,
- );
-
- to_binder_result(context.and_then(|c| self.do_odrefresh(c)))
+ to_binder_result(self.do_odrefresh(args))
}
fn getPublicKey(&self) -> BinderResult<Vec<u8>> {
@@ -147,10 +126,10 @@
}
impl CompOsService {
- fn do_odrefresh(&self, context: OdrefreshContext) -> Result<i8> {
+ fn do_odrefresh(&self, args: &OdrefreshArgs) -> Result<i8> {
let authfs_service = binder::get_interface(AUTHFS_SERVICE_NAME)
.context("Unable to connect to AuthFS service")?;
- let exit_code = odrefresh(&self.odrefresh_path, context, authfs_service, |output_dir| {
+ let exit_code = odrefresh(&self.odrefresh_path, args, authfs_service, |output_dir| {
// authfs only shows us the files we created, so it's ok to just sign everything
// under the output directory.
let mut artifact_signer = ArtifactSigner::new(&output_dir);