mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-04 13:16:06 +00:00
fix(ffi): UTF-8 fail-closed for optional filter pointers (todo 010)
Closes the UTF-8 portion of P2 todo 010. The resolver relaxed-pass
stays — it's the right behavior for ad_find's lossy-rebuild fallback
and the recently-landed bounds_hash preservation already constrains
the duplicate-label drift case.
The UTF-8 side needed a tighter contract. `c_to_string` returned
`Option<String>` and collapsed null + invalid-bytes into the same
`None`, so an app_filter passed in with hostile bytes was silently
treated as "no filter" and widened ad_list_windows,
ad_dismiss_all_notifications, ad_find, etc. to every app on the box.
Add `try_c_to_string` returning `Result<Option<String>, ()>`:
- `Ok(None)` — null pointer (caller treats as absent).
- `Ok(Some(s))` — valid UTF-8.
- `Err(())` — non-null + invalid UTF-8 (caller must surface
AD_RESULT_ERR_INVALID_ARGS).
Add `decode_optional_filter!` macro so call sites read as a single
line:
let role_filter = decode_optional_filter!(q.role, "query.role");
The macro shortcircuits the enclosing AdResult fn with InvalidArgs
and a tailored last-error ("<label> is not valid UTF-8").
Applied to every optional filter pointer consumer:
- observation/find.rs: role, name_substring, value_substring
- observation/is.rs: role, name_substring, value_substring
- windows/list.rs: app_filter
- notifications/dismiss.rs: app_filter
- notifications/dismiss_all.rs: app_filter
- notifications/filter.rs: filter_from_c returns Result; app/text
fields fail-closed. list_notifications call site propagates.
Regression test in c_abi_harness.rs:
- invalid_utf8_filter_rejected_not_silently_widened: passes a
truncated-UTF-8 byte pair as ad_list_windows app_filter; must return
InvalidArgs (or ErrInternal on worker) with list still null, never
a populated list built from the widened "all apps" scope.
85 FFI tests pass, clippy clean.
This commit is contained in:
parent
b45f17763b
commit
394e29ba13
9 changed files with 129 additions and 23 deletions
|
|
@ -48,6 +48,50 @@ pub(crate) unsafe fn c_to_string(ptr: *const c_char) -> Option<String> {
|
|||
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<Option<String>, ()> {
|
||||
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(())));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)) => {
|
||||
|
|
|
|||
|
|
@ -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<NotificationFilter, AdapterError> {
|
||||
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
|
||||
},
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<AdNotificationInfo> =
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 => {
|
||||
|
|
|
|||
|
|
@ -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) => {
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Reference in a new issue