fix(kitty): open a file transfer non-blocking, so the fifo check can run

`read_file` refuses anything that is not a regular file, and the comment
above it says why: fifos and devices, "where the open itself can block". The
check sat one call after the open. `open(O_RDONLY)` on a fifo blocks until a
writer arrives — for a fifo nobody intends to write to, forever — so the
guard never got the chance to refuse the thing it was written to refuse.

Reachable the same way everything else in this path is: the name comes out of
an escape sequence, so `mkfifo` plus one line of output, or any file someone
`cat`s, is enough. `resolve` runs on the daemon's pane reader thread, so the
pane's output stops there and does not come back while the process lives.

The open now carries `O_NONBLOCK`, which returns immediately on a fifo and
lets the existing check refuse it. On a regular file the flag changes
nothing — it is not consulted by `read`, and reads of regular files do not
block regardless.

Found by mutation: removing this check, the shm-name check, and the temp-dir
unlink check each left the whole workspace suite green, so all three were
being asserted only as pure functions and never where they are wired in.
Testing the first one properly is what surfaced the ordering.

The guard resolves a transfer naming a real fifo on a worker with a ten-second
deadline, because without the fix this does not fail, it hangs — and a test
that hangs says much less than one that reports what happened. Verified: with
the blocking open restored it fails at ten seconds with that message.
This commit is contained in:
l0ng-ai
2026-08-23 04:15:43 +08:00
parent ba28a8224e
commit bf68eb9f20
2 changed files with 80 additions and 3 deletions
+10
View File
@@ -153,6 +153,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
- **A kitty graphics transfer naming a FIFO no longer wedges the pane.**
`t=f`/`t=t` name a path for the terminal to read, and the name comes out of
an escape sequence — displaying a file is enough to send one. Opening a FIFO
for reading blocks until a writer arrives, which for one nobody intends to
write to is never, and this runs on the pane's reader thread: that pane's
output would stop for the life of the process. The check that refuses
anything which is not a regular file was already there and says it keeps us
off FIFOs, but it sat one call after the open. The open now uses
`O_NONBLOCK`, so it returns and the check can do its job.
- **A second agent in the same pane no longer inherits the first one's
turn.** The foreground process is read twice a second, and a chained
`claude; codex` starts the second agent microseconds after the first exits
+70 -3
View File
@@ -930,9 +930,25 @@ impl MediumTransfer {
// -reachable (any program that can write to the pty picks it), and
// `fs::read` on `/dev/zero` never returns — on the daemon *reader*
// thread, which would wedge the pane's whole output path. Refusing
// anything that isn't a regular file also keeps us off fifos and
// devices, where the open itself can block.
let file = std::fs::File::open(&path).ok()?;
// anything that isn't a regular file keeps us off fifos and devices.
//
// `O_NONBLOCK` on the *open*, because that check comes after it and
// opening is where a fifo hangs: `open(O_RDONLY)` on one blocks until a
// writer arrives, which for a fifo nobody intends to write to is never.
// Naming one costs an attacker a `mkfifo` and a line of output, and it
// would wedge that pane's reader thread for the life of the process —
// the exact failure the next few lines are written to prevent, reached
// one call too early to prevent it. On a regular file the flag changes
// nothing: it is not inherited by `read`, and reads of regular files do
// not block regardless.
let file = {
use std::os::unix::fs::OpenOptionsExt as _;
std::fs::OpenOptions::new()
.read(true)
.custom_flags(libc::O_NONBLOCK)
.open(&path)
.ok()?
};
let meta = file.metadata().ok()?;
if !meta.is_file() || meta.len() as usize > MAX_IMAGE_BYTES {
return None;
@@ -1733,6 +1749,57 @@ mod tests {
}
}
/// A transfer naming a fifo is refused, and refused *promptly*.
///
/// `open(O_RDONLY)` on a fifo blocks until a writer arrives, and for one
/// nobody intends to write to that is never. `resolve` runs on the daemon's
/// pane reader thread, so a single line of output naming a fifo — `mkfifo`
/// plus a `printf` of an APC sequence, or any file someone `cat`s — would
/// wedge that pane's output for the life of the process.
///
/// The "not a regular file" check has always been there and says it keeps
/// us off fifos; it sat one call *after* the open, so it never got the
/// chance. The open now carries `O_NONBLOCK`, which returns immediately on
/// a fifo and lets the check do its job.
///
/// Run on a worker with a deadline rather than inline: without the fix this
/// does not fail, it hangs, and a test that hangs tells whoever broke it
/// much less than one that says what happened.
#[test]
#[cfg(unix)]
fn a_transfer_naming_a_fifo_is_refused_instead_of_blocking_the_reader() {
let dir = std::env::temp_dir().join(format!("tty7-kitty-fifo-{}", std::process::id()));
let _ = std::fs::remove_dir_all(&dir);
std::fs::create_dir_all(&dir).unwrap();
let fifo = dir.join("pipe");
let c = std::ffi::CString::new(fifo.as_os_str().as_encoded_bytes()).unwrap();
// 0o600: nothing else needs it, and the test should not depend on umask.
let made = unsafe { libc::mkfifo(c.as_ptr(), 0o600) };
assert_eq!(made, 0, "could not create the fifo to test against");
let (tx, rx) = std::sync::mpsc::channel();
let probe = fifo.clone();
std::thread::spawn(move || {
let _ = tx.send(temp_file_transfer(&probe).resolve());
});
match rx.recv_timeout(std::time::Duration::from_secs(10)) {
Ok(resolved) => assert_eq!(
resolved, None,
"a fifo is not a regular file and must not resolve"
),
Err(_) => panic!(
"resolving a transfer that names a fifo blocked; on the daemon \
this is the pane's reader thread and it would never come back"
),
}
// The fifo is not in the sender's gift to delete either — the temp-dir
// check allows it here, but a refused read must not have unlinked it.
assert!(fifo.exists(), "a refused transfer should leave the path alone");
let _ = std::fs::remove_dir_all(&dir);
}
#[test]
#[cfg(unix)]
fn a_temp_file_transfer_outside_the_temp_dir_is_read_but_not_deleted() {