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)
This commit is contained in:
ddidderr committed
1 parent
7b3fffded3
commit
da9599140b
21 files changed
+479
-133
No files matched your search
@@ -30,6 +30,7 @@ use crate::{
|
||||
#[derive(Default)]
|
||||
pub struct Keep {
|
||||
strings: Vec<CString>,
|
||||
counted: Vec<Box<[u8]>>,
|
||||
config: Option<ghostty_config_t>,
|
||||
}
|
||||
|
||||
@@ -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::<c_char>();
|
||||
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::<u8>(),
|
||||
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::<u8>(),
|
||||
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::<u8>(), 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,
|
||||
);
|
||||
|
||||
@@ -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<std::ffi::OsString> {
|
||||
let mut out = Vec::new();
|
||||
let mut cur: Vec<u8> = 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() {
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
@@ -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<OsString> = Vec::with_capacity(argc);
|
||||
if !argv.is_null() {
|
||||
|
||||
Reference in new issue
Block a user