From 4da282ecebcf592c62656d7403aa7c42703ac0fd Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 16 Apr 2026 05:26:08 -0700 Subject: [PATCH] fix(ffi): validate pointers + null out-params on error (todo 003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/ffi/src/actions/execute.rs | 4 ++ crates/ffi/src/actions/resolve.rs | 3 ++ crates/ffi/src/adapter.rs | 1 + crates/ffi/src/apps/close.rs | 1 + crates/ffi/src/apps/launch.rs | 4 +- crates/ffi/src/apps/list.rs | 2 + crates/ffi/src/input/clipboard.rs | 5 ++ crates/ffi/src/input/drag.rs | 2 + crates/ffi/src/input/mouse.rs | 2 + crates/ffi/src/lib.rs | 1 + crates/ffi/src/notifications/action.rs | 2 + crates/ffi/src/notifications/dismiss.rs | 1 + crates/ffi/src/notifications/dismiss_all.rs | 3 ++ crates/ffi/src/notifications/list.rs | 2 + crates/ffi/src/observation/find.rs | 4 ++ crates/ffi/src/observation/get.rs | 3 ++ crates/ffi/src/observation/is.rs | 4 ++ crates/ffi/src/pointer_guard.rs | 52 +++++++++++++++++++++ crates/ffi/src/screenshot/capture.rs | 3 ++ crates/ffi/src/surfaces/list.rs | 2 + crates/ffi/src/tree/get.rs | 4 ++ crates/ffi/src/windows/focus.rs | 2 + crates/ffi/src/windows/list.rs | 2 + crates/ffi/src/windows/op.rs | 2 + crates/ffi/tests/c_abi_harness.rs | 32 +++++++++++++ 25 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 crates/ffi/src/pointer_guard.rs diff --git a/crates/ffi/src/actions/execute.rs b/crates/ffi/src/actions/execute.rs index 4a30cf5..eb87ddc 100644 --- a/crates/ffi/src/actions/execute.rs +++ b/crates/ffi/src/actions/execute.rs @@ -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; diff --git a/crates/ffi/src/actions/resolve.rs b/crates/ffi/src/actions/resolve.rs index f6ea9f1..a86dafb 100644 --- a/crates/ffi/src/actions/resolve.rs +++ b/crates/ffi/src/actions/resolve.rs @@ -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; diff --git a/crates/ffi/src/adapter.rs b/crates/ffi/src/adapter.rs index 41e1c83..46af26d 100644 --- a/crates/ffi/src/adapter.rs +++ b/crates/ffi/src/adapter.rs @@ -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, diff --git a/crates/ffi/src/apps/close.rs b/crates/ffi/src/apps/close.rs index 7ced7ff..4b600f3 100644 --- a/crates/ffi/src/apps/close.rs +++ b/crates/ffi/src/apps/close.rs @@ -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, diff --git a/crates/ffi/src/apps/launch.rs b/crates/ffi/src/apps/launch.rs index 0b2433c..687db9e 100644 --- a/crates/ffi/src/apps/launch.rs +++ b/crates/ffi/src/apps/launch.rs @@ -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); diff --git a/crates/ffi/src/apps/list.rs b/crates/ffi/src/apps/list.rs index 54a08ce..7a89161 100644 --- a/crates/ffi/src/apps/list.rs +++ b/crates/ffi/src/apps/list.rs @@ -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() { diff --git a/crates/ffi/src/input/clipboard.rs b/crates/ffi/src/input/clipboard.rs index e1bc91c..d91ebf9 100644 --- a/crates/ffi/src/input/clipboard.rs +++ b/crates/ffi/src/input/clipboard.rs @@ -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, diff --git a/crates/ffi/src/input/drag.rs b/crates/ffi/src/input/drag.rs index 63b5180..ae808c6 100644 --- a/crates/ffi/src/input/drag.rs +++ b/crates/ffi/src/input/drag.rs @@ -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 { diff --git a/crates/ffi/src/input/mouse.rs b/crates/ffi/src/input/mouse.rs index b2b7840..888ecfe 100644 --- a/crates/ffi/src/input/mouse.rs +++ b/crates/ffi/src/input/mouse.rs @@ -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) { diff --git a/crates/ffi/src/lib.rs b/crates/ffi/src/lib.rs index 7a0436e..c150e3f 100644 --- a/crates/ffi/src/lib.rs +++ b/crates/ffi/src/lib.rs @@ -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; diff --git a/crates/ffi/src/notifications/action.rs b/crates/ffi/src/notifications/action.rs index faa9036..97e73e5 100644 --- a/crates/ffi/src/notifications/action.rs +++ b/crates/ffi/src/notifications/action.rs @@ -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) { diff --git a/crates/ffi/src/notifications/dismiss.rs b/crates/ffi/src/notifications/dismiss.rs index e9c25ae..e695211 100644 --- a/crates/ffi/src/notifications/dismiss.rs +++ b/crates/ffi/src/notifications/dismiss.rs @@ -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(); diff --git a/crates/ffi/src/notifications/dismiss_all.rs b/crates/ffi/src/notifications/dismiss_all.rs index 8f6ecd9..9ae7f34 100644 --- a/crates/ffi/src/notifications/dismiss_all.rs +++ b/crates/ffi/src/notifications/dismiss_all.rs @@ -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; diff --git a/crates/ffi/src/notifications/list.rs b/crates/ffi/src/notifications/list.rs index 9e1f61c..2e0d674 100644 --- a/crates/ffi/src/notifications/list.rs +++ b/crates/ffi/src/notifications/list.rs @@ -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); diff --git a/crates/ffi/src/observation/find.rs b/crates/ffi/src/observation/find.rs index ae129ca..e413fc7 100644 --- a/crates/ffi/src/observation/find.rs +++ b/crates/ffi/src/observation/find.rs @@ -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) { diff --git a/crates/ffi/src/observation/get.rs b/crates/ffi/src/observation/get.rs index e91bedb..8f1320c 100644 --- a/crates/ffi/src/observation/get.rs +++ b/crates/ffi/src/observation/get.rs @@ -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); diff --git a/crates/ffi/src/observation/is.rs b/crates/ffi/src/observation/is.rs index 673b7f5..bdb5e33 100644 --- a/crates/ffi/src/observation/is.rs +++ b/crates/ffi/src/observation/is.rs @@ -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) { diff --git a/crates/ffi/src/pointer_guard.rs b/crates/ffi/src/pointer_guard.rs new file mode 100644 index 0000000..faae254 --- /dev/null +++ b/crates/ffi/src/pointer_guard.rs @@ -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)); + } +} diff --git a/crates/ffi/src/screenshot/capture.rs b/crates/ffi/src/screenshot/capture.rs index 8726eb9..02b8b13 100644 --- a/crates/ffi/src/screenshot/capture.rs +++ b/crates/ffi/src/screenshot/capture.rs @@ -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; diff --git a/crates/ffi/src/surfaces/list.rs b/crates/ffi/src/surfaces/list.rs index ba6d2de..0e59673 100644 --- a/crates/ffi/src/surfaces/list.rs +++ b/crates/ffi/src/surfaces/list.rs @@ -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) { diff --git a/crates/ffi/src/tree/get.rs b/crates/ffi/src/tree/get.rs index 6f96bde..19df83c 100644 --- a/crates/ffi/src/tree/get.rs +++ b/crates/ffi/src/tree/get.rs @@ -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; diff --git a/crates/ffi/src/windows/focus.rs b/crates/ffi/src/windows/focus.rs index 1d9e39e..482955e 100644 --- a/crates/ffi/src/windows/focus.rs +++ b/crates/ffi/src/windows/focus.rs @@ -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, diff --git a/crates/ffi/src/windows/list.rs b/crates/ffi/src/windows/list.rs index 7aea62f..a65aa0c 100644 --- a/crates/ffi/src/windows/list.rs +++ b/crates/ffi/src/windows/list.rs @@ -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 { diff --git a/crates/ffi/src/windows/op.rs b/crates/ffi/src/windows/op.rs index f0f1062..10d08cd 100644 --- a/crates/ffi/src/windows/op.rs +++ b/crates/ffi/src/windows/op.rs @@ -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, diff --git a/crates/ffi/tests/c_abi_harness.rs b/crates/ffi/tests/c_abi_harness.rs index 3bef4e6..246b5b3 100644 --- a/crates/ffi/tests/c_abi_harness.rs +++ b/crates/ffi/tests/c_abi_harness.rs @@ -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 {