diff --git a/crates/ghostty-apprt-gtk/src/ipc/new_window.rs b/crates/ghostty-apprt-gtk/src/ipc/new_window.rs index a064613..a51b750 100644 --- a/crates/ghostty-apprt-gtk/src/ipc/new_window.rs +++ b/crates/ghostty-apprt-gtk/src/ipc/new_window.rs @@ -21,6 +21,12 @@ use gtk::glib; use super::dbus::DBus; pub fn new_window(target: &Target, value: &NewWindow) -> Result { + // Validate before touching the bus (deviant fix, see `arguments_variant`). + let command = value + .arguments + .as_deref() + .map(arguments_variant) + .transpose()?; let mut dbus = DBus::new( target, if value.arguments.is_none() { @@ -30,31 +36,38 @@ pub fn new_window(target: &Target, value: &NewWindow) -> Result { }, )?; - if let Some(arguments) = &value.arguments { + if let Some(command) = &command { // If any arguments were specified on the command line, the first // parameter is an array of strings that contain the arguments. They // will be sent to the main Ghostty instance and interpreted as CLI // arguments. - dbus.add_parameter(&arguments_variant(arguments)); + dbus.add_parameter(command); } dbus.send()?; Ok(true) } -/// Zig constructs each GVariant string from serialized bytes including its NUL. -/// Keep valid wire bytes unchanged. Zig marks these strings trusted, causing -/// GIO to abort when serializing invalid UTF-8 or an interior NUL. Fail the same -/// way explicitly: an untrusted GVariant would silently substitute an empty -/// string, while a malformed trusted variant would violate Rust GLib's contract. -fn arguments_variant(arguments: &[Vec]) -> glib::Variant { - glib::Variant::array_from_iter_with_type( +/// Zig constructs each GVariant string from serialized bytes including its NUL +/// and marks it trusted (`new_window.zig:49-53`), so GIO aborts the process when +/// serializing invalid UTF-8 or an interior NUL (input-triggered crash). +/// DEVIANT FIX: reject such arguments gracefully instead (see +/// DEVIANT_FIXES.md); valid wire bytes are unchanged. +fn arguments_variant(arguments: &[Vec]) -> Result { + if let Some(bad) = arguments + .iter() + .find(|arg| arg.contains(&0) || std::str::from_utf8(arg).is_err()) + { + log::error!("invalid D-Bus string in new-window arguments"); + eprintln!( + "new-window argument is not valid UTF-8 or contains a NUL byte: {}", + String::from_utf8_lossy(bad).escape_debug() + ); + return Err(Error::IpcFailed); + } + Ok(glib::Variant::array_from_iter_with_type( glib::VariantTy::STRING, arguments.iter().map(|arg| { - if arg.contains(&0) || std::str::from_utf8(arg).is_err() { - log::error!("invalid D-Bus string in new-window arguments"); - std::process::abort(); - } let mut bytes = arg.clone(); bytes.push(0); glib::Variant::from_bytes_with_type( @@ -62,47 +75,26 @@ fn arguments_variant(arguments: &[Vec]) -> glib::Variant { glib::VariantTy::STRING, ) }), - ) + )) } #[cfg(test)] mod tests { #![allow(clippy::unwrap_used)] use super::*; - #[cfg(unix)] #[test] - fn invalid_dbus_strings_fail_without_substitution() { - use std::os::unix::process::ExitStatusExt; - const CHILD: &str = "GHOSTTY_TEST_INVALID_DBUS_VALUE"; - if let Ok(kind) = std::env::var(CHILD) { - let value: &[u8] = if kind == "utf8" { - b"--title=bad\xff" - } else { - b"--title=bad\0tail" - }; - arguments_variant(&[value.to_vec()]); - panic!("invalid D-Bus argument must abort like Zig/GIO"); - } - for kind in ["utf8", "nul"] { - // Disable core dumps in the child without changing this test process. - let result = std::process::Command::new("/bin/sh") - .args(["-c", "ulimit -c 0; exec \"$@\"", "ghostty-dbus-test"]) - .arg(std::env::current_exe().unwrap()) - .args([ - "--exact", - "ipc::new_window::tests::invalid_dbus_strings_fail_without_substitution", - ]) - .env(CHILD, kind) - .output() - .unwrap(); - assert_eq!(result.status.signal(), Some(libc::SIGABRT)); + fn invalid_dbus_strings_are_rejected_without_abort() { + for value in [&b"--title=bad\xff"[..], &b"--title=bad\0tail"[..]] { + let result = arguments_variant(&[b"ok".to_vec(), value.to_vec()]); + assert!(matches!(result, Err(Error::IpcFailed))); } + assert!(arguments_variant(&[b"ok".to_vec()]).is_ok()); } #[test] fn arguments_are_plain_dbus_strings_not_escape_encoded() { let argument = "--working-directory=/tmp/\u{fdd0}é"; - let variant = arguments_variant(&[argument.as_bytes().to_vec()]); + let variant = arguments_variant(&[argument.as_bytes().to_vec()]).unwrap(); assert_eq!(variant.get::>().unwrap(), [argument]); assert_eq!( variant.child_value(0).data(), diff --git a/docs/rewrite/DEVIANT_FIXES.md b/docs/rewrite/DEVIANT_FIXES.md index ca654f7..a18174e 100644 --- a/docs/rewrite/DEVIANT_FIXES.md +++ b/docs/rewrite/DEVIANT_FIXES.md @@ -56,6 +56,7 @@ when this file was written. | 28 | CLI: `+list-colors` plain output | `cli/list_colors.zig:59-72` | 4096-byte buffer, never flushed; piped output stops midway through `PeachPuff3` (12,297 bytes). | Flushed (15,434 bytes). | 6286e02 | none; found by RV11 (`review-rv11-codex.md`) | | 29 | CLI: `+list-themes` diagnostics | `cli/list_themes.zig:124-131,171` | The messages ("Could not find the Ghostty resources directory", "No themes found...") go to a 4096-byte stderr buffer that is never flushed, so they are lost. | Flushed when the action ends. | `410d8fc` fix-deviant(cli) | `zig_writer::tests::unflushed_buffer_is_flushed_on_drop` | | 30 | CLI: stdout as a regular file | `std.fs.File.writer` (Zig 0.15.2) as used in `cli/show_face.zig:68-73`, `cli/help.zig:34`, `cli/validate_config.zig:43-46`, `cli/show_config.zig:84` | The writer `pwrite`s at its own offset starting at 0: with stdout redirected to a file, output overwrites itself and earlier file content, and `Writer.end` truncates. | Writes sequentially at the inherited descriptor's current offset (no `pwrite` from offset 0, so earlier content and the writers' own output are not overwritten; this appends only when the descriptor is opened for append, e.g. `>>`); no truncation. | `410d8fc` fix-deviant(cli) (append behaviour first in `f76179f`) | `zig_writer::tests::regular_file_output_is_complete_and_appended` | +| 31 | GTK: `+new-window` D-Bus arguments | `apprt/gtk/ipc/new_window.zig:49-53` (`Bytes.new(ptr, len + 1)` + `Variant.newFromBytes(..., trusted = true)`) | Arguments are passed as trusted GVariant strings without validation; invalid UTF-8 or an interior NUL makes GIO abort the process while serializing (input-triggered crash). | `arguments_variant` validates first and returns `Error::IpcFailed` (message on stderr, no D-Bus call); valid arguments are byte-identical to Zig. | uncommitted (Z18) | `ipc::new_window::tests::invalid_dbus_strings_are_rejected_without_abort` (`ghostty-apprt-gtk`) | ## Upstream quirks deliberately kept (not fixed)