RPC Server never returns
In normal operation AVmPayload_runVsockRpcServer should never return -
the calling thread joins the server's thread pool, and we provide no
way to shut down the server. If the server does exit, that indicates a
failure somewhere.
If there is a failure of any sort (including unexepected server exit)
there is nothing the caller can do, so we panic rather than returning
a bool.
Update callers to not expect a return value.
I got slightly carried away and also:
- Modified compsvc to use AVmPayload_runVsockRpcServer rather than
rolling its own.
- Turned on unsafe_op_in_unsafe_fn in the API implementation. That
requires us to explicitly mark unsafe blocks in unsafe functions, so
I've gone through and done that. I checked that all the top-level
functions that should be marked unsafe are.
Bug: 243512108
Test: atest MicrodroidTests
Test: composd_cmd test-compile
Change-Id: I447ce0baa09d6a244ffe2ba7ab08092be3cd0f82
diff --git a/compos/src/compsvc_main.rs b/compos/src/compsvc_main.rs
index 206dd4b..77e2daa 100644
--- a/compos/src/compsvc_main.rs
+++ b/compos/src/compsvc_main.rs
@@ -23,11 +23,13 @@
mod fsverity;
use anyhow::Result;
+use binder::unstable_api::AsNative;
use compos_common::COMPOS_VSOCK_PORT;
use log::{debug, error};
-use rpcbinder::RpcServer;
+use std::os::raw::c_void;
use std::panic;
-use vm_payload_bindgen::AVmPayload_notifyPayloadReady;
+use std::ptr;
+use vm_payload_bindgen::{AIBinder, AVmPayload_notifyPayloadReady, AVmPayload_runVsockRpcServer};
fn main() {
if let Err(e) = try_main() {
@@ -46,10 +48,20 @@
}));
debug!("compsvc is starting as a rpc service.");
- let service = compsvc::new_binder()?.as_binder();
- let server = RpcServer::new_vsock(service, COMPOS_VSOCK_PORT)?;
- // SAFETY: Invokes a method from the bindgen library `vm_payload_bindgen`.
- unsafe { AVmPayload_notifyPayloadReady() };
- server.join();
+ let param = ptr::null_mut();
+ let mut service = compsvc::new_binder()?.as_binder();
+ unsafe {
+ // SAFETY: We hold a strong pointer, so the raw pointer remains valid. The bindgen AIBinder
+ // is the same type as sys::AIBinder.
+ let service = service.as_native_mut() as *mut AIBinder;
+ // SAFETY: It is safe for on_ready to be invoked at any time, with any parameter.
+ AVmPayload_runVsockRpcServer(service, COMPOS_VSOCK_PORT, Some(on_ready), param);
+ }
Ok(())
}
+
+extern "C" fn on_ready(_param: *mut c_void) {
+ // SAFETY: Invokes a method from the bindgen library `vm_payload_bindgen` which is safe to
+ // call at any time.
+ unsafe { AVmPayload_notifyPayloadReady() };
+}
diff --git a/tests/benchmark/src/native/benchmarkbinary.cpp b/tests/benchmark/src/native/benchmarkbinary.cpp
index 66b41a1..70ec7db 100644
--- a/tests/benchmark/src/native/benchmarkbinary.cpp
+++ b/tests/benchmark/src/native/benchmarkbinary.cpp
@@ -161,10 +161,8 @@
Result<void> run_io_benchmark_tests() {
auto test_service = ndk::SharedRefBase::make<IOBenchmarkService>();
auto callback = []([[maybe_unused]] void* param) { AVmPayload_notifyPayloadReady(); };
- if (!AVmPayload_runVsockRpcServer(test_service->asBinder().get(), test_service->SERVICE_PORT,
- callback, nullptr)) {
- return Error() << "RPC Server failed to run";
- }
+ AVmPayload_runVsockRpcServer(test_service->asBinder().get(), test_service->SERVICE_PORT,
+ callback, nullptr);
return {};
}
} // Anonymous namespace
diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp
index 5c217ff..c0a8c0e 100644
--- a/tests/testapk/src/native/testbinary.cpp
+++ b/tests/testapk/src/native/testbinary.cpp
@@ -119,10 +119,8 @@
auto testService = ndk::SharedRefBase::make<TestService>();
auto callback = []([[maybe_unused]] void* param) { AVmPayload_notifyPayloadReady(); };
- if (!AVmPayload_runVsockRpcServer(testService->asBinder().get(), testService->SERVICE_PORT,
- callback, nullptr)) {
- return Error() << "RPC Server failed to run";
- }
+ AVmPayload_runVsockRpcServer(testService->asBinder().get(), testService->SERVICE_PORT, callback,
+ nullptr);
return {};
}
diff --git a/vm_payload/include/vm_payload.h b/vm_payload/include/vm_payload.h
index 0ad4c64..7c224f6 100644
--- a/vm_payload/include/vm_payload.h
+++ b/vm_payload/include/vm_payload.h
@@ -18,6 +18,7 @@
#include <stdbool.h>
#include <stddef.h>
+#include <stdnoreturn.h>
#include <sys/cdefs.h>
#include "vm_main.h"
@@ -46,18 +47,17 @@
* called to allow appropriate action to be taken - e.g. to notify clients that they may now
* attempt to connect with `AVmPayload_notifyPayloadReady`.
*
- * The current thread is joined to the binder thread pool to handle incoming messages.
+ * Note that this function does not return. The calling thread joins the binder
+ * thread pool to handle incoming messages.
*
* \param service the service to bind to the given port.
* \param port vsock port.
* \param on_ready the callback to execute once the server is ready for connections. The callback
* will be called at most once.
* \param param param for the `on_ready` callback.
- *
- * \return true if the server has shutdown normally, false if it failed in some way.
*/
-bool AVmPayload_runVsockRpcServer(AIBinder *service, unsigned int port,
- void (*on_ready)(void *param), void *param);
+noreturn void AVmPayload_runVsockRpcServer(AIBinder *service, unsigned int port,
+ void (*on_ready)(void *param), void *param);
/**
* Get a secret that is uniquely bound to this VM instance. The secrets are
diff --git a/vm_payload/src/api.rs b/vm_payload/src/api.rs
index febc2be..a79c0bb 100644
--- a/vm_payload/src/api.rs
+++ b/vm_payload/src/api.rs
@@ -14,13 +14,17 @@
//! This module handles the interaction with virtual machine payload service.
+// We're implementing unsafe functions, but we still want warnings on unsafe usage within them.
+#![warn(unsafe_op_in_unsafe_fn)]
+
use android_system_virtualization_payload::aidl::android::system::virtualization::payload::IVmPayloadService::{
IVmPayloadService, VM_PAYLOAD_SERVICE_SOCKET_NAME, VM_APK_CONTENTS_PATH};
-use anyhow::{ensure, Context, Result};
+use anyhow::{ensure, bail, Context, Result};
use binder::{Strong, unstable_api::{AIBinder, new_spibinder}};
use lazy_static::lazy_static;
use log::{error, info, Level};
use rpcbinder::{get_unix_domain_rpc_interface, RpcServer};
+use std::convert::Infallible;
use std::ffi::CString;
use std::fmt::Debug;
use std::os::raw::{c_char, c_void};
@@ -96,44 +100,55 @@
/// called to allow appropriate action to be taken - e.g. to notify clients that they may now
/// attempt to connect.
///
-/// The current thread is joined to the binder thread pool to handle incoming messages.
+/// The current thread joins the binder thread pool to handle incoming messages.
+/// This function never returns.
///
-/// Returns true if the server has shutdown normally, false if it failed in some way.
+/// Panics on error (including unexpected server exit).
///
/// # Safety
///
-/// The `on_ready` callback is only called inside `run_vsock_rpc_server`, within the lifetime of
-/// `ReadyNotifier` (the last parameter of `run_vsock_rpc_server`). If `on_ready` is called with
-/// wrong param, the callback execution could go wrong.
+/// If present, the `on_ready` callback must be a valid function pointer, which will be called at
+/// most once, while this function is executing, with the `param` parameter.
#[no_mangle]
pub unsafe extern "C" fn AVmPayload_runVsockRpcServer(
service: *mut AIBinder,
port: u32,
on_ready: Option<unsafe extern "C" fn(param: *mut c_void)>,
param: *mut c_void,
-) -> bool {
+) -> Infallible {
initialize_logging();
+ // SAFETY: try_run_vsock_server has the same requirements as this function
+ unwrap_or_abort(unsafe { try_run_vsock_server(service, port, on_ready, param) })
+}
+
+/// # Safety: Same as `AVmPayload_runVsockRpcServer`.
+unsafe fn try_run_vsock_server(
+ service: *mut AIBinder,
+ port: u32,
+ on_ready: Option<unsafe extern "C" fn(param: *mut c_void)>,
+ param: *mut c_void,
+) -> Result<Infallible> {
// SAFETY: AIBinder returned has correct reference count, and the ownership can
// safely be taken by new_spibinder.
- let service = new_spibinder(service);
+ let service = unsafe { new_spibinder(service) };
if let Some(service) = service {
match RpcServer::new_vsock(service, port) {
Ok(server) => {
if let Some(on_ready) = on_ready {
- on_ready(param);
+ // SAFETY: We're calling the callback with the parameter specified within the
+ // allowed lifetime.
+ unsafe { on_ready(param) };
}
server.join();
- true
+ bail!("RpcServer unexpectedly terminated");
}
Err(err) => {
- error!("Failed to start RpcServer: {:?}", err);
- false
+ bail!("Failed to start RpcServer: {:?}", err);
}
}
} else {
- error!("Failed to convert the given service from AIBinder to SpIBinder.");
- false
+ bail!("Failed to convert the given service from AIBinder to SpIBinder.");
}
}
@@ -157,9 +172,15 @@
) {
initialize_logging();
- let identifier = std::slice::from_raw_parts(identifier, identifier_size);
+ // SAFETY: See the requirements on `identifier` above.
+ let identifier = unsafe { std::slice::from_raw_parts(identifier, identifier_size) };
let vm_secret = unwrap_or_abort(try_get_vm_instance_secret(identifier, size));
- ptr::copy_nonoverlapping(vm_secret.as_ptr(), secret, size);
+
+ // SAFETY: See the requirements on `secret` above; `vm_secret` is known to have length `size`,
+ // and cannot overlap `secret` because we just allocated it.
+ unsafe {
+ ptr::copy_nonoverlapping(vm_secret.as_ptr(), secret, size);
+ }
}
fn try_get_vm_instance_secret(identifier: &[u8], size: usize) -> Result<Vec<u8>> {
@@ -190,7 +211,9 @@
initialize_logging();
let chain = unwrap_or_abort(try_get_dice_attestation_chain());
- ptr::copy_nonoverlapping(chain.as_ptr(), data, std::cmp::min(chain.len(), size));
+ // SAFETY: See the requirements on `data` above. The number of bytes copied doesn't exceed
+ // the length of either buffer, and `chain` cannot overlap `data` because we just allocated it.
+ unsafe { ptr::copy_nonoverlapping(chain.as_ptr(), data, std::cmp::min(chain.len(), size)) };
chain.len()
}
@@ -213,7 +236,9 @@
initialize_logging();
let cdi = unwrap_or_abort(try_get_dice_attestation_cdi());
- ptr::copy_nonoverlapping(cdi.as_ptr(), data, std::cmp::min(cdi.len(), size));
+ // SAFETY: See the requirements on `data` above. The number of bytes copied doesn't exceed
+ // the length of either buffer, and `cdi` cannot overlap `data` because we just allocated it.
+ unsafe { ptr::copy_nonoverlapping(cdi.as_ptr(), data, std::cmp::min(cdi.len(), size)) };
cdi.len()
}