diff --git a/crates/ffi/src/convert/string.rs b/crates/ffi/src/convert/string.rs index 3fb15c3..91b4b93 100644 --- a/crates/ffi/src/convert/string.rs +++ b/crates/ffi/src/convert/string.rs @@ -48,6 +48,50 @@ pub(crate) unsafe fn c_to_string(ptr: *const c_char) -> Option { CStr::from_ptr(ptr).to_str().ok().map(str::to_owned) } +/// Tri-state decode of a foreign C string used for optional filter +/// fields where conflating null with invalid UTF-8 would silently widen +/// an operation (e.g. treat "bad bytes" as "no filter"). +/// +/// - `Ok(None)` — pointer is null. Caller should treat as "filter +/// absent". +/// - `Ok(Some(s))` — pointer is non-null and decodes as valid UTF-8. +/// - `Err(())` — pointer is non-null but the bytes are not UTF-8. +/// Caller should surface `AD_RESULT_ERR_INVALID_ARGS` instead of +/// treating this as missing. +/// +/// # Safety +/// `ptr` must be null or a NUL-terminated C string. +pub(crate) unsafe fn try_c_to_string(ptr: *const c_char) -> Result, ()> { + if ptr.is_null() { + return Ok(None); + } + match CStr::from_ptr(ptr).to_str() { + Ok(s) => Ok(Some(s.to_owned())), + Err(_) => Err(()), + } +} + +/// Decode an optional filter string, short-circuiting the enclosing +/// `AdResult`-returning fn with `AD_RESULT_ERR_INVALID_ARGS` (and a +/// tailored last-error diagnostic) when the pointer is non-null but +/// the bytes are not UTF-8. Null → `None` (treated as "no filter"). +macro_rules! decode_optional_filter { + ($ptr:expr, $label:expr) => {{ + match $crate::convert::string::try_c_to_string($ptr) { + Ok(value) => value, + Err(()) => { + $crate::error::set_last_error(&agent_desktop_core::error::AdapterError::new( + agent_desktop_core::error::ErrorCode::InvalidArgs, + concat!($label, " is not valid UTF-8"), + )); + return $crate::error::AdResult::ErrInvalidArgs; + } + } + }}; +} + +pub(crate) use decode_optional_filter; + #[cfg(test)] mod tests { use super::*; @@ -100,4 +144,25 @@ mod tests { assert_eq!(back, "\u{FFFD}\u{FFFD}\u{FFFD}"); unsafe { free_c_string(c) }; } + + #[test] + fn try_c_to_string_null_is_ok_none() { + let result = unsafe { try_c_to_string(ptr::null()) }; + assert!(matches!(result, Ok(None))); + } + + #[test] + fn try_c_to_string_valid_utf8_is_some() { + let c = string_to_c("agent"); + let result = unsafe { try_c_to_string(c) }; + assert!(matches!(result, Ok(Some(ref s)) if s == "agent")); + unsafe { free_c_string(c) }; + } + + #[test] + fn try_c_to_string_invalid_utf8_is_err() { + let bad: [u8; 3] = [0xC3, 0xFF, 0x00]; + let result = unsafe { try_c_to_string(bad.as_ptr() as *const c_char) }; + assert!(matches!(result, Err(()))); + } } diff --git a/crates/ffi/src/notifications/dismiss.rs b/crates/ffi/src/notifications/dismiss.rs index e695211..81269a6 100644 --- a/crates/ffi/src/notifications/dismiss.rs +++ b/crates/ffi/src/notifications/dismiss.rs @@ -1,4 +1,4 @@ -use crate::convert::string::c_to_string; +use crate::convert::string::decode_optional_filter; use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::AdAdapter; @@ -23,7 +23,7 @@ pub unsafe extern "C" fn ad_dismiss_notification( } crate::pointer_guard::guard_non_null!(adapter, c"adapter is null"); let adapter = &*adapter; - let filter = c_to_string(app_filter); + let filter = decode_optional_filter!(app_filter, "app_filter"); let filter_ref = filter.as_deref(); match adapter .inner diff --git a/crates/ffi/src/notifications/dismiss_all.rs b/crates/ffi/src/notifications/dismiss_all.rs index 1816500..c26d72f 100644 --- a/crates/ffi/src/notifications/dismiss_all.rs +++ b/crates/ffi/src/notifications/dismiss_all.rs @@ -1,5 +1,5 @@ use crate::convert::notification::notification_info_to_c; -use crate::convert::string::c_to_string; +use crate::convert::string::decode_optional_filter; use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::notifications::list::ad_notification_list_free; @@ -40,7 +40,7 @@ pub unsafe extern "C" fn ad_dismiss_all_notifications( } crate::pointer_guard::guard_non_null!(adapter, c"adapter is null"); let adapter = &*adapter; - let filter = c_to_string(app_filter); + let filter = decode_optional_filter!(app_filter, "app_filter"); let filter_ref = filter.as_deref(); match adapter.inner.dismiss_all_notifications(filter_ref) { Ok((dismissed, failed_messages)) => { diff --git a/crates/ffi/src/notifications/filter.rs b/crates/ffi/src/notifications/filter.rs index be5721c..6c3b72a 100644 --- a/crates/ffi/src/notifications/filter.rs +++ b/crates/ffi/src/notifications/filter.rs @@ -1,25 +1,39 @@ -use crate::convert::string::c_to_string; +use crate::convert::string::try_c_to_string; use crate::types::AdNotificationFilter; +use agent_desktop_core::error::{AdapterError, ErrorCode}; use agent_desktop_core::notification::NotificationFilter; /// Converts a C `AdNotificationFilter` into the core filter type. -/// Null pointers become `None`; `has_limit == false` clears the limit. +/// +/// - `null` pointer → `Ok(NotificationFilter::default())` (no filter). +/// - `has_limit == false` → the numeric limit is cleared regardless of +/// `limit` contents. +/// - Non-null `app` / `text` with invalid UTF-8 → `Err` rather than +/// silently dropping the filter (which would widen operations like +/// `ad_dismiss_all_notifications` to every app on the system). /// /// # Safety /// `filter` must be null or point to a valid `AdNotificationFilter`. /// The embedded C strings must outlive this call. -pub(crate) unsafe fn filter_from_c(filter: *const AdNotificationFilter) -> NotificationFilter { +pub(crate) unsafe fn filter_from_c( + filter: *const AdNotificationFilter, +) -> Result { if filter.is_null() { - return NotificationFilter::default(); + return Ok(NotificationFilter::default()); } let f: &AdNotificationFilter = unsafe { &*filter }; - NotificationFilter { - app: unsafe { c_to_string(f.app) }, - text: unsafe { c_to_string(f.text) }, + let app = unsafe { try_c_to_string(f.app) } + .map_err(|()| AdapterError::new(ErrorCode::InvalidArgs, "filter.app is not valid UTF-8"))?; + let text = unsafe { try_c_to_string(f.text) }.map_err(|()| { + AdapterError::new(ErrorCode::InvalidArgs, "filter.text is not valid UTF-8") + })?; + Ok(NotificationFilter { + app, + text, limit: if f.has_limit { Some(f.limit as usize) } else { None }, - } + }) } diff --git a/crates/ffi/src/notifications/list.rs b/crates/ffi/src/notifications/list.rs index dcfc2de..0d7aa75 100644 --- a/crates/ffi/src/notifications/list.rs +++ b/crates/ffi/src/notifications/list.rs @@ -31,7 +31,13 @@ pub unsafe extern "C" fn ad_list_notifications( } crate::pointer_guard::guard_non_null!(adapter, c"adapter is null"); let adapter = &*adapter; - let core_filter = filter_from_c(filter); + let core_filter = match filter_from_c(filter) { + Ok(f) => f, + Err(e) => { + set_last_error(&e); + return crate::error::last_error_code(); + } + }; match adapter.inner.list_notifications(&core_filter) { Ok(notifications) => { let items: Vec = diff --git a/crates/ffi/src/observation/find.rs b/crates/ffi/src/observation/find.rs index eee739f..2f24459 100644 --- a/crates/ffi/src/observation/find.rs +++ b/crates/ffi/src/observation/find.rs @@ -1,4 +1,4 @@ -use crate::convert::string::c_to_string; +use crate::convert::string::decode_optional_filter; use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::observation::walk::find_first_match; @@ -47,9 +47,9 @@ pub unsafe extern "C" fn ad_find( } }; let q = &*query; - let role_filter = c_to_string(q.role); - let name_filter = c_to_string(q.name_substring); - let value_filter = c_to_string(q.value_substring); + let role_filter = decode_optional_filter!(q.role, "query.role"); + let name_filter = decode_optional_filter!(q.name_substring, "query.name_substring"); + let value_filter = decode_optional_filter!(q.value_substring, "query.value_substring"); // include_bounds must be true: the resolver disambiguates // duplicate-label siblings using bounds_hash, and without the diff --git a/crates/ffi/src/observation/is.rs b/crates/ffi/src/observation/is.rs index 412fc91..5494815 100644 --- a/crates/ffi/src/observation/is.rs +++ b/crates/ffi/src/observation/is.rs @@ -1,4 +1,4 @@ -use crate::convert::string::c_to_string; +use crate::convert::string::{c_to_string, decode_optional_filter}; use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::types::{AdFindQuery, AdWindowInfo}; @@ -62,9 +62,9 @@ pub unsafe extern "C" fn ad_is( } }; let q = &*query; - let role_filter = c_to_string(q.role); - let name_filter = c_to_string(q.name_substring); - let value_filter = c_to_string(q.value_substring); + let role_filter = decode_optional_filter!(q.role, "query.role"); + let name_filter = decode_optional_filter!(q.name_substring, "query.name_substring"); + let value_filter = decode_optional_filter!(q.value_substring, "query.value_substring"); let prop = match c_to_string(property) { Some(s) => s, None => { diff --git a/crates/ffi/src/windows/list.rs b/crates/ffi/src/windows/list.rs index 09d8219..93e7059 100644 --- a/crates/ffi/src/windows/list.rs +++ b/crates/ffi/src/windows/list.rs @@ -1,4 +1,4 @@ -use crate::convert::string::c_to_string; +use crate::convert::string::decode_optional_filter; use crate::convert::window::{free_window_info_fields, window_info_to_c}; use crate::error::{set_last_error, AdResult}; use crate::ffi_try::{trap_panic, trap_panic_void}; @@ -29,7 +29,7 @@ pub unsafe extern "C" fn ad_list_windows( let adapter = &*adapter; let filter = WindowFilter { focused_only, - app: c_to_string(app_filter), + app: decode_optional_filter!(app_filter, "app_filter"), }; match adapter.inner.list_windows(&filter) { Ok(windows) => { diff --git a/crates/ffi/tests/c_abi_harness.rs b/crates/ffi/tests/c_abi_harness.rs index eb12db6..523c6f7 100644 --- a/crates/ffi/tests/c_abi_harness.rs +++ b/crates/ffi/tests/c_abi_harness.rs @@ -198,6 +198,27 @@ fn dirty_out_param_is_cleared_before_early_return_on_worker_thread() { }); } +#[test] +fn invalid_utf8_filter_rejected_not_silently_widened() { + // Regression for todo 010: prior c_to_string conflated null with + // invalid UTF-8, so a non-null buffer with bogus bytes in the + // app_filter slot would be treated as "no filter" and widen + // ad_list_windows to every app on the system. Must now fail closed. + with_adapter(|adapter| unsafe { + let bad: [u8; 2] = [0xC3, 0x00]; + let mut list: *mut AdWindowList = std::ptr::null_mut(); + let rc = ad_list_windows(adapter, bad.as_ptr() as *const c_char, false, &mut list); + // Main-thread guard (ErrInternal on worker) or UTF-8 rejection + // (ErrInvalidArgs) — either way we do NOT produce a list by + // silently treating bad bytes as "no filter". + assert!(matches!( + rc, + AdResult::ErrInvalidArgs | AdResult::ErrInternal + )); + assert!(list.is_null()); + }); +} + #[test] fn null_out_param_rejected_before_write() { with_adapter(|adapter| unsafe {