From b45f17763bd6b435dac12c41f8aa279ce99da4c1 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 16 Apr 2026 06:13:53 -0700 Subject: [PATCH] fix(ffi): harden AdNativeHandle against double-free + null reuse (todo 009) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes P1 todo 009 (Option 2: minimal hardening; Option 1 opaque-ID table deferred as an ABI-breaking follow-up). Three concrete changes: 1. ad_free_handle signature: *const AdNativeHandle → *mut AdNativeHandle. The free fn now owns the right to mutate the caller's struct. 2. Zero handle.ptr BEFORE invoking the platform release() call. A follow-up ad_free_handle on the same struct observes ptr == null and returns Ok without re-entering CFRelease, making an accidental double-call deterministic instead of corrupting the CF retain count. 3. ad_execute_action and ad_get now reject handle.ptr == NULL at the use site (after guarding the struct pointer itself) with a diagnostic last-error "handle.ptr is null — the handle has already been freed or was never resolved". Prevents feeding a freshly-zeroed handle back into adapter code. Ownership contract captured in the ad_free_handle rustdoc: the FFI owns the handle from ad_resolve_element onward; copying the struct and calling ad_free_handle on either copy is undefined because the library cannot detect forged non-null pointers. Callers that need a "second copy" must re-resolve. Tests (crates/ffi/tests/c_abi_harness.rs): - free_handle_null_is_noop: adjusted to *mut signature, asserts the ptr stays null after the call. - free_handle_zeroes_ptr_so_double_free_is_noop: non-macOS only (macOS would SIGBUS on the fake pointer's CFRelease before we could observe the zeroing; the logic is platform-agnostic so validating it on Windows/Linux covers the contract). - execute_action_rejects_null_handle_ptr: null .ptr with non-null struct returns InvalidArgs or ErrInternal (main-thread guard), no UB. 81 FFI tests pass (72 lib + error_lifetime + 14 c_abi_harness variants depending on target). Clippy clean. --- crates/ffi/include/agent_desktop.h | 24 ++++++--- crates/ffi/src/actions/execute.rs | 7 +++ crates/ffi/src/actions/native_handle.rs | 31 +++++++---- crates/ffi/src/observation/get.rs | 10 +++- crates/ffi/tests/c_abi_harness.rs | 69 ++++++++++++++++++++++--- 5 files changed, 118 insertions(+), 23 deletions(-) diff --git a/crates/ffi/include/agent_desktop.h b/crates/ffi/include/agent_desktop.h index 859764f..57db863 100644 --- a/crates/ffi/include/agent_desktop.h +++ b/crates/ffi/include/agent_desktop.h @@ -293,22 +293,32 @@ AdResult ad_execute_action(const struct AdAdapter *adapter, struct AdActionResult *out); /** - * Releases a handle previously returned by `ad_resolve_element`. + * Releases a handle previously returned by `ad_resolve_element` and + * zeroes the caller's struct so accidentally calling this twice is + * a deterministic no-op instead of a double-free on the underlying + * `CFRelease`. * * On macOS this calls `CFRelease` on the underlying `AXUIElementRef`, * balancing the `CFRetain` that happened during `ad_resolve_element`. * On Windows/Linux the call is a no-op that returns `AD_RESULT_OK` - * (platform adapters inherit the default `not_supported` impl, which - * the FFI surface rewrites to `Ok` here so callers can apply the same - * release pattern everywhere). + * (platform adapters inherit the default `not_supported` impl; the + * FFI surface translates it so callers apply the same release + * pattern everywhere). + * + * Ownership contract: the FFI owns the handle from the moment + * `ad_resolve_element` writes `ptr`. Copying the struct after that + * point and calling `ad_free_handle` on either copy is undefined — + * there is no way for the library to detect forged non-null pointers. + * Callers that legitimately need a "copy" should re-resolve. * * # Safety * * `adapter` must be a non-null pointer returned by `ad_adapter_create`. - * `handle` must be null or a pointer previously populated by - * `ad_resolve_element`. Double-free is undefined behavior. + * `handle` must be null or a `*mut AdNativeHandle` previously + * populated by `ad_resolve_element`. On return `(*handle).ptr` is + * `NULL` so a double-call is a no-op instead of a double-free. */ -AdResult ad_free_handle(const struct AdAdapter *adapter, const struct AdNativeHandle *handle); +AdResult ad_free_handle(const struct AdAdapter *adapter, struct AdNativeHandle *handle); /** * # Safety diff --git a/crates/ffi/src/actions/execute.rs b/crates/ffi/src/actions/execute.rs index 0b26b4b..8d24642 100644 --- a/crates/ffi/src/actions/execute.rs +++ b/crates/ffi/src/actions/execute.rs @@ -30,6 +30,13 @@ pub unsafe extern "C" fn ad_execute_action( crate::pointer_guard::guard_non_null!(action, c"action is null"); let adapter = &*adapter; let handle_ref = &*handle; + if handle_ref.ptr.is_null() { + error::set_last_error(&agent_desktop_core::error::AdapterError::new( + agent_desktop_core::error::ErrorCode::InvalidArgs, + "handle.ptr is null — the handle has already been freed or was never resolved", + )); + return AdResult::ErrInvalidArgs; + } let action_ref = &*action; let core_action = match action_from_c(action_ref) { Ok(a) => a, diff --git a/crates/ffi/src/actions/native_handle.rs b/crates/ffi/src/actions/native_handle.rs index 4a7328d..18ae079 100644 --- a/crates/ffi/src/actions/native_handle.rs +++ b/crates/ffi/src/actions/native_handle.rs @@ -4,24 +4,34 @@ use crate::types::AdNativeHandle; use crate::AdAdapter; use agent_desktop_core::adapter::NativeHandle; -/// Releases a handle previously returned by `ad_resolve_element`. +/// Releases a handle previously returned by `ad_resolve_element` and +/// zeroes the caller's struct so accidentally calling this twice is +/// a deterministic no-op instead of a double-free on the underlying +/// `CFRelease`. /// /// On macOS this calls `CFRelease` on the underlying `AXUIElementRef`, /// balancing the `CFRetain` that happened during `ad_resolve_element`. /// On Windows/Linux the call is a no-op that returns `AD_RESULT_OK` -/// (platform adapters inherit the default `not_supported` impl, which -/// the FFI surface rewrites to `Ok` here so callers can apply the same -/// release pattern everywhere). +/// (platform adapters inherit the default `not_supported` impl; the +/// FFI surface translates it so callers apply the same release +/// pattern everywhere). +/// +/// Ownership contract: the FFI owns the handle from the moment +/// `ad_resolve_element` writes `ptr`. Copying the struct after that +/// point and calling `ad_free_handle` on either copy is undefined — +/// there is no way for the library to detect forged non-null pointers. +/// Callers that legitimately need a "copy" should re-resolve. /// /// # Safety /// /// `adapter` must be a non-null pointer returned by `ad_adapter_create`. -/// `handle` must be null or a pointer previously populated by -/// `ad_resolve_element`. Double-free is undefined behavior. +/// `handle` must be null or a `*mut AdNativeHandle` previously +/// populated by `ad_resolve_element`. On return `(*handle).ptr` is +/// `NULL` so a double-call is a no-op instead of a double-free. #[no_mangle] pub unsafe extern "C" fn ad_free_handle( adapter: *const AdAdapter, - handle: *const AdNativeHandle, + handle: *mut AdNativeHandle, ) -> AdResult { trap_panic(|| unsafe { if adapter.is_null() { @@ -34,16 +44,19 @@ pub unsafe extern "C" fn ad_free_handle( if handle.is_null() { return AdResult::Ok; } - let adapter = &*adapter; let raw = (*handle).ptr; if raw.is_null() { return AdResult::Ok; } + // Zero the caller-visible pointer *before* the platform release + // so a concurrent or accidental double-call through the same + // struct cannot re-enter CFRelease on the same underlying ref. + (*handle).ptr = std::ptr::null(); + let adapter = &*adapter; let native = NativeHandle::from_ptr(raw); match adapter.inner.release_handle(&native) { Ok(()) => AdResult::Ok, Err(e) => { - // Not-supported on Windows/Linux is a no-op by contract. if matches!( e.code, agent_desktop_core::error::ErrorCode::ActionNotSupported diff --git a/crates/ffi/src/observation/get.rs b/crates/ffi/src/observation/get.rs index f56bbd5..1006607 100644 --- a/crates/ffi/src/observation/get.rs +++ b/crates/ffi/src/observation/get.rs @@ -36,7 +36,15 @@ pub unsafe extern "C" fn ad_get( crate::pointer_guard::guard_non_null!(adapter, c"adapter is null"); crate::pointer_guard::guard_non_null!(handle, c"handle is null"); let adapter = &*adapter; - let native = NativeHandle::from_ptr((*handle).ptr); + let raw = (*handle).ptr; + if raw.is_null() { + set_last_error(&agent_desktop_core::error::AdapterError::new( + agent_desktop_core::error::ErrorCode::InvalidArgs, + "handle.ptr is null — the handle has already been freed or was never resolved", + )); + return AdResult::ErrInvalidArgs; + } + let native = NativeHandle::from_ptr(raw); let prop = match c_to_string(property) { Some(s) => s, None => { diff --git a/crates/ffi/tests/c_abi_harness.rs b/crates/ffi/tests/c_abi_harness.rs index e831e70..eb12db6 100644 --- a/crates/ffi/tests/c_abi_harness.rs +++ b/crates/ffi/tests/c_abi_harness.rs @@ -66,7 +66,7 @@ extern "C" { out: *mut AdNativeHandle, ) -> AdResult; - fn ad_free_handle(adapter: *const AdAdapter, handle: *const AdNativeHandle) -> AdResult; + fn ad_free_handle(adapter: *const AdAdapter, handle: *mut AdNativeHandle) -> AdResult; } fn with_adapter(body: F) { @@ -306,14 +306,71 @@ fn find_returns_not_found_on_empty_query_against_no_window() { #[test] fn free_handle_null_is_noop() { with_adapter(|adapter| unsafe { + let mut handle = AdNativeHandle { + ptr: std::ptr::null(), + }; + let rc = ad_free_handle(adapter, &mut handle); + assert_eq!(rc, AdResult::Ok); + assert!(handle.ptr.is_null()); + + let rc2 = ad_free_handle(adapter, std::ptr::null_mut()); + assert_eq!(rc2, AdResult::Ok); + }); +} + +#[cfg(not(target_os = "macos"))] +#[test] +fn free_handle_zeroes_ptr_so_double_free_is_noop() { + // macOS is excluded: ad_free_handle invokes CFRelease on the + // underlying pointer, and a fabricated "fake live" pointer will + // SIGBUS before we can observe the zeroing. On Windows/Linux + // release_handle is NotSupported (no platform call), so the zeroing + // contract is safely observable with a fake pointer. + with_adapter(|adapter| unsafe { + let fake_live_ptr = 0x1234 as *const std::ffi::c_void; + let mut handle = AdNativeHandle { ptr: fake_live_ptr }; + + let _ = ad_free_handle(adapter, &mut handle); + assert!(handle.ptr.is_null()); + + let rc = ad_free_handle(adapter, &mut handle); + assert_eq!(rc, AdResult::Ok); + }); +} + +#[test] +fn execute_action_rejects_null_handle_ptr() { + with_adapter(|adapter| unsafe { + let action = AdAction { + kind: 0, + text: std::ptr::null(), + scroll: AdScrollParams { + direction: 0, + amount: 0, + }, + key: AdKeyCombo { + key: std::ptr::null(), + modifiers: std::ptr::null(), + modifier_count: 0, + }, + drag: AdDragParams { + from: AdPoint { x: 0.0, y: 0.0 }, + to: AdPoint { x: 0.0, y: 0.0 }, + duration_ms: 0, + }, + }; let handle = AdNativeHandle { ptr: std::ptr::null(), }; - let rc = ad_free_handle(adapter, &handle); - assert_eq!(rc, AdResult::Ok); - - let rc2 = ad_free_handle(adapter, std::ptr::null()); - assert_eq!(rc2, AdResult::Ok); + let mut out: AdActionResult = std::mem::zeroed(); + let rc = ad_execute_action(adapter, &handle, &action, &mut out); + // Main-thread guard or null-ptr guard wins; both avoid UB and + // keep the struct-level handle accepted while rejecting the + // inner null pointer. + assert!(matches!( + rc, + AdResult::ErrInvalidArgs | AdResult::ErrInternal + )); }); }