mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-05 05:30:21 +00:00
feat(ffi): errno-style last-error lifetime (Unit 3)
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.
This commit is contained in:
parent
c3856c1be4
commit
b7af53e920
19 changed files with 97 additions and 51 deletions
|
|
@ -6,7 +6,7 @@ license.workspace = true
|
|||
publish = false
|
||||
|
||||
[lib]
|
||||
crate-type = ["cdylib"]
|
||||
crate-type = ["cdylib", "rlib"]
|
||||
|
||||
[dependencies]
|
||||
agent-desktop-core.workspace = true
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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) => {
|
||||
|
|
|
|||
|
|
@ -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) => {
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<AdAppInfo> = apps.iter().map(app_info_to_c).collect();
|
||||
let count = c_apps.len() as u32;
|
||||
if c_apps.is_empty() {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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<AdSurfaceInfo> =
|
||||
surfaces.iter().map(surface_info_to_c).collect();
|
||||
let count = c_surfaces.len() as u32;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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<AdWindowInfo> = windows.iter().map(window_info_to_c).collect();
|
||||
let count = c_wins.len() as u32;
|
||||
if c_wins.is_empty() {
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
46
crates/ffi/tests/error_lifetime.rs
Normal file
46
crates/ffi/tests/error_lifetime.rs
Normal file
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue