fix-deviant(gtk): reject malformed +new-window arguments instead of aborting
Intentional deviation from Ghostty 1.3.1 (DEVIANT_FIXES.md row 31). Zig's apprt/gtk/ipc/new_window.zig:49-53 builds the D-Bus argument array from raw bytes as a *trusted* GVariant; an argument with invalid UTF-8 or an interior NUL violates GVariant's string invariants, which GLib turns into a process abort. The Rust port mirrored this with an explicit abort(). new_window now validates every argument before touching the bus: a bad argument prints an error to stderr and returns Error::IpcFailed, so the CLI exits with failure and nothing crashes. Valid arguments are sent byte-identically to Ghostty. Test Plan: - cargo test -p ghostty-apprt-gtk new_window (invalid_dbus_strings_are_rejected_without_abort)
This commit is contained in:
1 parent
03e70e4276
commit
67a7b2937a
2 files changed
+34
-41
No files matched your search
@@ -21,6 +21,12 @@ use gtk::glib;
|
|||||||
use super::dbus::DBus;
|
use super::dbus::DBus;
|
||||||
|
|
||||||
pub fn new_window(target: &Target, value: &NewWindow) -> Result<bool, Error> {
|
pub fn new_window(target: &Target, value: &NewWindow) -> Result<bool, Error> {
|
||||||
|
// 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(
|
let mut dbus = DBus::new(
|
||||||
target,
|
target,
|
||||||
if value.arguments.is_none() {
|
if value.arguments.is_none() {
|
||||||
@@ -30,31 +36,38 @@ pub fn new_window(target: &Target, value: &NewWindow) -> Result<bool, Error> {
|
|||||||
},
|
},
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
if let Some(arguments) = &value.arguments {
|
if let Some(command) = &command {
|
||||||
// If any arguments were specified on the command line, the first
|
// If any arguments were specified on the command line, the first
|
||||||
// parameter is an array of strings that contain the arguments. They
|
// parameter is an array of strings that contain the arguments. They
|
||||||
// will be sent to the main Ghostty instance and interpreted as CLI
|
// will be sent to the main Ghostty instance and interpreted as CLI
|
||||||
// arguments.
|
// arguments.
|
||||||
dbus.add_parameter(&arguments_variant(arguments));
|
dbus.add_parameter(command);
|
||||||
}
|
}
|
||||||
|
|
||||||
dbus.send()?;
|
dbus.send()?;
|
||||||
Ok(true)
|
Ok(true)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Zig constructs each GVariant string from serialized bytes including its NUL.
|
/// Zig constructs each GVariant string from serialized bytes including its NUL
|
||||||
/// Keep valid wire bytes unchanged. Zig marks these strings trusted, causing
|
/// and marks it trusted (`new_window.zig:49-53`), so GIO aborts the process when
|
||||||
/// GIO to abort when serializing invalid UTF-8 or an interior NUL. Fail the same
|
/// serializing invalid UTF-8 or an interior NUL (input-triggered crash).
|
||||||
/// way explicitly: an untrusted GVariant would silently substitute an empty
|
/// DEVIANT FIX: reject such arguments gracefully instead (see
|
||||||
/// string, while a malformed trusted variant would violate Rust GLib's contract.
|
/// DEVIANT_FIXES.md); valid wire bytes are unchanged.
|
||||||
fn arguments_variant(arguments: &[Vec<u8>]) -> glib::Variant {
|
fn arguments_variant(arguments: &[Vec<u8>]) -> Result<glib::Variant, Error> {
|
||||||
glib::Variant::array_from_iter_with_type(
|
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,
|
glib::VariantTy::STRING,
|
||||||
arguments.iter().map(|arg| {
|
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();
|
let mut bytes = arg.clone();
|
||||||
bytes.push(0);
|
bytes.push(0);
|
||||||
glib::Variant::from_bytes_with_type(
|
glib::Variant::from_bytes_with_type(
|
||||||
@@ -62,47 +75,26 @@ fn arguments_variant(arguments: &[Vec<u8>]) -> glib::Variant {
|
|||||||
glib::VariantTy::STRING,
|
glib::VariantTy::STRING,
|
||||||
)
|
)
|
||||||
}),
|
}),
|
||||||
)
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
#![allow(clippy::unwrap_used)]
|
#![allow(clippy::unwrap_used)]
|
||||||
use super::*;
|
use super::*;
|
||||||
#[cfg(unix)]
|
|
||||||
#[test]
|
#[test]
|
||||||
fn invalid_dbus_strings_fail_without_substitution() {
|
fn invalid_dbus_strings_are_rejected_without_abort() {
|
||||||
use std::os::unix::process::ExitStatusExt;
|
for value in [&b"--title=bad\xff"[..], &b"--title=bad\0tail"[..]] {
|
||||||
const CHILD: &str = "GHOSTTY_TEST_INVALID_DBUS_VALUE";
|
let result = arguments_variant(&[b"ok".to_vec(), value.to_vec()]);
|
||||||
if let Ok(kind) = std::env::var(CHILD) {
|
assert!(matches!(result, Err(Error::IpcFailed)));
|
||||||
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));
|
|
||||||
}
|
}
|
||||||
|
assert!(arguments_variant(&[b"ok".to_vec()]).is_ok());
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn arguments_are_plain_dbus_strings_not_escape_encoded() {
|
fn arguments_are_plain_dbus_strings_not_escape_encoded() {
|
||||||
let argument = "--working-directory=/tmp/\u{fdd0}é";
|
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::<Vec<String>>().unwrap(), [argument]);
|
assert_eq!(variant.get::<Vec<String>>().unwrap(), [argument]);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
variant.child_value(0).data(),
|
variant.child_value(0).data(),
|
||||||
|
|||||||
@@ -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`) |
|
| 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` |
|
| 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` |
|
| 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)
|
## Upstream quirks deliberately kept (not fixed)
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user