From b7af53e920a75e697e520cba3c96ef227cbde442 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 16 Apr 2026 03:47:27 -0700 Subject: [PATCH] feat(ffi): errno-style last-error lifetime (Unit 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last-error pointers returned by ad_last_error_{code,message,suggestion, platform_detail} now survive across any number of subsequent successful FFI calls — only the next *failing* call rotates them. This matches the POSIX errno contract and closes R2 from the PR #22 review (use-after-free when caller cached the message pointer and made another successful call). Changes: - Remove error::clear_last_error() from every Ok branch across adapter, tree/get, actions/{resolve,execute}, apps/{list,launch,close}, windows/{list,focus,op}, input/{clipboard,mouse,drag}, screenshot/capture, surfaces/list. The slot now only rotates on a new set_last_error(). - clear_last_error is gated behind #[cfg(test)] since no production caller needs it after this change. - Add a crate-level rustdoc block on ad_last_error_code documenting the errno-style lifetime contract — cbindgen propagates this to agent_desktop.h so C consumers can read the rule from the header. - crates/ffi/tests/error_lifetime.rs: integration test reproducing the review's UAF scenario — fails on ErrInvalidArgs, caches the message pointer, makes 10 successful ad_check_permissions calls, asserts the cached pointer still resolves to the same string. Lib Cargo.toml now emits both cdylib and rlib so the integration test can link against the public crate without duplicating symbol bindings. 35 tests pass (34 lib + 1 integration). Clippy clean. --- crates/ffi/Cargo.toml | 2 +- crates/ffi/include/agent_desktop.h | 17 ++++++++++ crates/ffi/src/actions/execute.rs | 1 - crates/ffi/src/actions/resolve.rs | 1 - crates/ffi/src/adapter.rs | 5 +-- crates/ffi/src/apps/close.rs | 7 ++--- crates/ffi/src/apps/launch.rs | 3 +- crates/ffi/src/apps/list.rs | 3 +- crates/ffi/src/error.rs | 16 ++++++++++ crates/ffi/src/input/clipboard.rs | 11 ++----- crates/ffi/src/input/drag.rs | 5 +-- crates/ffi/src/input/mouse.rs | 5 +-- crates/ffi/src/screenshot/capture.rs | 3 +- crates/ffi/src/surfaces/list.rs | 3 +- crates/ffi/src/tree/get.rs | 3 +- crates/ffi/src/windows/focus.rs | 7 ++--- crates/ffi/src/windows/list.rs | 3 +- crates/ffi/src/windows/op.rs | 7 ++--- crates/ffi/tests/error_lifetime.rs | 46 ++++++++++++++++++++++++++++ 19 files changed, 97 insertions(+), 51 deletions(-) create mode 100644 crates/ffi/tests/error_lifetime.rs diff --git a/crates/ffi/Cargo.toml b/crates/ffi/Cargo.toml index 1ec3eb5..c6af817 100644 --- a/crates/ffi/Cargo.toml +++ b/crates/ffi/Cargo.toml @@ -6,7 +6,7 @@ license.workspace = true publish = false [lib] -crate-type = ["cdylib"] +crate-type = ["cdylib", "rlib"] [dependencies] agent-desktop-core.workspace = true diff --git a/crates/ffi/include/agent_desktop.h b/crates/ffi/include/agent_desktop.h index 8ec200b..7f7ef05 100644 --- a/crates/ffi/include/agent_desktop.h +++ b/crates/ffi/include/agent_desktop.h @@ -321,6 +321,23 @@ AdResult ad_list_apps(const struct AdAdapter *adapter, struct AdAppInfo **out, u */ void ad_free_apps(struct AdAppInfo *apps, uint32_t count); +/** + * Last-error lifetime — errno-style. + * + * The pointer returned by `ad_last_error_message`, + * `ad_last_error_suggestion`, and `ad_last_error_platform_detail` + * remains valid across any number of subsequent **successful** FFI + * calls on the same thread. Only the next FFI call that itself **fails** + * (returns a non-`AD_RESULT_OK` code) invalidates the previous pointers. + * + * Consumers can therefore read an error once, cache the pointer, and + * keep reading it back across follow-up work that clears or re-fetches + * state before handing control to the user. + * + * This matches the POSIX `errno` / `strerror` contract and is scoped + * per-thread via thread-local storage — Thread A's last-error never + * leaks to Thread B. + */ AdResult ad_last_error_code(void); const char *ad_last_error_message(void); diff --git a/crates/ffi/src/actions/execute.rs b/crates/ffi/src/actions/execute.rs index 692cd3d..e49cf01 100644 --- a/crates/ffi/src/actions/execute.rs +++ b/crates/ffi/src/actions/execute.rs @@ -37,7 +37,6 @@ pub unsafe extern "C" fn ad_execute_action( match adapter.inner.execute_action(&native_handle, core_action) { Ok(result) => { *out = action_result_to_c(&result); - error::clear_last_error(); AdResult::Ok } Err(e) => { diff --git a/crates/ffi/src/actions/resolve.rs b/crates/ffi/src/actions/resolve.rs index 260f852..4605f76 100644 --- a/crates/ffi/src/actions/resolve.rs +++ b/crates/ffi/src/actions/resolve.rs @@ -49,7 +49,6 @@ pub unsafe extern "C" fn ad_resolve_element( match adapter.inner.resolve_element(&core_entry) { Ok(handle) => { (*out).ptr = handle.as_raw(); - error::clear_last_error(); AdResult::Ok } Err(e) => { diff --git a/crates/ffi/src/adapter.rs b/crates/ffi/src/adapter.rs index f953209..703d500 100644 --- a/crates/ffi/src/adapter.rs +++ b/crates/ffi/src/adapter.rs @@ -58,10 +58,7 @@ pub unsafe extern "C" fn ad_check_permissions(adapter: *const AdAdapter) -> AdRe trap_panic(|| { let adapter = unsafe { &*adapter }; match adapter.inner.check_permissions() { - agent_desktop_core::adapter::PermissionStatus::Granted => { - error::clear_last_error(); - AdResult::Ok - } + agent_desktop_core::adapter::PermissionStatus::Granted => AdResult::Ok, agent_desktop_core::adapter::PermissionStatus::Denied { suggestion } => { error::set_last_error( &agent_desktop_core::error::AdapterError::new( diff --git a/crates/ffi/src/apps/close.rs b/crates/ffi/src/apps/close.rs index 725f8b1..f63b10f 100644 --- a/crates/ffi/src/apps/close.rs +++ b/crates/ffi/src/apps/close.rs @@ -1,5 +1,5 @@ use crate::convert::string::c_to_string; -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::AdAdapter; use std::os::raw::c_char; @@ -26,10 +26,7 @@ pub unsafe extern "C" fn ad_close_app( }; match adapter.inner.close_app(&id_str, force) { - Ok(()) => { - clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { set_last_error(&e); crate::error::last_error_code() diff --git a/crates/ffi/src/apps/launch.rs b/crates/ffi/src/apps/launch.rs index 386911b..5795f92 100644 --- a/crates/ffi/src/apps/launch.rs +++ b/crates/ffi/src/apps/launch.rs @@ -1,6 +1,6 @@ use crate::convert::string::c_to_string; use crate::convert::window::window_info_to_c; -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::types::AdWindowInfo; use crate::AdAdapter; @@ -30,7 +30,6 @@ pub unsafe extern "C" fn ad_launch_app( match adapter.inner.launch_app(&id_str, timeout_ms) { Ok(win) => { - clear_last_error(); *out = window_info_to_c(&win); AdResult::Ok } diff --git a/crates/ffi/src/apps/list.rs b/crates/ffi/src/apps/list.rs index 42be0ae..2ecc16c 100644 --- a/crates/ffi/src/apps/list.rs +++ b/crates/ffi/src/apps/list.rs @@ -1,5 +1,5 @@ use crate::convert::app::{app_info_to_c, free_app_info_fields}; -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::{trap_panic, trap_panic_void}; use crate::types::AdAppInfo; use crate::AdAdapter; @@ -21,7 +21,6 @@ pub unsafe extern "C" fn ad_list_apps( let adapter = &*adapter; match adapter.inner.list_apps() { Ok(apps) => { - clear_last_error(); let c_apps: Vec = apps.iter().map(app_info_to_c).collect(); let count = c_apps.len() as u32; if c_apps.is_empty() { diff --git a/crates/ffi/src/error.rs b/crates/ffi/src/error.rs index 6f65e07..ba6a4af 100644 --- a/crates/ffi/src/error.rs +++ b/crates/ffi/src/error.rs @@ -93,6 +93,7 @@ pub(crate) fn set_last_error(err: &AdapterError) { }); } +#[cfg(test)] pub(crate) fn clear_last_error() { LAST_ERROR.with(|cell| { *cell.borrow_mut() = None; @@ -121,6 +122,21 @@ pub(crate) fn last_error_code() -> AdResult { }) } +/// Last-error lifetime — errno-style. +/// +/// The pointer returned by `ad_last_error_message`, +/// `ad_last_error_suggestion`, and `ad_last_error_platform_detail` +/// remains valid across any number of subsequent **successful** FFI +/// calls on the same thread. Only the next FFI call that itself **fails** +/// (returns a non-`AD_RESULT_OK` code) invalidates the previous pointers. +/// +/// Consumers can therefore read an error once, cache the pointer, and +/// keep reading it back across follow-up work that clears or re-fetches +/// state before handing control to the user. +/// +/// This matches the POSIX `errno` / `strerror` contract and is scoped +/// per-thread via thread-local storage — Thread A's last-error never +/// leaks to Thread B. #[no_mangle] pub extern "C" fn ad_last_error_code() -> AdResult { crate::ffi_try::trap_panic(last_error_code) diff --git a/crates/ffi/src/input/clipboard.rs b/crates/ffi/src/input/clipboard.rs index e546b2f..67cec04 100644 --- a/crates/ffi/src/input/clipboard.rs +++ b/crates/ffi/src/input/clipboard.rs @@ -19,7 +19,6 @@ pub unsafe extern "C" fn ad_get_clipboard( match adapter.inner.get_clipboard() { Ok(text) => { *out = string_to_c(&text); - error::clear_last_error(); AdResult::Ok } Err(e) => { @@ -52,10 +51,7 @@ pub unsafe extern "C" fn ad_set_clipboard( } }; match adapter.inner.set_clipboard(&text) { - Ok(()) => { - error::clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { error::set_last_error(&e); error::last_error_code() @@ -72,10 +68,7 @@ pub unsafe extern "C" fn ad_clear_clipboard(adapter: *const AdAdapter) -> AdResu trap_panic(|| unsafe { let adapter = &*adapter; match adapter.inner.clear_clipboard() { - Ok(()) => { - error::clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { error::set_last_error(&e); error::last_error_code() diff --git a/crates/ffi/src/input/drag.rs b/crates/ffi/src/input/drag.rs index 428fc87..e8fa3d5 100644 --- a/crates/ffi/src/input/drag.rs +++ b/crates/ffi/src/input/drag.rs @@ -32,10 +32,7 @@ pub unsafe extern "C" fn ad_drag( }, }; match adapter.inner.drag(core_params) { - Ok(()) => { - error::clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { error::set_last_error(&e); error::last_error_code() diff --git a/crates/ffi/src/input/mouse.rs b/crates/ffi/src/input/mouse.rs index b3bb0a0..572a9e8 100644 --- a/crates/ffi/src/input/mouse.rs +++ b/crates/ffi/src/input/mouse.rs @@ -46,10 +46,7 @@ pub unsafe extern "C" fn ad_mouse_event( button, }; match adapter.inner.mouse_event(core_event) { - Ok(()) => { - error::clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { error::set_last_error(&e); error::last_error_code() diff --git a/crates/ffi/src/screenshot/capture.rs b/crates/ffi/src/screenshot/capture.rs index 5565332..cc45938 100644 --- a/crates/ffi/src/screenshot/capture.rs +++ b/crates/ffi/src/screenshot/capture.rs @@ -1,4 +1,4 @@ -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::types::{AdImageBuffer, AdImageFormat, AdScreenshotKind, AdScreenshotTarget}; use crate::AdAdapter; @@ -23,7 +23,6 @@ pub unsafe extern "C" fn ad_screenshot( match adapter.inner.screenshot(core_target) { Ok(img) => { - clear_last_error(); let data_len = img.data.len() as u64; let mut boxed = img.data.into_boxed_slice(); let data_ptr = boxed.as_mut_ptr(); diff --git a/crates/ffi/src/surfaces/list.rs b/crates/ffi/src/surfaces/list.rs index 594572a..caaa9b5 100644 --- a/crates/ffi/src/surfaces/list.rs +++ b/crates/ffi/src/surfaces/list.rs @@ -1,5 +1,5 @@ use crate::convert::surface::{free_surface_info_fields, surface_info_to_c}; -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::{trap_panic, trap_panic_void}; use crate::types::AdSurfaceInfo; use crate::AdAdapter; @@ -21,7 +21,6 @@ pub unsafe extern "C" fn ad_list_surfaces( let adapter = &*adapter; match adapter.inner.list_surfaces(pid) { Ok(surfaces) => { - clear_last_error(); let c_surfaces: Vec = surfaces.iter().map(surface_info_to_c).collect(); let count = c_surfaces.len() as u32; diff --git a/crates/ffi/src/tree/get.rs b/crates/ffi/src/tree/get.rs index c1d4516..bbb5cf1 100644 --- a/crates/ffi/src/tree/get.rs +++ b/crates/ffi/src/tree/get.rs @@ -1,4 +1,4 @@ -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::tree::flatten::flatten_tree; use crate::types::{AdNodeTree, AdTreeOptions, AdWindowInfo}; @@ -33,7 +33,6 @@ pub unsafe extern "C" fn ad_get_tree( match adapter.inner.get_tree(&core_win, &core_opts) { Ok(tree) => { - clear_last_error(); unsafe { *out = flatten_tree(&tree) }; AdResult::Ok } diff --git a/crates/ffi/src/windows/focus.rs b/crates/ffi/src/windows/focus.rs index 1fc9087..6fcf682 100644 --- a/crates/ffi/src/windows/focus.rs +++ b/crates/ffi/src/windows/focus.rs @@ -1,4 +1,4 @@ -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::types::AdWindowInfo; use crate::windows::to_core::ad_window_to_core; @@ -15,10 +15,7 @@ pub unsafe extern "C" fn ad_focus_window( let adapter = &*adapter; let core_win = ad_window_to_core(&*win); match adapter.inner.focus_window(&core_win) { - Ok(()) => { - clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { set_last_error(&e); crate::error::last_error_code() diff --git a/crates/ffi/src/windows/list.rs b/crates/ffi/src/windows/list.rs index 1228d0a..056af43 100644 --- a/crates/ffi/src/windows/list.rs +++ b/crates/ffi/src/windows/list.rs @@ -1,6 +1,6 @@ use crate::convert::string::c_to_string; use crate::convert::window::{free_window_info_fields, window_info_to_c}; -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::{trap_panic, trap_panic_void}; use crate::types::AdWindowInfo; use crate::AdAdapter; @@ -27,7 +27,6 @@ pub unsafe extern "C" fn ad_list_windows( }; match adapter.inner.list_windows(&filter) { Ok(windows) => { - clear_last_error(); let c_wins: Vec = windows.iter().map(window_info_to_c).collect(); let count = c_wins.len() as u32; if c_wins.is_empty() { diff --git a/crates/ffi/src/windows/op.rs b/crates/ffi/src/windows/op.rs index 4578b8e..5a19837 100644 --- a/crates/ffi/src/windows/op.rs +++ b/crates/ffi/src/windows/op.rs @@ -1,4 +1,4 @@ -use crate::error::{clear_last_error, set_last_error, AdResult}; +use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::types::{AdWindowInfo, AdWindowOp, AdWindowOpKind}; use crate::windows::to_core::ad_window_to_core; @@ -27,10 +27,7 @@ pub unsafe extern "C" fn ad_window_op( AdWindowOpKind::Restore => WindowOp::Restore, }; match adapter.inner.window_op(&core_win, core_op) { - Ok(()) => { - clear_last_error(); - AdResult::Ok - } + Ok(()) => AdResult::Ok, Err(e) => { set_last_error(&e); crate::error::last_error_code() diff --git a/crates/ffi/tests/error_lifetime.rs b/crates/ffi/tests/error_lifetime.rs new file mode 100644 index 0000000..2e6402e --- /dev/null +++ b/crates/ffi/tests/error_lifetime.rs @@ -0,0 +1,46 @@ +use agent_desktop_ffi::error::AdResult; +use std::ffi::CStr; + +#[allow(improper_ctypes)] +extern "C" { + fn ad_adapter_create() -> *mut agent_desktop_ffi::AdAdapter; + fn ad_adapter_destroy(adapter: *mut agent_desktop_ffi::AdAdapter); + fn ad_launch_app( + adapter: *const agent_desktop_ffi::AdAdapter, + id: *const std::os::raw::c_char, + timeout_ms: u64, + out: *mut agent_desktop_ffi::AdWindowInfo, + ) -> AdResult; + fn ad_last_error_message() -> *const std::os::raw::c_char; + fn ad_last_error_code() -> AdResult; + fn ad_check_permissions(adapter: *const agent_desktop_ffi::AdAdapter) -> AdResult; +} + +#[test] +fn last_error_pointer_survives_across_successful_calls() { + unsafe { + let adapter = ad_adapter_create(); + assert!(!adapter.is_null()); + + let bad_id = std::ptr::null(); + let mut out_win: agent_desktop_ffi::AdWindowInfo = std::mem::zeroed(); + let rc = ad_launch_app(adapter, bad_id, 0, &mut out_win); + assert_eq!(rc, AdResult::ErrInvalidArgs); + + let first_msg_ptr = ad_last_error_message(); + assert!(!first_msg_ptr.is_null()); + let first_msg = CStr::from_ptr(first_msg_ptr).to_string_lossy().into_owned(); + + for _ in 0..10 { + let _rc = ad_check_permissions(adapter); + } + + let later_msg_ptr = ad_last_error_message(); + assert_eq!(first_msg_ptr, later_msg_ptr); + let later_msg = CStr::from_ptr(later_msg_ptr).to_string_lossy().into_owned(); + assert_eq!(first_msg, later_msg); + assert_eq!(ad_last_error_code(), AdResult::ErrInvalidArgs); + + ad_adapter_destroy(adapter); + } +}