From f2ccb04aa438c9358f8bc8bd50fd9847e30c511c Mon Sep 17 00:00:00 2001 From: ddidderr Date: Mon, 12 Oct 2026 13:09:04 +0200 Subject: [PATCH] fix: C action buffer lengths, byte-faithful callbacks, runtime finalize Resolves the 8 gaps in docs/rewrite/notes/review-rv10-codex.md (final-Z13.md): - P1 memory safety: the C key-table action (and hovered-link/open-URL payloads) could advertise a length larger than the NUL-truncated allocation. A counted_bytes helper now passes the full bytes plus a trailing NUL with len == byte count, like Zig's backing slices; all C pointer+len structs were audited and tested to read exactly len. - ghostty-bench args follow cli/args.zig (ordered duplicates, empty value resets, first bad value errors, parseBool spellings); the C benchmark tokenizer uses the ArgIteratorGeneral port. - OSC 7 derived titles, desktop notifications and OSC 8 hover links forward raw bytes to the runtime; GTK converts only for display. - C diagnostic getter exports original bytes; escaped surrogates report Utf8CannotEncodeSurrogateHalf; config float formatters print `nan`. - Config::finalize_for(AppRuntime): GTK-only finalization (gtk-single- instance=detect) and termio's cgroup hooks apply only to the GTK runtime, so libghostty keeps `detect` like Zig. Test Plan: - cargo test --workspace --no-fail-fast (exit 0) - cargo test -p ghostty-lib --features test-platform - just check-wasm (macOS/Windows clippy re-run after the concurrent terminal changes land) --- .../src/class/application.rs | 15 ++- crates/ghostty-apprt/src/action.rs | 12 +- crates/ghostty-bench/src/args.rs | 125 +++++++++++++----- crates/ghostty-config/src/config.rs | 87 ++++++++++-- crates/ghostty-config/src/string.rs | 21 ++- crates/ghostty-config/src/types_a.rs | 9 +- crates/ghostty-config/src/types_b.rs | 11 +- crates/ghostty-core/src/surface/clipboard.rs | 6 +- crates/ghostty-core/src/surface/keyboard.rs | 4 +- crates/ghostty-core/src/surface/mod.rs | 4 +- crates/ghostty-core/src/surface/mouse.rs | 11 +- crates/ghostty-core/tests/e2e_mouse.rs | 4 +- crates/ghostty-core/tests/e2e_term.rs | 17 ++- crates/ghostty-lib/src/action_c.rs | 102 +++++++++++++- crates/ghostty-lib/src/benchmark.rs | 65 +++++---- crates/ghostty-lib/src/config_capi.rs | 61 ++++++++- crates/ghostty-lib/src/main_c.rs | 1 + crates/ghostty-termio/src/exec.rs | 23 ++-- crates/ghostty-termio/src/stream_handler.rs | 21 ++- crates/ghostty-wasm/src/config.rs | 1 + docs/rewrite/notes/final-Z13.md | 12 ++ 21 files changed, 479 insertions(+), 133 deletions(-) create mode 100644 docs/rewrite/notes/final-Z13.md diff --git a/crates/ghostty-apprt-gtk/src/class/application.rs b/crates/ghostty-apprt-gtk/src/class/application.rs index 96c2cf7..82a7e46 100644 --- a/crates/ghostty-apprt-gtk/src/class/application.rs +++ b/crates/ghostty-apprt-gtk/src/class/application.rs @@ -1286,8 +1286,9 @@ impl Application { }) } Action::MouseOverLink(v) => { + let url = v.url.to_text_lossy(); tgt.with_surface_void("mouse over link", |s| { - s.set_mouse_hover_url(if v.url.is_empty() { None } else { Some(&v.url) }); + s.set_mouse_hover_url(if url.is_empty() { None } else { Some(&url) }); }); true } @@ -1585,26 +1586,28 @@ impl Application { fn desktop_notification(&self, tgt: &Tgt, n: &action::DesktopNotification) { if let Some(s) = &tgt.surface { - s.send_desktop_notification(&n.title, &n.body); + s.send_desktop_notification(&n.title.to_text_lossy(), &n.body.to_text_lossy()); return; } // Set a default title if we don't already have one - let title = if n.title.is_empty() { + let n_title = n.title.to_text_lossy(); + let n_body = n.body.to_text_lossy(); + let title = if n_title.is_empty() { "Ghostty" } else { - n.title.as_str() + n_title.as_str() }; let notification = gio::Notification::new(title); - notification.set_body(Some(&n.body)); + notification.set_body(Some(&n_body)); notification.set_icon(&gio::ThemedIcon::new("com.mitchellh.ghostty")); notification .set_default_action_and_target_value("app.present-surface", Some(&0u64.to_variant())); // We set the notification ID to the body content. If the content is // the same, this notification may replace a previous notification. - self.send_notification(Some(&n.body), ¬ification); + self.send_notification(Some(&n_body), ¬ification); } fn goto_split(surface: &Surface, to: action::GotoSplit) -> bool { diff --git a/crates/ghostty-apprt/src/action.rs b/crates/ghostty-apprt/src/action.rs index 877fb49..c806dad 100644 --- a/crates/ghostty-apprt/src/action.rs +++ b/crates/ghostty-apprt/src/action.rs @@ -613,7 +613,8 @@ pub enum PromptTitle { #[derive(Debug, Clone, PartialEq, Eq)] pub struct MouseOverLink { - pub url: String, + /// Raw bytes (Zig `[]const u8`), in the reversible byte encoding. + pub url: ghostty_os::osbytes::EncodedOs, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -664,8 +665,10 @@ impl std::fmt::Display for Pwd { /// The desktop notification to show. #[derive(Debug, Clone, PartialEq, Eq)] pub struct DesktopNotification { - pub title: String, - pub body: String, + /// Raw bytes (Zig `[:0]const u8`), in the reversible byte encoding. + pub title: ghostty_os::osbytes::EncodedOs, + /// Raw bytes, like `title`. + pub body: ghostty_os::osbytes::EncodedOs, } impl std::fmt::Display for DesktopNotification { @@ -673,7 +676,8 @@ impl std::fmt::Display for DesktopNotification { write!( f, "DesktopNotification{{ title: {}, body: {} }}", - self.title, self.body + self.title.to_text_lossy(), + self.body.to_text_lossy() ) } } diff --git a/crates/ghostty-bench/src/args.rs b/crates/ghostty-bench/src/args.rs index 55c9d4c..8d674ba 100644 --- a/crates/ghostty-bench/src/args.rs +++ b/crates/ghostty-bench/src/args.rs @@ -38,11 +38,13 @@ impl ZigArg for f32 { } } -/// Parsed `--key[=value]` arguments. +/// Parsed `--key[=value]` arguments, in command line order. #[derive(Debug, Default)] pub struct Args { // Values are OS strings: file names (`--data=...`) need not be UTF-8. - map: HashMap>, + // Every occurrence is kept: like `cli/args.zig` each one is applied in + // order when the field is taken. + map: HashMap>>, } /// The action name of a `+action` argument, if `arg` is one. @@ -60,7 +62,7 @@ impl Args { I: IntoIterator, S: Into, { - let mut map = HashMap::new(); + let mut map: HashMap>> = HashMap::new(); for arg in iter { let bytes = os_bytes(&arg.into()); if bytes.starts_with(b"+") { @@ -78,57 +80,69 @@ impl Args { }; let key = String::from_utf8(key.to_vec()) .map_err(|_| "option names must be valid UTF-8".to_owned())?; - map.insert(key, value); + map.entry(key).or_default().push(value); } Ok(Args { map }) } - fn take_os(&mut self, key: &str) -> Result, String> { - match self.map.remove(key) { - None => Ok(None), - Some(None) => Err(format!("option --{key} requires a value")), - Some(Some(v)) => Ok(Some(v)), + /// Apply every occurrence of `key` in order (Zig `parseIntoField`): an + /// empty value resets to the default, otherwise `set` parses the value + /// (`None` = no `=value`). The first error wins. + fn apply( + &mut self, + key: &str, + default: T, + mut set: impl FnMut(Option<&OsStr>) -> Result, + ) -> Result { + let mut cur = default.clone(); + for v in self.map.remove(key).unwrap_or_default() { + cur = match v.as_deref() { + Some(v) if v.is_empty() => default.clone(), + v => set(v)?, + }; } + Ok(cur) } /// Take a typed value, falling back to `default`. - pub fn take(&mut self, key: &str, default: T) -> Result { - match self.take_os(key)? { - None => Ok(default), - Some(v) => T::parse_zig(&os_bytes(&v)) - .ok_or_else(|| format!("invalid value for --{key}: {}", v.to_string_lossy())), - } + pub fn take(&mut self, key: &str, default: T) -> Result { + self.apply(key, default, |v| { + let v = v.ok_or_else(|| format!("option --{key} requires a value"))?; + T::parse_zig(&os_bytes(v)) + .ok_or_else(|| format!("invalid value for --{key}: {}", v.to_string_lossy())) + }) } pub fn take_opt_string(&mut self, key: &str) -> Result, String> { - match self.take_os(key)? { - None => Ok(None), - Some(v) => v - .into_string() - .map(Some) - .map_err(|v| format!("invalid value for --{key}: {}", v.to_string_lossy())), - } + self.apply(key, None, |v| { + let v = v.ok_or_else(|| format!("option --{key} requires a value"))?; + v.to_str() + .map(|s| Some(s.to_owned())) + .ok_or_else(|| format!("invalid value for --{key}: {}", v.to_string_lossy())) + }) } /// Take a file path value (any bytes are allowed). pub fn take_opt_path(&mut self, key: &str) -> Result, String> { - Ok(self.take_os(key)?.map(PathBuf::from)) + self.apply(key, None, |v| { + let v = v.ok_or_else(|| format!("option --{key} requires a value"))?; + Ok(Some(PathBuf::from(v))) + }) } - /// Take a boolean flag (`--key`, `--key=true`, `--key=false`). + /// Take a boolean flag (`--key`, `--key=true`, `--key=false`; `parseBool`). pub fn take_flag(&mut self, key: &str) -> Result { - match self.map.remove(key) { - None => Ok(false), - Some(None) => Ok(true), - Some(Some(v)) => match v.to_str().unwrap_or_default() { - "true" | "t" | "1" | "yes" => Ok(true), - "false" | "f" | "0" | "no" => Ok(false), + self.apply(key, false, |v| match v.map(os_bytes) { + None => Ok(true), + Some(v) => match v.as_slice() { + b"1" | b"t" | b"T" | b"true" => Ok(true), + b"0" | b"f" | b"F" | b"false" => Ok(false), _ => Err(format!( "invalid value for --{key}: {}", - v.to_string_lossy() + String::from_utf8_lossy(&v) )), }, - } + }) } /// Error on any unconsumed option. @@ -183,6 +197,53 @@ mod tests { assert!(a.take("n", 0u16).is_err()); } + #[test] + fn duplicates_apply_in_order_and_first_error_wins() { + let a = |v: &[&str]| Args::parse(v.iter().map(OsString::from)).unwrap(); + // A bad value earlier is an error even if a later one is fine. + assert!( + a(&["--terminal-rows=bad", "--terminal-rows=10"]) + .take("terminal-rows", 80u16) + .is_err() + ); + // The last good value wins. + assert_eq!( + a(&["--terminal-rows=5", "--terminal-rows=10"]) + .take("terminal-rows", 80u16) + .unwrap(), + 10 + ); + // An empty value resets to the default. + assert_eq!( + a(&["--terminal-rows=5", "--terminal-rows="]) + .take("terminal-rows", 80u16) + .unwrap(), + 80 + ); + assert_eq!( + a(&["--terminal-rows="]) + .take("terminal-rows", 80u16) + .unwrap(), + 80 + ); + // Missing value is still an error. + assert!( + a(&["--terminal-rows"]) + .take("terminal-rows", 80u16) + .is_err() + ); + // Optional path: empty resets to unset. + assert_eq!( + a(&["--data=/x", "--data="]).take_opt_path("data").unwrap(), + None + ); + assert_eq!(a(&["--data="]).take_opt_path("data").unwrap(), None); + // Flags: empty resets, parseBool spellings only. + assert!(!a(&["--time", "--time="]).take_flag("time").unwrap()); + assert!(a(&["--time=T"]).take_flag("time").unwrap()); + assert!(a(&["--time=yes"]).take_flag("time").is_err()); + } + #[test] fn data_path_keeps_non_utf8_bytes() { let arg = OsString::from_vec(b"--data=/tmp/a\xffb".to_vec()); diff --git a/crates/ghostty-config/src/config.rs b/crates/ghostty-config/src/config.rs index 01a52c3..17595db 100644 --- a/crates/ghostty-config/src/config.rs +++ b/crates/ghostty-config/src/config.rs @@ -3818,6 +3818,47 @@ fn shell_from_env(value: &std::ffi::OsStr) -> Command { Command::Shell(ghostty_os::osbytes::EncodedOs::from_os(value)) } +/// The application runtime (Zig `build_config.app_runtime`). It only +/// affects finalization (GTK-only defaults). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AppRuntime { + /// No apprt: the embedded library (and wasm). + None, + /// GTK, the default on Linux and BSD. + Gtk, +} + +const RUNTIME_DEFAULT: u8 = if cfg!(all( + unix, + not(target_vendor = "apple"), + not(target_family = "wasm") +)) { + 1 +} else { + 0 +}; + +static APP_RUNTIME: std::sync::atomic::AtomicU8 = std::sync::atomic::AtomicU8::new(RUNTIME_DEFAULT); + +/// The process-wide runtime used by [`Config::finalize`] (and by the +/// load/reload paths which finalize internally). +#[must_use] +pub fn app_runtime() -> AppRuntime { + match APP_RUNTIME.load(std::sync::atomic::Ordering::Relaxed) { + 1 => AppRuntime::Gtk, + _ => AppRuntime::None, + } +} + +/// Select the process-wide runtime. The embedded library selects +/// [`AppRuntime::None`] so `gtk-single-instance = detect` stays `detect`. +pub fn set_app_runtime(runtime: AppRuntime) { + APP_RUNTIME.store( + u8::from(runtime == AppRuntime::Gtk), + std::sync::atomic::Ordering::Relaxed, + ); +} + impl Config { /// The value of the config key in the form the C API exposes it (Zig: /// `c_get.get`). `None` if there is no such key. @@ -4478,6 +4519,12 @@ impl Config { /// Call this once after you are done setting configuration. This /// is idempotent but will waste memory if called multiple times. pub fn finalize(&mut self) -> Result<(), Error> { + self.finalize_for(app_runtime()) + } + + /// [`Config::finalize`] for an explicit application runtime (Zig's + /// `build_config.app_runtime`, a build option there). + pub fn finalize_for(&mut self, runtime: AppRuntime) -> Result<(), Error> { // We always load the theme first because it may set other fields // in our config. if let Some(theme) = self.theme.clone() { @@ -4543,19 +4590,22 @@ impl Config { wd.finalize(); self.working_directory = Some(wd); - // Apprt-specific defaults (GTK) - match self.gtk_single_instance { - GtkSingleInstance::True | GtkSingleInstance::False => {} + // Apprt-specific defaults + match runtime { + AppRuntime::None => {} + AppRuntime::Gtk => match self.gtk_single_instance { + GtkSingleInstance::True | GtkSingleInstance::False => {} - // For detection, we assume single instance unless we're - // in a CLI environment, then we disable single instance. - GtkSingleInstance::Detect => { - self.gtk_single_instance = if probable_cli { - GtkSingleInstance::False - } else { - GtkSingleInstance::True - }; - } + // For detection, we assume single instance unless we're + // in a CLI environment, then we disable single instance. + GtkSingleInstance::Detect => { + self.gtk_single_instance = if probable_cli { + GtkSingleInstance::False + } else { + GtkSingleInstance::True + }; + } + }, } // Default our click interval @@ -5433,6 +5483,19 @@ mod tests { assert!(cfg.conditional_set.contains(&conditional::Key::Theme)); } + #[test] + fn gtk_single_instance_detect_only_finalizes_for_gtk() { + let mut cfg = Config::default(); + cfg.load_iter(&mut it(&["--gtk-single-instance=detect"])) + .unwrap(); + let mut none = cfg.clone(); + none.finalize_for(AppRuntime::None).unwrap(); + assert_eq!(none.gtk_single_instance, GtkSingleInstance::Detect); + let mut gtk = cfg; + gtk.finalize_for(AppRuntime::Gtk).unwrap(); + assert_ne!(gtk.gtk_single_instance, GtkSingleInstance::Detect); + } + #[test] fn compatibility_gtk_single_instance_desktop() { let mut cfg = Config::default(); diff --git a/crates/ghostty-config/src/string.rs b/crates/ghostty-config/src/string.rs index eb646a6..26bb935 100644 --- a/crates/ghostty-config/src/string.rs +++ b/crates/ghostty-config/src/string.rs @@ -4,11 +4,16 @@ #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum StringError { InvalidString, + /// `std.unicode.utf8Encode` of an escaped surrogate half. + Utf8CannotEncodeSurrogateHalf, } impl std::fmt::Display for StringError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str("InvalidString") + f.write_str(match self { + StringError::InvalidString => "InvalidString", + StringError::Utf8CannotEncodeSurrogateHalf => "Utf8CannotEncodeSurrogateHalf", + }) } } @@ -88,7 +93,11 @@ pub fn parse_bytes(input: impl AsRef<[u8]>) -> Result, StringError> { continue; } let cp = parse_escape_sequence(bytes, &mut i).ok_or(StringError::InvalidString)?; - let ch = char::from_u32(cp).ok_or(StringError::InvalidString)?; + let ch = char::from_u32(cp).ok_or(if (0xd800..=0xdfff).contains(&cp) { + StringError::Utf8CannotEncodeSurrogateHalf + } else { + StringError::InvalidString + })?; out.extend_from_slice(ch.encode_utf8(&mut [0; 4]).as_bytes()); } Ok(out) @@ -200,6 +209,14 @@ mod tests { #[test] fn parse_invalid() { + assert_eq!( + parse("\\u{d800}"), + Err(StringError::Utf8CannotEncodeSurrogateHalf) + ); + assert_eq!( + crate::cli_args::ArgError::from(StringError::Utf8CannotEncodeSurrogateHalf), + crate::cli_args::ArgError::other("Utf8CannotEncodeSurrogateHalf") + ); assert_eq!(parse("\\q"), Err(StringError::InvalidString)); assert_eq!(parse("\\"), Err(StringError::InvalidString)); assert_eq!(parse("\\u{}"), Err(StringError::InvalidString)); diff --git a/crates/ghostty-config/src/types_a.rs b/crates/ghostty-config/src/types_a.rs index db28bcd..0e63b8b 100644 --- a/crates/ghostty-config/src/types_a.rs +++ b/crates/ghostty-config/src/types_a.rs @@ -738,7 +738,7 @@ impl ConfigField for RepeatableFontVariation { &format!( "{}={}", ghostty_os::osbytes::bytes_to_config_string(&id), - v.value + ghostty_os::zig_fmt::format_float_d(v.value) ), )?; } @@ -1508,6 +1508,13 @@ mod tests { assert_eq!(list.list, vec![v(b"wght", 200.0), v(b"slnt", -15.0)]); } + #[test] + fn format_nan_like_zig() { + let mut list = RepeatableFontVariation::default(); + parse(&mut list, "wght=NaN").unwrap(); + assert_eq!(fe(&list), "a = wght=nan\n"); + } + #[test] fn format_config_single() { let mut list = RepeatableFontVariation::default(); diff --git a/crates/ghostty-config/src/types_b.rs b/crates/ghostty-config/src/types_b.rs index 70c0301..16d3dd7 100644 --- a/crates/ghostty-config/src/types_b.rs +++ b/crates/ghostty-config/src/types_b.rs @@ -568,7 +568,9 @@ impl QuickTerminalSizeValue { fn format(self, out: &mut String) { match self { - Self::Percentage(v) => out.push_str(&format!("{v}%")), + Self::Percentage(v) => { + out.push_str(&format!("{}%", ghostty_os::zig_fmt::format_float_d(v))); + } Self::Pixels(v) => out.push_str(&format!("{v}px")), } } @@ -1287,6 +1289,13 @@ config_packed! { #[cfg(test)] #[allow(clippy::unwrap_used, clippy::approx_constant)] mod tests { + #[test] + fn quick_terminal_size_value_formats_nan_like_zig() { + let mut out = String::new(); + QuickTerminalSizeValue::Percentage(f32::NAN).format(&mut out); + assert_eq!(out, "nan%"); + } + use super::*; use crate::formatter::format_entry; diff --git a/crates/ghostty-core/src/surface/clipboard.rs b/crates/ghostty-core/src/surface/clipboard.rs index b6a396d..d8db4d9 100644 --- a/crates/ghostty-core/src/surface/clipboard.rs +++ b/crates/ghostty-core/src/surface/clipboard.rs @@ -358,7 +358,7 @@ impl Surface { /// Show a desktop notification, rate limited (Zig /// `showDesktopNotification`). - pub(crate) fn show_desktop_notification(&self, title: &str, body: &str) { + pub(crate) fn show_desktop_notification(&self, title: &[u8], body: &[u8]) { let now = Instant::now(); // Set a limit of one desktop notification per second so that the OS @@ -391,8 +391,8 @@ impl Surface { self.app.last_notification_time.set(Some(now)); self.app.last_notification_digest.set(new_digest); self.perform(RtAction::DesktopNotification(DesktopNotification { - title: title.to_owned(), - body: body.to_owned(), + title: ghostty_os::osbytes::EncodedOs::from_bytes(title), + body: ghostty_os::osbytes::EncodedOs::from_bytes(body), })); } diff --git a/crates/ghostty-core/src/surface/keyboard.rs b/crates/ghostty-core/src/surface/keyboard.rs index 74352d0..5024def 100644 --- a/crates/ghostty-core/src/surface/keyboard.rs +++ b/crates/ghostty-core/src/surface/keyboard.rs @@ -514,7 +514,9 @@ impl Surface { // we reset state self.perform(RtAction::MouseShape(mouse_shape)); self.perform(RtAction::MouseOverLink( - ghostty_apprt::action::MouseOverLink { url: String::new() }, + ghostty_apprt::action::MouseOverLink { + url: ghostty_os::osbytes::EncodedOs::new(), + }, )); self.queue_render(); } diff --git a/crates/ghostty-core/src/surface/mod.rs b/crates/ghostty-core/src/surface/mod.rs index 2b52358..d554f56 100644 --- a/crates/ghostty-core/src/surface/mod.rs +++ b/crates/ghostty-core/src/surface/mod.rs @@ -960,9 +960,7 @@ impl Surface { return Ok(()); } - let title = String::from_utf8_lossy(notification.title()).into_owned(); - let body = String::from_utf8_lossy(notification.body()).into_owned(); - self.show_desktop_notification(&title, &body); + self.show_desktop_notification(notification.title(), notification.body()); } Message::RendererHealth(health) => self.update_renderer_health(health), diff --git a/crates/ghostty-core/src/surface/mouse.rs b/crates/ghostty-core/src/surface/mouse.rs index acd2365..1fb23c5 100644 --- a/crates/ghostty-core/src/surface/mouse.rs +++ b/crates/ghostty-core/src/surface/mouse.rs @@ -1801,7 +1801,9 @@ impl Surface { trim: false, }); ( - Some(MouseOverLink { url: str }), + Some(MouseOverLink { + url: ghostty_os::osbytes::EncodedOs::from_plain_text(&str), + }), cfg.link_previews == LinkPreviews::True, ) } @@ -1814,9 +1816,8 @@ impl Surface { break 'link (None, false); }; ( - // Display-only text for the status bar. Some(MouseOverLink { - url: String::from_utf8_lossy(&uri).into_owned(), + url: ghostty_os::osbytes::EncodedOs::from_bytes(&uri), }), cfg.link_previews != LinkPreviews::False, ) @@ -1848,7 +1849,7 @@ impl Surface { out.actions .push(RtAction::MouseShape(st.terminal.mouse_shape)); out.actions.push(RtAction::MouseOverLink(MouseOverLink { - url: String::new(), + url: ghostty_os::osbytes::EncodedOs::new(), })); out.render = true; } @@ -1930,7 +1931,7 @@ impl Surface { let shape = self.lock_terminal().mouse_shape; self.perform(RtAction::MouseShape(shape)); self.perform(RtAction::MouseOverLink(MouseOverLink { - url: String::new(), + url: ghostty_os::osbytes::EncodedOs::new(), })); self.queue_render(); } diff --git a/crates/ghostty-core/tests/e2e_mouse.rs b/crates/ghostty-core/tests/e2e_mouse.rs index 2342b5f..06160bc 100644 --- a/crates/ghostty-core/tests/e2e_mouse.rs +++ b/crates/ghostty-core/tests/e2e_mouse.rs @@ -302,7 +302,7 @@ fn osc8_link_hover_and_ctrl_click_opens() { // hover with ctrl over "here" (cols 3..6) h.move_to(4, 0, Some(ctrl())); let url = h.wait_action("mouse over link", |a| match a { - Action::MouseOverLink(l) if !l.url.is_empty() => Some(l.url.clone()), + Action::MouseOverLink(l) if !l.url.is_empty() => Some(l.url.to_text_lossy()), _ => None, }); assert_eq!(url, "http://osc8.example/x"); @@ -324,7 +324,7 @@ fn regex_url_ctrl_click_opens_and_plain_click_does_not() { h.move_to(30, 2, Some(none())); h.move_to(20, 0, Some(ctrl())); h.wait_action("hover", |a| match a { - Action::MouseOverLink(l) if l.url.contains("ghostty.org") => Some(()), + Action::MouseOverLink(l) if l.url.to_text_lossy().contains("ghostty.org") => Some(()), _ => None, }); h.click(20, 0, L, ctrl()); diff --git a/crates/ghostty-core/tests/e2e_term.rs b/crates/ghostty-core/tests/e2e_term.rs index 2db46c8..7952405 100644 --- a/crates/ghostty-core/tests/e2e_term.rs +++ b/crates/ghostty-core/tests/e2e_term.rs @@ -229,11 +229,24 @@ fn osc52_read_allow_and_deny() { assert!(h.rt.confirmations.borrow().is_empty()); } +#[test] +fn desktop_notification_keeps_invalid_utf8_bytes() { + let h = harness!(Harness::recorder(r"\033]777;notify;T\377;B\377\007", &[])); + let (t, b) = h.wait_action("osc777", |a| match a { + Action::DesktopNotification(n) => Some((n.title.to_bytes(), n.body.to_bytes())), + _ => None, + }); + assert_eq!( + (t.as_slice(), b.as_slice()), + (b"T\xff".as_slice(), b"B\xff".as_slice()) + ); +} + #[test] fn desktop_notifications() { let h = harness!(Harness::recorder(r"\033]9;just a body\007", &[])); let (t, b) = h.wait_action("osc9", |a| match a { - Action::DesktopNotification(n) => Some((n.title.clone(), n.body.clone())), + Action::DesktopNotification(n) => Some((n.title.to_text_lossy(), n.body.to_text_lossy())), _ => None, }); assert_eq!(b, "just a body"); @@ -244,7 +257,7 @@ fn desktop_notifications() { &[] )); let (t, b) = h.wait_action("osc777", |a| match a { - Action::DesktopNotification(n) => Some((n.title.clone(), n.body.clone())), + Action::DesktopNotification(n) => Some((n.title.to_text_lossy(), n.body.to_text_lossy())), _ => None, }); assert_eq!((t.as_str(), b.as_str()), ("My Title", "My Body")); diff --git a/crates/ghostty-lib/src/action_c.rs b/crates/ghostty-lib/src/action_c.rs index 86df4b8..e6b5f7b 100644 --- a/crates/ghostty-lib/src/action_c.rs +++ b/crates/ghostty-lib/src/action_c.rs @@ -30,6 +30,7 @@ use crate::{ #[derive(Default)] pub struct Keep { strings: Vec, + counted: Vec>, config: Option, } @@ -48,6 +49,20 @@ impl Keep { self.strings.push(c); ptr } + + /// The complete `s` (interior NULs included) followed by a NUL, for + /// pointer + length pairs: the advertised length is always `s.len()` and + /// the allocation holds at least that many bytes (Zig passes the whole + /// backing slice). + fn counted_bytes(&mut self, s: &[u8]) -> *const c_char { + let mut v = Vec::with_capacity(s.len() + 1); + v.extend_from_slice(s); + v.push(0); + let b = v.into_boxed_slice(); + let ptr = b.as_ptr().cast::(); + self.counted.push(b); + ptr + } } impl Drop for Keep { @@ -123,8 +138,8 @@ pub fn action_to_c(action: &Action, keep: &mut Keep) -> ghostty_action_s { } Action::Inspector(i) => v.inspector = *i as c_int, Action::DesktopNotification(n) => { - v.desktop_notification.title = keep.cstr(&n.title); - v.desktop_notification.body = keep.cstr(&n.body); + v.desktop_notification.title = keep.cstr_bytes(&n.title.to_bytes()); + v.desktop_notification.body = keep.cstr_bytes(&n.body.to_bytes()); } Action::SetTitle(t) => v.set_title.title = keep.cstr_bytes(&t.title.to_bytes()), Action::SetTabTitle(t) => v.set_tab_title.title = keep.cstr_bytes(&t.title.to_bytes()), @@ -135,8 +150,9 @@ pub fn action_to_c(action: &Action, keep: &mut Keep) -> ghostty_action_s { Action::MouseShape(s) => v.mouse_shape = *s as c_int, Action::MouseVisibility(m) => v.mouse_visibility = *m as c_int, Action::MouseOverLink(l) => { - v.mouse_over_link.url = keep.cstr(&l.url); - v.mouse_over_link.len = l.url.len(); + let url = l.url.to_bytes(); + v.mouse_over_link.url = keep.counted_bytes(&url); + v.mouse_over_link.len = url.len(); } Action::RendererHealth(h) => { v.renderer_health = match h { @@ -158,7 +174,7 @@ pub fn action_to_c(action: &Action, keep: &mut Keep) -> ghostty_action_s { Action::KeyTable(KeyTable::Activate(name)) => { v.key_table.tag = 0; let bytes = name.to_bytes(); - v.key_table.value.activate.name = keep.cstr_bytes(&bytes); + v.key_table.value.activate.name = keep.counted_bytes(&bytes); v.key_table.value.activate.len = bytes.len(); } Action::KeyTable(KeyTable::Deactivate) => v.key_table.tag = 1, @@ -186,7 +202,7 @@ pub fn action_to_c(action: &Action, keep: &mut Keep) -> ghostty_action_s { // The URL may be the escaped form of a non-UTF-8 path; hand the // embedder the real bytes. let url = u.url.to_bytes(); - v.open_url.url = keep.cstr_bytes(&url); + v.open_url.url = keep.counted_bytes(&url); v.open_url.len = url.len(); } Action::ShowChildExited(c) => { @@ -293,6 +309,78 @@ mod tests { assert_eq!(name, raw); } + #[test] + fn counted_buffers_hold_exactly_len_bytes() { + use ghostty_os::osbytes::EncodedOs; + let mut keep = Keep::default(); + let raw = b"a\0b"; + let c = action_to_c( + &Action::KeyTable(KeyTable::Activate(EncodedOs::from_bytes(raw))), + &mut keep, + ); + // SAFETY: tag is key_table.activate; keep owns len + 1 bytes. + let (p, n) = unsafe { + ( + c.action.key_table.value.activate.name.cast::(), + c.action.key_table.value.activate.len, + ) + }; + assert_eq!(n, 3); + // SAFETY: the allocation holds `n` bytes plus a NUL. + assert_eq!(unsafe { std::slice::from_raw_parts(p, n + 1) }, b"a\0b\0"); + + let c = action_to_c( + &Action::MouseOverLink(MouseOverLink { + url: EncodedOs::from_bytes(b"u\0v"), + }), + &mut keep, + ); + // SAFETY: tag is mouse_over_link. + let (p, n) = unsafe { + ( + c.action.mouse_over_link.url.cast::(), + c.action.mouse_over_link.len, + ) + }; + assert_eq!(n, 3); + // SAFETY: as above. + assert_eq!(unsafe { std::slice::from_raw_parts(p, n + 1) }, b"u\0v\0"); + + let c = action_to_c( + &Action::OpenUrl(ghostty_apprt::action::OpenUrl { + kind: ghostty_apprt::action::OpenUrlKind::default(), + url: EncodedOs::from_bytes(b"x\0y"), + }), + &mut keep, + ); + // SAFETY: tag is open_url. + let (p, n) = unsafe { (c.action.open_url.url.cast::(), c.action.open_url.len) }; + assert_eq!(n, 3); + // SAFETY: as above. + assert_eq!(unsafe { std::slice::from_raw_parts(p, n + 1) }, b"x\0y\0"); + } + + #[test] + fn notification_forwards_raw_bytes() { + use ghostty_os::osbytes::EncodedOs; + let mut keep = Keep::default(); + let c = action_to_c( + &Action::DesktopNotification(ghostty_apprt::action::DesktopNotification { + title: EncodedOs::from_bytes(b"t\xff"), + body: EncodedOs::from_bytes(b"b\xff"), + }), + &mut keep, + ); + // SAFETY: tag is desktop_notification; keep owns the strings. + let (t, b) = unsafe { + ( + std::ffi::CStr::from_ptr(c.action.desktop_notification.title), + std::ffi::CStr::from_ptr(c.action.desktop_notification.body), + ) + }; + assert_eq!((t.to_bytes(), b.to_bytes()), (&b"t\xff"[..], &b"b\xff"[..])); + } + #[test] fn target_cval() { let t = target_to_c(&Target::App); @@ -380,7 +468,7 @@ mod tests { let a = action_to_c( &Action::MouseOverLink(MouseOverLink { - url: "https://x".into(), + url: ghostty_os::osbytes::EncodedOs::from_bytes(b"https://x"), }), &mut keep, ); diff --git a/crates/ghostty-lib/src/benchmark.rs b/crates/ghostty-lib/src/benchmark.rs index 14795a5..555e3f2 100644 --- a/crates/ghostty-lib/src/benchmark.rs +++ b/crates/ghostty-lib/src/benchmark.rs @@ -5,42 +5,16 @@ use std::ffi::{CStr, c_char}; use ghostty_bench::{args::Args, benches}; -/// Split the argument string like `std.process.ArgIteratorGeneral(.{})`: -/// whitespace separated, with double quotes grouping and backslash -/// escapes inside quotes. Works on raw bytes: file names need not be UTF-8. +/// Split the argument string like `std.process.ArgIteratorGeneral(.{})` +/// (backslashes are only special before a double quote). Works on raw bytes: +/// file names need not be UTF-8. fn split_args(s: &[u8]) -> Vec { - let mut out = Vec::new(); - let mut cur: Vec = Vec::new(); - let mut have = false; - let mut in_quote = false; - let mut bytes = s.iter().copied(); - while let Some(c) = bytes.next() { - match c { - b'"' => { - in_quote = !in_quote; - have = true; - } - b'\\' if in_quote => { - if let Some(n) = bytes.next() { - cur.push(n); - } - } - c if c.is_ascii_whitespace() && !in_quote => { - if have { - out.push(ghostty_os::osbytes::os_from_bytes(std::mem::take(&mut cur))); - have = false; - } - } - c => { - cur.push(c); - have = true; - } - } - } - if have { - out.push(ghostty_os::osbytes::os_from_bytes(cur)); - } - out + ghostty_os::arg_iterator::ArgIteratorGeneral::new( + ghostty_os::arg_iterator::Options::default(), + s, + ) + .map(ghostty_os::osbytes::os_from_bytes) + .collect() } /// Run the Ghostty benchmark CLI with the given action and arguments. @@ -90,6 +64,27 @@ mod tests { assert_eq!(split_args(b" ").len(), 0); } + #[test] + fn split_args_backslashes_are_literal_except_before_quotes() { + assert_eq!( + split_args(br#"--terminal-rows="1\0""#), + [r"--terminal-rows=1\0"] + ); + assert_eq!(split_args(b"a\\ b"), [r"a\", "b"]); + assert_eq!(split_args(br#""a\"b""#), ["a\"b"]); + } + + #[test] + fn backslash_zero_row_count_fails_like_zig() { + // SAFETY: valid NUL terminated strings. + assert!(!unsafe { + ghostty_benchmark_cli( + c"terminal-stream".as_ptr(), + c"--terminal-rows=\"1\\0\"".as_ptr(), + ) + }); + } + #[cfg(unix)] #[test] fn split_args_keeps_non_utf8_bytes() { diff --git a/crates/ghostty-lib/src/config_capi.rs b/crates/ghostty-lib/src/config_capi.rs index 6deb96a..4e1e3ae 100644 --- a/crates/ghostty-lib/src/config_capi.rs +++ b/crates/ghostty-lib/src/config_capi.rs @@ -67,6 +67,8 @@ struct GetCache { impl CConfig { pub fn new(config: Config) -> CConfig { + // The embedded library has no apprt (Zig `app_runtime = .none`). + ghostty_config::config::set_app_runtime(ghostty_config::config::AppRuntime::None); CConfig { config, cache: RefCell::default(), @@ -302,7 +304,10 @@ pub unsafe extern "C" fn ghostty_config_finalize(ptr: ghostty_config_t) { let Some(c) = (unsafe { cconfig(ptr) }) else { return; }; - if let Err(err) = c.config.finalize() { + if let Err(err) = c + .config + .finalize_for(ghostty_config::config::AppRuntime::None) + { log::error!("error finalizing config err={err}"); } } @@ -575,7 +580,7 @@ pub unsafe extern "C" fn ghostty_config_get_diagnostic( while msgs.len() < items.len() { let mut text = String::new(); let _ = items[msgs.len()].format(&mut text); - msgs.push(cstring(&text)); + msgs.push(cstring_value(&text)); } ghostty_diagnostic_s { message: msgs[idx].as_ptr(), @@ -774,3 +779,55 @@ mod tests { unsafe { ghostty_config_free(h) }; } } + +#[cfg(all(test, unix))] +mod runtime_and_diag_tests { + use super::*; + + #[test] + fn embedded_config_keeps_gtk_single_instance_detect() { + let h = ghostty_config_new(); + // SAFETY: just created. + let c = unsafe { cconfig(h) }.unwrap(); + c.config.gtk_single_instance = ghostty_config::types_b::GtkSingleInstance::Detect; + // SAFETY: valid handle. + unsafe { ghostty_config_finalize(h) }; + let mut out: *const c_char = std::ptr::null(); + let key = "gtk-single-instance"; + // SAFETY: the key's C type is a C string. + assert!(unsafe { + ghostty_config_get(h, (&raw mut out).cast(), key.as_ptr().cast(), key.len()) + }); + // SAFETY: valid NUL terminated string owned by the config. + assert_eq!(unsafe { CStr::from_ptr(out) }.to_bytes(), b"detect"); + // SAFETY: valid handle. + unsafe { ghostty_config_free(h) }; + } + + #[test] + fn diagnostic_getter_exports_original_bytes() { + use std::os::unix::ffi::OsStrExt; + let dir = std::env::temp_dir().join(format!("ghostty-z13-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let file = dir.join("cfg"); + std::fs::write(&file, b"font-size = \xff\n").unwrap(); + let path = CString::new(file.as_os_str().as_bytes()).unwrap(); + let h = ghostty_config_new(); + // SAFETY: valid handle and NUL terminated path. + unsafe { ghostty_config_load_file(h, path.as_ptr()) }; + // SAFETY: valid handle. + assert!(unsafe { ghostty_config_diagnostics_count(h) } >= 1); + // SAFETY: valid handle, index in range. + let d = unsafe { ghostty_config_get_diagnostic(h, 0) }; + // SAFETY: NUL terminated string owned by the config. + let msg = unsafe { CStr::from_ptr(d.message) }.to_bytes().to_vec(); + assert!(msg.windows(1).any(|w| w == [0xff]), "{msg:?}"); + assert!( + !msg.windows(5).any(|w| w == [0xef, 0xb7, 0x90, 0xc3, 0xbf]), + "{msg:?}" + ); + // SAFETY: valid handle. + unsafe { ghostty_config_free(h) }; + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/ghostty-lib/src/main_c.rs b/crates/ghostty-lib/src/main_c.rs index a201c1a..6f85f7d 100644 --- a/crates/ghostty-lib/src/main_c.rs +++ b/crates/ghostty-lib/src/main_c.rs @@ -84,6 +84,7 @@ pub(crate) fn alloc_c_text(bytes: &[u8]) -> *const c_char { /// `argv` must be null or point to `argc` valid NUL terminated strings. #[unsafe(no_mangle)] pub unsafe extern "C" fn ghostty_init(argc: usize, argv: *mut *mut c_char) -> c_int { + ghostty_config::config::set_app_runtime(ghostty_config::config::AppRuntime::None); // Raw argument bytes (Zig: `std.os.argv = argv[0..argc]`). let mut raw: Vec = Vec::with_capacity(argc); if !argv.is_null() { diff --git a/crates/ghostty-termio/src/exec.rs b/crates/ghostty-termio/src/exec.rs index 8166bb4..341f5e0 100644 --- a/crates/ghostty-termio/src/exec.rs +++ b/crates/ghostty-termio/src/exec.rs @@ -1066,6 +1066,7 @@ impl Subprocess { let pre_info = self.rt_pre_exec_info; let post_info = self.rt_post_fork_info; let create_scope = self.create_scope.clone(); + let gtk = ghostty_config::config::app_runtime() == ghostty_config::config::AppRuntime::Gtk; let mut cmd = Command { path: self.args[0].clone(), args: self.args.clone(), @@ -1081,15 +1082,19 @@ impl Subprocess { let _ = pty::child_pre_exec(master_raw, slave_raw); None })), - rt_pre_exec: Some(Box::new(move || linux_cgroup_pre_exec(&pre_info))), - rt_post_fork: Some(Box::new(move |c: &Command| { - let create = |pid, mem, tasks| match &create_scope { - Some(f) => f(pid, mem, tasks), - None => None, - }; - linux_cgroup_post_fork(c, &post_info, create) - .map_err(|_: PostForkError| PostForkError) - })), + // Zig installs these only when the apprt declares them + // (`@hasDecl(apprt.runtime, "pre_exec")`): GTK. + rt_pre_exec: gtk.then(|| Box::new(move || linux_cgroup_pre_exec(&pre_info)) as Box<_>), + rt_post_fork: gtk.then(|| { + Box::new(move |c: &Command| { + let create = |pid, mem, tasks| match &create_scope { + Some(f) => f(pid, mem, tasks), + None => None, + }; + linux_cgroup_post_fork(c, &post_info, create) + .map_err(|_: PostForkError| PostForkError) + }) as Box<_> + }), pid: None, }; diff --git a/crates/ghostty-termio/src/stream_handler.rs b/crates/ghostty-termio/src/stream_handler.rs index 37524c0..28e8844 100644 --- a/crates/ghostty-termio/src/stream_handler.rs +++ b/crates/ghostty-termio/src/stream_handler.rs @@ -839,9 +839,8 @@ impl StreamHandler<'_> { //------------------------------------------------------------------------- // OSC - fn window_title(&mut self, title: &str) { + fn window_title(&mut self, title: &[u8]) { let mut buf = [0u8; SET_TITLE_LEN]; - let title = title.as_bytes(); if title.len() >= buf.len() { log::warn!("change title requested larger than our buffer size, ignoring"); return; @@ -949,7 +948,7 @@ impl StreamHandler<'_> { // If we haven't seen a title, we're using the pwd as our title. // Set it to blank which will reset our title behavior. if !self.st.seen_title { - self.window_title(""); + self.window_title(b""); debug_assert!(!self.st.seen_title); } @@ -1015,8 +1014,7 @@ impl StreamHandler<'_> { // If we haven't seen a title, use our pwd as the title. if !self.st.seen_title { - // Titles are text; only the title is lossy. - self.window_title(&String::from_utf8_lossy(&path)); + self.window_title(&path); self.st.seen_title = false; } Ok(()) @@ -1501,7 +1499,7 @@ impl stream::Handler for StreamHandler<'_> { Action::EndHyperlink => self.terminal.screen_mut().end_hyperlink(), Action::ActiveStatusDisplay(v) => self.terminal.status_display = v, Action::Decaln => self.terminal.decaln()?, - Action::WindowTitle(v) => self.window_title(v.title), + Action::WindowTitle(v) => self.window_title(v.title.as_bytes()), Action::ReportPwd(v) => self.report_pwd(v.url)?, Action::ShowDesktopNotification(v) => self.show_desktop_notification(v.title, v.body), Action::ProgressReport(v) => self.progress_report(v), @@ -1984,6 +1982,17 @@ mod tests { assert!(!h.stream.state.seen_title); } + #[test] + fn report_pwd_title_keeps_raw_bytes() { + let mut h = Harness::new(); + h.feed(b"\x1b]7;file://localhost/tmp/%FF\x1b\\"); + let msgs = h.surface_msgs(); + assert!( + matches!(msgs.last(), Some(SurfaceMessage::SetTitle(t)) if t.starts_with(b"/tmp/\xff\0")), + "{msgs:?}" + ); + } + #[test] fn report_pwd_remote_host_ignored_and_reset() { let mut h = Harness::new(); diff --git a/crates/ghostty-wasm/src/config.rs b/crates/ghostty-wasm/src/config.rs index 82f05d1..f3d248f 100644 --- a/crates/ghostty-wasm/src/config.rs +++ b/crates/ghostty-wasm/src/config.rs @@ -14,6 +14,7 @@ use crate::{ensure_init, host_bytes}; #[unsafe(no_mangle)] pub extern "C" fn config_new() -> *mut Config { ensure_init(); + ghostty_config::config::set_app_runtime(ghostty_config::config::AppRuntime::None); Box::into_raw(Box::new(Config::new_default())) } diff --git a/docs/rewrite/notes/final-Z13.md b/docs/rewrite/notes/final-Z13.md new file mode 100644 index 0000000..830e9ee --- /dev/null +++ b/docs/rewrite/notes/final-Z13.md @@ -0,0 +1,12 @@ +# Z13: RV10 codex gaps + +1. action_c.rs: key-table name, mouse-over-link and open-url use `Keep::counted_bytes` (full bytes + NUL, len always equals the allocation). Audited config_capi/embedded: other pointer+len pairs already match. Test `counted_buffers_hold_exactly_len_bytes`. +2. bench/args.rs keeps every occurrence in order (empty resets default, first error wins, parseBool spellings); lib/benchmark.rs uses `ghostty_os::arg_iterator::ArgIteratorGeneral`. +3. termio `window_title` takes bytes; OSC 7 forwards the raw path. +4. apprt `DesktopNotification`/`MouseOverLink` carry `EncodedOs`; core passes raw bytes; C actions decode them. GTK uses lossy text at the UI boundary. +5. Diagnostic getter decodes the internal encoding (`cstring_value`). +6. `StringError::Utf8CannotEncodeSurrogateHalf`. +7. NaN: both formatters use `format_float_d`. +8. `AppRuntime` + `Config::finalize_for`; process-wide `app_runtime()` (default Gtk on Linux/BSD, None elsewhere); embedded lib / wasm set None. termio installs the cgroup pre-exec/post-fork hooks only for Gtk (Zig `@hasDecl(apprt.runtime, "pre_exec")`). + +Gate: workspace tests pass; clippy clean except ghostty-terminal/difftest (DF in-progress edits), which also stops `just check-macos`/`check-windows` at ghostty-terminal; check-wasm passes.