mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-16 03:56:16 +00:00
fix(ffi): validate pointers + null out-params on error (todo 003)
Closes P1 todo 003. Prior code dereferenced raw inbound pointers before validating them; null or stale adapter/input/out pointers could segfault the host before the FFI had a chance to return a structured error. trap_panic can only catch Rust panics — not raw-pointer UB. Add crates/ffi/src/pointer_guard.rs with a guard_non_null! macro that short-circuits the enclosing AdResult-returning fn with AD_RESULT_ERR_INVALID_ARGS and populates last-error via the 'static errno slot. Zero allocation on the error path. Apply the macro to every extern fn that dereferences inputs or writes out-params, guarding pointers before the first use: - adapter: ad_check_permissions - apps: ad_launch_app, ad_close_app, ad_list_apps - windows: ad_list_windows, ad_focus_window, ad_window_op - input: ad_mouse_event, ad_drag, ad_get_clipboard, ad_set_clipboard, ad_clear_clipboard - screenshot: ad_screenshot - surfaces: ad_list_surfaces - tree: ad_get_tree - actions: ad_resolve_element, ad_execute_action - observation: ad_find, ad_get, ad_is - notifications: ad_list_notifications, ad_dismiss_notification, ad_dismiss_all_notifications, ad_notification_action Fix ad_get_clipboard to null-initialize *out after validating the out-pointer and before any fallible adapter call — aligns the implementation with the documented null-on-error contract. New c_abi_harness regression tests: - null_adapter_rejected_without_ub: passes null adapter to ad_list_apps and ad_check_permissions; both reject without a segfault. - null_out_param_rejected_before_write: passes null *out to ad_list_apps; rejects before the first *out = ... write. Both accept ErrInvalidArgs OR ErrInternal (worker-thread cargo tests trip the macOS main-thread guard first for guarded fns; ad_check_permissions has no main-thread guard so remains deterministic). 67 FFI tests pass, clippy clean.
This commit is contained in:
parent
659457b52f
commit
4da282eceb
25 changed files with 142 additions and 1 deletions
|
|
@ -23,6 +23,10 @@ pub unsafe extern "C" fn ad_execute_action(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(handle, c"handle is null");
|
||||
crate::pointer_guard::guard_non_null!(action, c"action is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = std::mem::zeroed();
|
||||
let adapter = &*adapter;
|
||||
let handle_ref = &*handle;
|
||||
|
|
|
|||
|
|
@ -20,6 +20,9 @@ pub unsafe extern "C" fn ad_resolve_element(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(entry, c"entry is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
(*out).ptr = std::ptr::null();
|
||||
let adapter = &*adapter;
|
||||
let entry = &*entry;
|
||||
|
|
|
|||
|
|
@ -63,6 +63,7 @@ pub unsafe extern "C" fn ad_adapter_destroy(adapter: *mut AdAdapter) {
|
|||
#[no_mangle]
|
||||
pub unsafe extern "C" fn ad_check_permissions(adapter: *const AdAdapter) -> AdResult {
|
||||
trap_panic(|| {
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
let adapter = unsafe { &*adapter };
|
||||
match adapter.inner.check_permissions() {
|
||||
agent_desktop_core::adapter::PermissionStatus::Granted => AdResult::Ok,
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ pub unsafe extern "C" fn ad_close_app(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
let adapter = &*adapter;
|
||||
let id_str = match c_to_string(id) {
|
||||
Some(s) => s,
|
||||
|
|
|
|||
|
|
@ -30,8 +30,9 @@ pub unsafe extern "C" fn ad_launch_app(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = std::mem::zeroed();
|
||||
let adapter = &*adapter;
|
||||
let id_str = match c_to_string(id) {
|
||||
Some(s) => s,
|
||||
None => {
|
||||
|
|
@ -43,6 +44,7 @@ pub unsafe extern "C" fn ad_launch_app(
|
|||
}
|
||||
};
|
||||
|
||||
let adapter = &*adapter;
|
||||
match adapter.inner.launch_app(&id_str, timeout_ms) {
|
||||
Ok(win) => {
|
||||
*out = window_info_to_c(&win);
|
||||
|
|
|
|||
|
|
@ -19,6 +19,8 @@ pub unsafe extern "C" fn ad_list_apps(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
match adapter.inner.list_apps() {
|
||||
|
|
|
|||
|
|
@ -20,6 +20,9 @@ pub unsafe extern "C" fn ad_get_clipboard(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = std::ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
match adapter.inner.get_clipboard() {
|
||||
Ok(text) => {
|
||||
|
|
@ -49,6 +52,7 @@ pub unsafe extern "C" fn ad_set_clipboard(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
let adapter = &*adapter;
|
||||
let text = match c_to_string(text) {
|
||||
Some(s) => s,
|
||||
|
|
@ -80,6 +84,7 @@ pub unsafe extern "C" fn ad_clear_clipboard(adapter: *const AdAdapter) -> AdResu
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
let adapter = &*adapter;
|
||||
match adapter.inner.clear_clipboard() {
|
||||
Ok(()) => AdResult::Ok,
|
||||
|
|
|
|||
|
|
@ -20,6 +20,8 @@ pub unsafe extern "C" fn ad_drag(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(params, c"params is null");
|
||||
let adapter = &*adapter;
|
||||
let p = &*params;
|
||||
let core_params = CoreDragParams {
|
||||
|
|
|
|||
|
|
@ -31,6 +31,8 @@ pub unsafe extern "C" fn ad_mouse_event(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(event, c"event is null");
|
||||
let adapter = &*adapter;
|
||||
let ev = &*event;
|
||||
let validated_button = match AdMouseButton::from_c(ev.button) {
|
||||
|
|
|
|||
|
|
@ -43,6 +43,7 @@ pub(crate) mod input;
|
|||
pub(crate) mod main_thread;
|
||||
pub(crate) mod notifications;
|
||||
pub(crate) mod observation;
|
||||
pub(crate) mod pointer_guard;
|
||||
pub(crate) mod screenshot;
|
||||
pub(crate) mod surfaces;
|
||||
pub(crate) mod tree;
|
||||
|
|
|
|||
|
|
@ -25,6 +25,8 @@ pub unsafe extern "C" fn ad_notification_action(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = std::mem::zeroed();
|
||||
let adapter = &*adapter;
|
||||
let action = match c_to_string(action_name) {
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ pub unsafe extern "C" fn ad_dismiss_notification(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
let adapter = &*adapter;
|
||||
let filter = c_to_string(app_filter);
|
||||
let filter_ref = filter.as_deref();
|
||||
|
|
|
|||
|
|
@ -34,6 +34,9 @@ pub unsafe extern "C" fn ad_dismiss_all_notifications(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(dismissed_out, c"dismissed_out is null");
|
||||
crate::pointer_guard::guard_non_null!(failed_out, c"failed_out is null");
|
||||
*dismissed_out = ptr::null_mut();
|
||||
*failed_out = ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
|
|
|
|||
|
|
@ -27,6 +27,8 @@ pub unsafe extern "C" fn ad_list_notifications(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
let core_filter = filter_from_c(filter);
|
||||
|
|
|
|||
|
|
@ -33,6 +33,10 @@ pub unsafe extern "C" fn ad_find(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(win, c"win is null");
|
||||
crate::pointer_guard::guard_non_null!(query, c"query is null");
|
||||
crate::pointer_guard::guard_non_null!(out_handle, c"out_handle is null");
|
||||
(*out_handle).ptr = std::ptr::null();
|
||||
let adapter = &*adapter;
|
||||
let core_win = match crate::windows::ad_window_to_core(&*win) {
|
||||
|
|
|
|||
|
|
@ -31,6 +31,9 @@ pub unsafe extern "C" fn ad_get(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(handle, c"handle is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = std::ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
let native = NativeHandle::from_ptr((*handle).ptr);
|
||||
|
|
|
|||
|
|
@ -40,6 +40,10 @@ pub unsafe extern "C" fn ad_is(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(win, c"win is null");
|
||||
crate::pointer_guard::guard_non_null!(query, c"query is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = false;
|
||||
let adapter = &*adapter;
|
||||
let core_win = match crate::windows::ad_window_to_core(&*win) {
|
||||
|
|
|
|||
52
crates/ffi/src/pointer_guard.rs
Normal file
52
crates/ffi/src/pointer_guard.rs
Normal file
|
|
@ -0,0 +1,52 @@
|
|||
//! Shared pointer-validation helper for FFI entrypoints.
|
||||
//!
|
||||
//! The FFI contract promises that malformed foreign input (null
|
||||
//! pointers) turns into a structured `AD_RESULT_ERR_INVALID_ARGS`
|
||||
//! error, not a segfault. `trap_panic` can only catch Rust panics — it
|
||||
//! does not recover from raw-pointer UB. Every extern fn therefore
|
||||
//! validates its inputs before the first dereference using
|
||||
//! [`guard_non_null`].
|
||||
|
||||
/// Bail out of the enclosing `AdResult`-returning function with
|
||||
/// `AD_RESULT_ERR_INVALID_ARGS` when `$ptr` is null. The `'static`
|
||||
/// `$message` is surfaced via the errno-style last-error slot so C
|
||||
/// consumers see which pointer was rejected.
|
||||
macro_rules! guard_non_null {
|
||||
($ptr:expr, $message:expr) => {
|
||||
if ($ptr).is_null() {
|
||||
$crate::error::set_last_error_static($crate::error::AdResult::ErrInvalidArgs, $message);
|
||||
return $crate::error::AdResult::ErrInvalidArgs;
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
pub(crate) use guard_non_null;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::error::AdResult;
|
||||
|
||||
fn null_case() -> AdResult {
|
||||
let null_ptr: *const u8 = std::ptr::null();
|
||||
guard_non_null!(null_ptr, c"null_ptr");
|
||||
AdResult::Ok
|
||||
}
|
||||
|
||||
fn nonnull_case() -> AdResult {
|
||||
let value: u8 = 0;
|
||||
let ptr: *const u8 = &value;
|
||||
guard_non_null!(ptr, c"ptr");
|
||||
AdResult::Ok
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn null_pointer_short_circuits_with_invalid_args() {
|
||||
assert!(matches!(null_case(), AdResult::ErrInvalidArgs));
|
||||
assert_eq!(crate::error::last_error_code(), AdResult::ErrInvalidArgs);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_null_pointer_passes_through() {
|
||||
assert!(matches!(nonnull_case(), AdResult::Ok));
|
||||
}
|
||||
}
|
||||
|
|
@ -24,6 +24,9 @@ pub unsafe extern "C" fn ad_screenshot(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(target, c"target is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
let t = &*target;
|
||||
|
|
|
|||
|
|
@ -19,6 +19,8 @@ pub unsafe extern "C" fn ad_list_surfaces(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
match adapter.inner.list_surfaces(pid) {
|
||||
|
|
|
|||
|
|
@ -45,6 +45,10 @@ pub unsafe extern "C" fn ad_get_tree(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(win, c"win is null");
|
||||
crate::pointer_guard::guard_non_null!(opts, c"opts is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
unsafe {
|
||||
(*out).nodes = ptr::null_mut();
|
||||
(*out).count = 0;
|
||||
|
|
|
|||
|
|
@ -21,6 +21,8 @@ pub unsafe extern "C" fn ad_focus_window(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(win, c"win is null");
|
||||
let adapter = &*adapter;
|
||||
let core_win = match ad_window_to_core(&*win) {
|
||||
Ok(w) => w,
|
||||
|
|
|
|||
|
|
@ -23,6 +23,8 @@ pub unsafe extern "C" fn ad_list_windows(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(out, c"out is null");
|
||||
*out = ptr::null_mut();
|
||||
let adapter = &*adapter;
|
||||
let filter = WindowFilter {
|
||||
|
|
|
|||
|
|
@ -25,6 +25,8 @@ pub unsafe extern "C" fn ad_window_op(
|
|||
if let Err(rc) = crate::main_thread::require_main_thread() {
|
||||
return rc;
|
||||
}
|
||||
crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
|
||||
crate::pointer_guard::guard_non_null!(win, c"win is null");
|
||||
let adapter = &*adapter;
|
||||
let core_win = match ad_window_to_core(&*win) {
|
||||
Ok(w) => w,
|
||||
|
|
|
|||
|
|
@ -154,6 +154,38 @@ fn enum_fuzz_invalid_discriminant_rejected() {
|
|||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn null_adapter_rejected_without_ub() {
|
||||
unsafe {
|
||||
let mut list: *mut AdAppList = std::ptr::null_mut();
|
||||
let rc = ad_list_apps(std::ptr::null(), &mut list);
|
||||
// On cargo-test worker threads the macOS main-thread guard
|
||||
// fires first (ErrInternal); on the main thread the null-adapter
|
||||
// guard wins (ErrInvalidArgs). Either way: no dereference, no UB.
|
||||
assert!(matches!(
|
||||
rc,
|
||||
AdResult::ErrInvalidArgs | AdResult::ErrInternal
|
||||
));
|
||||
assert!(list.is_null(), "out-param must stay null on failure");
|
||||
|
||||
let rc2 = ad_check_permissions(std::ptr::null());
|
||||
// ad_check_permissions has no main-thread guard — null adapter
|
||||
// must hit the null-check and return InvalidArgs deterministically.
|
||||
assert_eq!(rc2, AdResult::ErrInvalidArgs);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn null_out_param_rejected_before_write() {
|
||||
with_adapter(|adapter| unsafe {
|
||||
let rc = ad_list_apps(adapter, std::ptr::null_mut());
|
||||
assert!(matches!(
|
||||
rc,
|
||||
AdResult::ErrInvalidArgs | AdResult::ErrInternal
|
||||
));
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn null_tolerance_on_list_accessors_and_free() {
|
||||
unsafe {
|
||||
|
|
|
|||
Loading…
Reference in a new issue