diff --git a/docs/tst-desk-backlog.md b/docs/tst-desk-backlog.md index 2ae75058..77b93d52 100644 --- a/docs/tst-desk-backlog.md +++ b/docs/tst-desk-backlog.md @@ -13041,6 +13041,29 @@ step 1; elsewhere the binary comes from `binary` or `PATH`. --- +### TD-5225 — Reading a window's accessibility tree crashes TST Desk +**Size:** 2 · **Depends on:** TD-4829 +**Status:** In review 2026-11-02. + +`ui_snapshot` through the host (`cu-agent.sock`, json op `ui_snapshot`) +aborted the whole app on macOS, for any target app, about a second after +the call. The host read every `AXValue`, `AXTitle` and similar attribute +as a CFString, but `AXValue` is a number on a checkbox or slider and an +AXValueRef on a scroll area. CoreFoundation throws on a non-string, and +Rust aborts on a foreign exception. The daemon then shut down with the +host, so every computer-use client lost its tools until the app was +reopened, and tst-cu-mcp quietly fell back to an empty local snapshot. +Separately, children taken out of a CFArray were used after the array +was released, which freed them. + +**Acceptance criteria:** +- [x] Attribute reads check the CF type: strings as-is, numbers and booleans written out, anything else empty; boolean reads accept CFBoolean or a number +- [x] Elements taken from a CFArray hold their own reference until the walk is done; `ui_action` keeps the element it found alive while acting on it +- [x] Unit tests on real CF objects, each shown to fail with its fix removed (no type check: the test process aborts; no retain: it segfaults) +- [x] Live on macOS: `ui_snapshot` of Finder and of TST Desk itself returns a tree with `path: host`, and the app keeps running + +--- + ### TD-5224 — A system-wide stop shortcut for computer use **Size:** 3 · **Depends on:** TD-5223 **Status:** In review 2026-11-02. diff --git a/shell/src/cu_ax.rs b/shell/src/cu_ax.rs index b962adaf..058e2a1d 100644 --- a/shell/src/cu_ax.rs +++ b/shell/src/cu_ax.rs @@ -3,15 +3,17 @@ //! Checkout Python is the wrong TCC identity. AXUIElement reads and //! AXPress/AXSetValue run here so System Settings attributes them to //! **TST Desk**. Nested tree JSON is flattened by the Python MCP layer. +//! Every CF value goes through `cu_cf`, which checks its type and owns its +//! reference (TD-5225). #![cfg(target_os = "macos")] -use std::ffi::{c_char, c_void, CStr, CString}; -use std::ptr; +use std::ffi::c_void; use serde_json::{json, Value}; -const K_CF_STRING_ENCODING_UTF8: u32 = 0x0800_0100; +use crate::cu_cf::{self, Owned}; + const AX_OK: i32 = 0; const MAX_CHILDREN: usize = 200; const MAX_DEPTH: usize = 24; @@ -33,27 +35,6 @@ extern "C" { ) -> i32; } -#[link(name = "CoreFoundation", kind = "framework")] -extern "C" { - fn CFRelease(cf: *const c_void); - fn CFStringCreateWithCString( - alloc: *mut c_void, - c_str: *const c_char, - encoding: u32, - ) -> *mut c_void; - fn CFStringGetLength(the_string: *const c_void) -> isize; - fn CFStringGetCString( - the_string: *const c_void, - buffer: *mut c_char, - buffer_size: isize, - encoding: u32, - ) -> u8; - fn CFArrayGetCount(the_array: *const c_void) -> isize; - fn CFArrayGetValueAtIndex(the_array: *const c_void, idx: isize) -> *const c_void; - fn CFBooleanGetValue(boolean: *const c_void) -> u8; - static kCFBooleanTrue: *const c_void; -} - pub fn handle(body: &Value) -> Value { let op = body.get("op").and_then(Value::as_str).unwrap_or(""); match op { @@ -69,12 +50,10 @@ fn snapshot(body: &Value) -> Value { return json!({"ok": false, "error": "pid must be a positive process id"}); } unsafe { - let app = AXUIElementCreateApplication(pid); - if app.is_null() { + let Some(app) = Owned::adopt(AXUIElementCreateApplication(pid)) else { return json!({"ok": false, "error": format!("no accessibility element for pid {pid}")}); - } - let tree = read_app(app); - CFRelease(app); + }; + let tree = read_app(app.as_ptr()); json!({"ok": true, "pid": pid, "tree": tree, "path": "host"}) } } @@ -91,25 +70,21 @@ fn act(body: &Value) -> Value { return json!({"ok": false, "error": "element id is empty"}); } unsafe { - let app = AXUIElementCreateApplication(pid); - if app.is_null() { + let Some(app) = Owned::adopt(AXUIElementCreateApplication(pid)) else { return json!({"ok": false, "error": format!("no accessibility element for pid {pid}")}); - } - let result = match element_at(app, id) { - Some(el) => perform(el, action, value), - None => json!({"ok": false, "error": format!("element {id} is not in the tree")}), }; - CFRelease(app); - result + match element_at(app.as_ptr(), id) { + Some(el) => perform(el.as_ptr(), action, value), + None => json!({"ok": false, "error": format!("element {id} is not in the tree")}), + } } } unsafe fn read_app(app: *mut c_void) -> Value { - let windows = attr_list(app, "AXWindows"); - let children: Vec = windows - .into_iter() + let children: Vec = attr_list(app, "AXWindows") + .iter() .take(MAX_CHILDREN) - .map(|child| read_element(child as *mut c_void, 1)) + .map(|child| read_element(child.as_ptr(), 1)) .collect(); json!({ "role": "AXApplication", @@ -128,9 +103,9 @@ unsafe fn read_element(element: *mut c_void, depth: usize) -> Value { Vec::new() } else { attr_list(element, "AXChildren") - .into_iter() + .iter() .take(MAX_CHILDREN) - .map(|child| read_element(child as *mut c_void, depth + 1)) + .map(|child| read_element(child.as_ptr(), depth + 1)) .collect() }; let mut description = attr_string(element, "AXDescription"); @@ -149,21 +124,25 @@ unsafe fn read_element(element: *mut c_void, depth: usize) -> Value { }) } -unsafe fn element_at(app: *mut c_void, element_id: &str) -> Option<*mut c_void> { - let mut node = app; +/// The element at a dotted path (`window.child.child…`), holding its own +/// reference so it outlives the lists it was found in. +unsafe fn element_at(app: *mut c_void, element_id: &str) -> Option { + let mut node: Option = None; for (index, part) in element_id.split('.').enumerate() { let pos: usize = part.parse().ok()?; - let siblings = if index == 0 { - attr_list(node, "AXWindows") + let parent = node.as_ref().map_or(app, Owned::as_ptr); + let name = if index == 0 { + "AXWindows" } else { - attr_list(node, "AXChildren") + "AXChildren" }; - node = *siblings.get(pos)? as *mut c_void; - if node.is_null() { + let mut siblings = attr_list(parent, name); + if pos >= siblings.len() { return None; } + node = Some(siblings.swap_remove(pos)); } - Some(node) + node } unsafe fn perform(element: *mut c_void, action: &str, value: &str) -> Value { @@ -171,8 +150,11 @@ unsafe fn perform(element: *mut c_void, action: &str, value: &str) -> Value { "press" => perform_named(element, "AXPress"), "raise" => perform_named(element, "AXRaise"), "show_menu" => perform_named(element, "AXShowMenu"), - "focus" => set_bool(element, "AXFocused"), - "set_value" => set_string(element, "AXValue", value), + "focus" => set_value(element, "AXFocused", cu_cf::boolean_true()), + "set_value" => match cu_cf::cfstring(value) { + Some(text) => set_value(element, "AXValue", text.as_ptr()), + None => -1, + }, other => { return json!({"ok": false, "error": format!("unknown ui action {other}")}); } @@ -184,151 +166,63 @@ unsafe fn perform(element: *mut c_void, action: &str, value: &str) -> Value { } unsafe fn perform_named(element: *mut c_void, name: &str) -> i32 { - let cf = cfstring(name); - if cf.is_null() { - return -1; + match cu_cf::cfstring(name) { + Some(action) => AXUIElementPerformAction(element, action.as_ptr()), + None => -1, } - let err = AXUIElementPerformAction(element, cf); - CFRelease(cf); - err } -unsafe fn set_string(element: *mut c_void, name: &str, value: &str) -> i32 { - let attr = cfstring(name); - let cf_val = cfstring(value); - if attr.is_null() || cf_val.is_null() { - if !attr.is_null() { - CFRelease(attr); - } - if !cf_val.is_null() { - CFRelease(cf_val); - } - return -1; +unsafe fn set_value(element: *mut c_void, name: &str, value: *const c_void) -> i32 { + match cu_cf::cfstring(name) { + Some(attr) => AXUIElementSetAttributeValue(element, attr.as_ptr(), value), + None => -1, } - let err = AXUIElementSetAttributeValue(element, attr, cf_val); - CFRelease(attr); - CFRelease(cf_val); - err } -unsafe fn set_bool(element: *mut c_void, name: &str) -> i32 { - let attr = cfstring(name); - if attr.is_null() { - return -1; +unsafe fn attr_copy(element: *mut c_void, name: &str) -> Option { + let attr = cu_cf::cfstring(name)?; + let mut out: *mut c_void = std::ptr::null_mut(); + if AXUIElementCopyAttributeValue(element, attr.as_ptr(), &mut out) != AX_OK { + return None; } - let err = AXUIElementSetAttributeValue(element, attr, kCFBooleanTrue); - CFRelease(attr); - err -} - -unsafe fn attr_copy(element: *mut c_void, name: &str) -> *mut c_void { - let attr = cfstring(name); - if attr.is_null() { - return ptr::null_mut(); - } - let mut out: *mut c_void = ptr::null_mut(); - let err = AXUIElementCopyAttributeValue(element, attr, &mut out); - CFRelease(attr); - if err != AX_OK { - return ptr::null_mut(); - } - out + Owned::adopt(out) } unsafe fn attr_string(element: *mut c_void, name: &str) -> String { - let out = attr_copy(element, name); - if out.is_null() { - return String::new(); - } - let text = cf_to_string(out); - CFRelease(out); - text + attr_copy(element, name).map_or_else(String::new, |v| cu_cf::text(v.as_ptr())) } unsafe fn attr_bool(element: *mut c_void, name: &str, default: bool) -> bool { - let out = attr_copy(element, name); - if out.is_null() { - return default; - } - let value = CFBooleanGetValue(out) != 0; - CFRelease(out); - value + attr_copy(element, name) + .and_then(|v| cu_cf::flag(v.as_ptr())) + .unwrap_or(default) } -unsafe fn attr_list(element: *mut c_void, name: &str) -> Vec<*const c_void> { - let out = attr_copy(element, name); - if out.is_null() { - return Vec::new(); - } - let count = CFArrayGetCount(out); - let mut items = Vec::new(); - if count > 0 { - for idx in 0..count { - let item = CFArrayGetValueAtIndex(out, idx); - if !item.is_null() { - items.push(item); - } - } - } - CFRelease(out); - items +unsafe fn attr_list(element: *mut c_void, name: &str) -> Vec { + attr_copy(element, name).map_or_else(Vec::new, |v| cu_cf::items(v.as_ptr())) } unsafe fn action_names(element: *mut c_void) -> Vec { - let mut names_ref: *mut c_void = ptr::null_mut(); - let err = AXUIElementCopyActionNames(element, &mut names_ref); + let mut names_ref: *mut c_void = std::ptr::null_mut(); let mut names: Vec = Vec::new(); - if err == AX_OK && !names_ref.is_null() { - let count = CFArrayGetCount(names_ref); - for idx in 0..count { - let item = CFArrayGetValueAtIndex(names_ref, idx); - let token = cf_to_string(item); - let mapped = match token.as_str() { - "AXPress" | "AXConfirm" => Some("press"), - "AXRaise" => Some("raise"), - "AXShowMenu" => Some("show_menu"), - _ => None, - }; - if let Some(name) = mapped { - if !names.iter().any(|existing| existing == name) { - names.push(name.to_string()); + if AXUIElementCopyActionNames(element, &mut names_ref) == AX_OK { + if let Some(list) = Owned::adopt(names_ref) { + for item in cu_cf::items(list.as_ptr()) { + let mapped = match cu_cf::text(item.as_ptr()).as_str() { + "AXPress" | "AXConfirm" => Some("press"), + "AXRaise" => Some("raise"), + "AXShowMenu" => Some("show_menu"), + _ => None, + }; + if let Some(name) = mapped { + if !names.iter().any(|existing| existing == name) { + names.push(name.to_string()); + } } } } - CFRelease(names_ref); } names.push("set_value".into()); names.push("focus".into()); names } - -fn cfstring(s: &str) -> *mut c_void { - let Ok(c) = CString::new(s) else { - return ptr::null_mut(); - }; - unsafe { CFStringCreateWithCString(ptr::null_mut(), c.as_ptr(), K_CF_STRING_ENCODING_UTF8) } -} - -unsafe fn cf_to_string(cf: *const c_void) -> String { - if cf.is_null() { - return String::new(); - } - let len = CFStringGetLength(cf); - if len < 0 { - return String::new(); - } - let cap = (len as usize).saturating_mul(4).saturating_add(1); - let mut buf = vec![0u8; cap]; - let ok = CFStringGetCString( - cf, - buf.as_mut_ptr().cast::(), - cap as isize, - K_CF_STRING_ENCODING_UTF8, - ); - if ok == 0 { - return String::new(); - } - CStr::from_ptr(buf.as_ptr().cast::()) - .to_string_lossy() - .into_owned() -} diff --git a/shell/src/cu_cf.rs b/shell/src/cu_cf.rs new file mode 100644 index 00000000..e9d10eaa --- /dev/null +++ b/shell/src/cu_cf.rs @@ -0,0 +1,319 @@ +//! Core Foundation reads for the host's accessibility tools (TD-5225). +//! +//! Accessibility attributes are loosely typed. `AXValue` is a string on a +//! text field, a number on a checkbox or slider, and an AXValueRef on a +//! scroll area. Reading every value as a CFString aborted the whole app on +//! the first checkbox, so each read checks the CF type first. +//! +//! Items taken out of a CFArray are retained: the array owns them, and +//! releasing it would free elements the tree walk is still reading. + +#![cfg(target_os = "macos")] + +use std::ffi::{c_char, c_void, CStr, CString}; +use std::ptr; + +const K_CF_STRING_ENCODING_UTF8: u32 = 0x0800_0100; +// i32, matching the declaration in cu_identity.rs (the extern must agree). +const K_CF_NUMBER_DOUBLE_TYPE: i32 = 13; + +#[link(name = "CoreFoundation", kind = "framework")] +extern "C" { + fn CFRetain(cf: *const c_void) -> *const c_void; + fn CFRelease(cf: *const c_void); + fn CFGetTypeID(cf: *const c_void) -> usize; + fn CFStringGetTypeID() -> usize; + fn CFNumberGetTypeID() -> usize; + fn CFBooleanGetTypeID() -> usize; + fn CFArrayGetTypeID() -> usize; + fn CFStringCreateWithCString( + alloc: *mut c_void, + c_str: *const c_char, + encoding: u32, + ) -> *mut c_void; + fn CFStringGetLength(the_string: *const c_void) -> isize; + fn CFStringGetCString( + the_string: *const c_void, + buffer: *mut c_char, + buffer_size: isize, + encoding: u32, + ) -> u8; + fn CFNumberGetValue(number: *const c_void, the_type: i32, value_ptr: *mut c_void) -> u8; + fn CFBooleanGetValue(boolean: *const c_void) -> u8; + fn CFArrayGetCount(the_array: *const c_void) -> isize; + fn CFArrayGetValueAtIndex(the_array: *const c_void, idx: isize) -> *const c_void; + static kCFBooleanTrue: *const c_void; +} + +/// One reference to a CF object, released on drop. +pub struct Owned(*const c_void); + +impl Owned { + /// Adopt a +1 reference (a Create or Copy result). `None` for null. + /// + /// # Safety + /// `cf` must be null or a CF object whose reference the caller owns. + pub unsafe fn adopt(cf: *const c_void) -> Option { + (!cf.is_null()).then_some(Self(cf)) + } + + /// Retain a borrowed reference so it outlives its container. + /// + /// # Safety + /// `cf` must be null or a live CF object. + pub unsafe fn retain(cf: *const c_void) -> Option { + if cf.is_null() { + return None; + } + CFRetain(cf); + Some(Self(cf)) + } + + pub fn as_ptr(&self) -> *mut c_void { + self.0.cast_mut() + } +} + +impl Drop for Owned { + fn drop(&mut self) { + // SAFETY: an Owned always holds exactly one reference it took. + unsafe { CFRelease(self.0) } + } +} + +pub fn cfstring(s: &str) -> Option { + let c = CString::new(s).ok()?; + // SAFETY: Create returns a +1 reference or null. + unsafe { + Owned::adopt(CFStringCreateWithCString( + ptr::null_mut(), + c.as_ptr(), + K_CF_STRING_ENCODING_UTF8, + )) + } +} + +pub fn boolean_true() -> *const c_void { + // SAFETY: a CoreFoundation constant that lives for the process. + unsafe { kCFBooleanTrue } +} + +/// Text for an attribute value: strings as-is, numbers and booleans +/// written out, anything else (AXValueRef, arrays, elements) empty. +/// +/// # Safety +/// `cf` must be null or a live CF object. +pub unsafe fn text(cf: *const c_void) -> String { + if cf.is_null() { + return String::new(); + } + let type_id = CFGetTypeID(cf); + if type_id == CFStringGetTypeID() { + string_of(cf) + } else if type_id == CFNumberGetTypeID() { + number_of(cf).map_or_else(String::new, format_number) + } else if type_id == CFBooleanGetTypeID() { + (CFBooleanGetValue(cf) != 0).to_string() + } else { + String::new() + } +} + +/// A boolean attribute: CFBoolean, or a number where nonzero is true +/// (checkbox values). `None` for any other type. +/// +/// # Safety +/// `cf` must be null or a live CF object. +pub unsafe fn flag(cf: *const c_void) -> Option { + if cf.is_null() { + return None; + } + let type_id = CFGetTypeID(cf); + if type_id == CFBooleanGetTypeID() { + Some(CFBooleanGetValue(cf) != 0) + } else if type_id == CFNumberGetTypeID() { + number_of(cf).map(|n| n != 0.0) + } else { + None + } +} + +/// The items of a CFArray, each retained. Empty when `cf` is not an array. +/// +/// # Safety +/// `cf` must be null or a live CF object. +pub unsafe fn items(cf: *const c_void) -> Vec { + if cf.is_null() || CFGetTypeID(cf) != CFArrayGetTypeID() { + return Vec::new(); + } + let count = CFArrayGetCount(cf).max(0); + (0..count) + .filter_map(|idx| Owned::retain(CFArrayGetValueAtIndex(cf, idx))) + .collect() +} + +unsafe fn string_of(cf: *const c_void) -> String { + let len = CFStringGetLength(cf); + if len < 0 { + return String::new(); + } + let cap = (len as usize).saturating_mul(4).saturating_add(1); + let mut buf = vec![0u8; cap]; + let ok = CFStringGetCString( + cf, + buf.as_mut_ptr().cast::(), + cap as isize, + K_CF_STRING_ENCODING_UTF8, + ); + if ok == 0 { + return String::new(); + } + CStr::from_ptr(buf.as_ptr().cast::()) + .to_string_lossy() + .into_owned() +} + +unsafe fn number_of(cf: *const c_void) -> Option { + let mut value = 0.0_f64; + // A lossy conversion returns false but still stores the nearest double, + // which is close enough to show. + CFNumberGetValue(cf, K_CF_NUMBER_DOUBLE_TYPE, ptr::addr_of_mut!(value).cast()); + value.is_finite().then_some(value) +} + +fn format_number(n: f64) -> String { + if n.fract() == 0.0 && n.abs() < 1e15 { + format!("{}", n as i64) + } else { + n.to_string() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[link(name = "CoreFoundation", kind = "framework")] + extern "C" { + fn CFNumberCreate( + alloc: *const c_void, + the_type: i32, + value_ptr: *const c_void, + ) -> *const c_void; + fn CFArrayCreate( + alloc: *const c_void, + values: *const *const c_void, + num_values: isize, + callbacks: *const c_void, + ) -> *const c_void; + fn CFStringCreateMutable(alloc: *const c_void, max_length: isize) -> *const c_void; + fn CFStringAppendCString(s: *const c_void, c_str: *const c_char, encoding: u32); + fn CFGetRetainCount(cf: *const c_void) -> isize; + static kCFTypeArrayCallBacks: c_void; + static kCFBooleanFalse: *const c_void; + } + + fn number(n: f64) -> Owned { + unsafe { + Owned::adopt(CFNumberCreate( + ptr::null(), + K_CF_NUMBER_DOUBLE_TYPE, + ptr::addr_of!(n).cast(), + )) + .unwrap() + } + } + + fn array(values: &[*const c_void]) -> Owned { + unsafe { + Owned::adopt(CFArrayCreate( + ptr::null(), + values.as_ptr(), + values.len() as isize, + ptr::addr_of!(kCFTypeArrayCallBacks), + )) + .unwrap() + } + } + + /// A heap string (not a constant or tagged pointer), so its retain + /// count is meaningful. + fn heap_string(text: &str) -> Owned { + let c = CString::new(text).unwrap(); + unsafe { + let s = Owned::adopt(CFStringCreateMutable(ptr::null(), 0)).unwrap(); + CFStringAppendCString(s.0, c.as_ptr(), K_CF_STRING_ENCODING_UTF8); + s + } + } + + #[test] + fn text_reads_strings_numbers_and_booleans() { + let s = cfstring("Save").unwrap(); + unsafe { + assert_eq!(text(s.0), "Save"); + assert_eq!(text(number(1.0).0), "1"); + assert_eq!(text(number(0.5).0), "0.5"); + assert_eq!(text(boolean_true()), "true"); + assert_eq!(text(kCFBooleanFalse), "false"); + assert_eq!(text(ptr::null()), ""); + } + } + + #[test] + fn text_of_a_non_string_value_is_empty_not_a_crash() { + // An AXValue can be an array or an AXValueRef; reading it as a + // string aborted the host (TD-5225). + let inner = cfstring("x").unwrap(); + let list = array(&[inner.0]); + unsafe { + assert_eq!(text(list.0), ""); + } + } + + #[test] + fn flag_reads_booleans_and_numbers_only() { + let s = cfstring("yes").unwrap(); + unsafe { + assert_eq!(flag(boolean_true()), Some(true)); + assert_eq!(flag(kCFBooleanFalse), Some(false)); + assert_eq!(flag(number(1.0).0), Some(true)); + assert_eq!(flag(number(0.0).0), Some(false)); + assert_eq!(flag(s.0), None); + assert_eq!(flag(ptr::null()), None); + } + } + + #[test] + fn items_hold_their_own_reference() { + let child = heap_string("child"); + let list = array(&[child.0]); + drop(child); // the array is now the only owner + unsafe { + assert_eq!(CFGetRetainCount(CFArrayGetValueAtIndex(list.0, 0)), 1); + let got = items(list.0); + assert_eq!(got.len(), 1); + assert_eq!(CFGetRetainCount(got[0].0), 2); + drop(list); + assert_eq!(CFGetRetainCount(got[0].0), 1); + assert_eq!(text(got[0].0), "child"); + } + } + + #[test] + fn items_of_a_non_array_is_empty() { + let s = cfstring("not a list").unwrap(); + unsafe { + assert!(items(s.0).is_empty()); + assert!(items(ptr::null()).is_empty()); + } + } + + #[test] + fn whole_numbers_print_without_a_decimal_point() { + assert_eq!(format_number(3.0), "3"); + assert_eq!(format_number(-2.0), "-2"); + assert_eq!(format_number(0.25), "0.25"); + assert_eq!(format_number(1e20), "100000000000000000000"); + } +} diff --git a/shell/src/lib.rs b/shell/src/lib.rs index 3f3ec6ca..2b322960 100644 --- a/shell/src/lib.rs +++ b/shell/src/lib.rs @@ -3,6 +3,8 @@ pub(crate) mod cu_agent; pub(crate) mod cu_stop; #[cfg(target_os = "macos")] mod cu_ax; +#[cfg(target_os = "macos")] +mod cu_cf; pub mod cu_identity; pub mod daemon; mod quick_entry;