From 52a09748366046cd3f5945d2d702a5f0bf086ad5 Mon Sep 17 00:00:00 2001 From: James Long Date: Thu, 8 Oct 2026 18:09:09 +0000 Subject: [PATCH 1/2] feat(daemon): enable private Windows named-pipe service Rebased onto named runtime directories and ticket takeover: - Windows uses the shared `--name`/`--runtime-dir` layout instead of a fixed LOCALAPPDATA path, keeping private ACL, reparse, lock and atomic FileRenameInfoEx publication checks (folded from the follow-up rename fixes). - The stale-sibling sweep is shared by Unix and Windows; platforms supply only process liveness and stale endpoint removal. - The shared server uses generation-based ownership, and the Windows handoff test covers takeover while the previous owner is connected. --- .github/workflows/windows.yml | 9 +- Cargo.toml | 1 + README.md | 46 +++- src/daemon.rs | 31 ++- src/daemon/server.rs | 74 +++--- src/daemon/sweep.rs | 222 +++++++++++++++++ src/daemon/unix.rs | 222 ++--------------- src/daemon/windows.rs | 402 ++++++++++++++++++++++++++++++ src/lib.rs | 2 +- src/transport/mod.rs | 4 +- src/transport/windows.rs | 28 ++- src/transport/windows/security.rs | 185 +++++++++++++- tests/windows-daemon.rs | 389 +++++++++++++++++++++++++++++ 13 files changed, 1347 insertions(+), 268 deletions(-) create mode 100644 src/daemon/sweep.rs create mode 100644 src/daemon/windows.rs create mode 100644 tests/windows-daemon.rs diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 4870a77..a5b9de5 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -126,6 +126,10 @@ jobs: if: failure() && steps.tests.outcome == 'failure' timeout-minutes: 3 run: cargo test --locked --test runtime -- --test-threads=1 --nocapture + - name: Diagnose failed daemon tests + if: failure() && steps.tests.outcome == 'failure' + timeout-minutes: 3 + run: cargo test --locked --test windows-daemon -- --test-threads=1 --nocapture - name: Smoke test executable if: ${{ !cancelled() && steps.build.outcome == 'success' }} run: | @@ -145,9 +149,10 @@ jobs: Target: $env:CARGO_BUILD_TARGET - This job builds the executable and runs all enabled tests natively, including libghostty parser, protocol, and direct TerminalService ConPTY runtime tests. + This job builds the executable and runs all enabled tests natively, including libghostty parser, protocol, direct TerminalService ConPTY runtime, named-pipe transport, and daemon tests. The runtime suite spawns real Rust console children and checks input/output, Unicode, cwd/environment/argv/PATH, OS resize, snapshots, replay, terminal replies, and independent terminals. + Daemon tests cover private atomic registration, ownership/handoff, stale registration/locking, partial requests, blocked subscribers, and live ConPTY operations over real named pipes. The older service, ownership, playground, and rows integration suites remain Unix-only. - A passing run does not imply Windows daemon support or complete ConPTY shutdown coverage is implemented. + Basic tests do not establish exhaustive lifecycle coverage or Windows interactive CLI parity. No packages or releases are published. "@ >> $env:GITHUB_STEP_SUMMARY diff --git a/Cargo.toml b/Cargo.toml index 50f8354..57a3480 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ windows-sys = { version = "0.61.2", features = [ "Win32_System_IO", "Win32_System_Pipes", "Win32_System_Threading", + "Win32_System_WindowsProgramming", ] } [target.'cfg(windows)'.dev-dependencies] diff --git a/README.md b/README.md index 994dc56..92e7425 100644 --- a/README.md +++ b/README.md @@ -10,18 +10,19 @@ cannot block the service or another terminal. Every daemon has one owner connection. The playground starts a daemon and holds that connection until exit; exiting the playground stops the daemon and all its terminals. Observer commands connect to an existing daemon without taking ownership. -The service uses a private authenticated Unix socket and atomic registration file. +The service uses a private authenticated local byte stream (Unix socket or Windows +named pipe) and an atomic registration file. Integrations launch `opencode-pty daemon --name NAME [--runtime-dir DIR]` (protocol 7). The server must claim the daemon within 5 seconds by sending the authenticated framed envelope `{"token":"...","request":{"op":"own","instance_id":"..."}}`. The response is `{"type":"owned"}`; that connection stays open as the sole -owner. Ordinary requests and subscriptions use their existing separate sockets. +owner. Ordinary requests and subscriptions use separate connections. The instance ID and token come from the private registration file. Losing the owner connection stops the daemon and its terminals unless the owner first sends `{"token":"...","request":{"op":"prepare_handoff"}}` on that same -socket. The response is `{"type":"handoff","ticket":"...","expires_at":123}`, +connection. The response is `{"type":"handoff","ticket":"...","expires_at":123}`, where `expires_at` is Unix milliseconds, 120 seconds from preparation. Repeated preparation during that window returns the same ticket and deadline. A successor claims the same instance with the ticket in its `own` request, even while the old @@ -74,8 +75,25 @@ response or the final subscription event. Completion retains queued bytes while waiting for that close, with a two-second grace period and daemon cancellation; it never calls the potentially unbounded `FlushFileBuffers`. Stopping acceptance retains a pipe instance until registration is removed, preventing namespace -squatting during cleanup. The backend has native Windows tests, but the Windows -daemon entrypoint/registration lifecycle is not enabled yet. +squatting during cleanup. + +On Windows, `opencode-pty daemon --name NAME [--runtime-dir DIR]` uses the same +`DIR/NAME` runtime directory; the default `DIR` resolves through `XDG_STATE_HOME` +or the user profile's `.local\state\opencode\pty`, matching OpenCode's state +directory. The runtime directory must be absolute. Storage entries must not be +reparse points or owned by another user; the directory, lock, and registration +use protected current-user-only ACLs, and clients verify the registration's owner +and ACL before trusting it. The held directory/lock handles deny delete sharing, +and registration is atomically replaced under the exclusive lock. Stale +registration is replaced with a fresh instance ID, token, and pipe name; cleanup +removes registration only if its instance ID still matches, then removes the lock +file and runtime directory after releasing their handles. The abandoned-sibling +sweep is shared with Unix; it checks liveness through the registered process +object and has no endpoint file to remove. + +The Windows Rust `TerminalClient` and interactive CLI are not ported. Integrations +can use protocol 7 directly and the minimal `daemon::PipeConnection` byte-stream +helper (connect plus optional read/write timeouts). ## Architecture @@ -278,11 +296,15 @@ observes actual console stdin without echo; `Size` and `Context` inspect the child's OS console and process context. The ignored `child` test is only its subprocess entry point, not skipped runtime coverage. -The older service, ownership, playground, and rows integration suites remain -Unix-only. Windows library tests also exercise real named-pipe roundtrips, -multiple connections, namespace ownership, cancellation, and final-frame -completion. These checks do not yet establish Windows daemon support or -complete ConPTY shutdown behavior. +The original service, ownership, playground, and rows integration suites remain +Unix-only. Windows library tests exercise real named-pipe roundtrips, multiple +connections, namespace ownership, cancellation, final-frame completion, and +private atomic registration. `tests/windows-daemon.rs` exercises authenticated +ownership/handoff, locking/stale registration, partial requests, blocked +subscribers, and live ConPTY create/input/output/resize/shutdown through the real +daemon. Natural child-exit/ConPTY EOF and runtime cleanup are checked by the +direct runtime suite; the basic daemon tests do not establish complete lifecycle +coverage on their own. On Windows, normal root-child exit hands the ConPTY master to the existing child-wait worker for closing. The actor and reader continue draining until the @@ -337,8 +359,8 @@ uses named pipes. Platform signing will be added later. ## Current Limits -- The Windows named-pipe backend is tested independently; persistent daemon - startup and private registration storage are still Unix-only. +- Windows supports the daemon/protocol transport, not the Rust interactive + TerminalClient/play/watch CLI. - Ordinary API operations use one framed JSON request per connection; subscriptions keep the authenticated connection open for ordered live events. - The OpenCode backend proxy and ordered group APIs are implemented, but the diff --git a/src/daemon.rs b/src/daemon.rs index 946e81b..c3d38a3 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1,6 +1,6 @@ use std::path::{Path, PathBuf}; -use anyhow::{Context, Result, bail}; +use anyhow::{Result, bail}; use serde::{Deserialize, Serialize}; pub const REGISTRATION_FILE: &str = "service.json"; @@ -57,21 +57,38 @@ pub fn registration_path(directory: &Path) -> PathBuf { } pub fn read_registration(directory: &Path) -> Result { - let data = std::fs::read(registration_path(directory)) - .context("opencode-pty registration is unavailable")?; - serde_json::from_slice(&data).context("invalid opencode-pty registration") + // Windows also verifies the file's owner and private ACL before trusting it. + #[cfg(windows)] + return platform::read_registration(directory); + #[cfg(not(windows))] + { + use anyhow::Context; + let data = std::fs::read(registration_path(directory)) + .context("opencode-pty registration is unavailable")?; + serde_json::from_slice(&data).context("invalid opencode-pty registration") + } } #[cfg(unix)] #[path = "daemon/unix.rs"] mod platform; -#[cfg(unix)] +#[cfg(windows)] +#[path = "daemon/windows.rs"] +mod platform; +#[cfg(any(unix, windows))] mod server; +#[cfg(any(unix, windows))] +mod sweep; -#[cfg(unix)] +#[cfg(any(unix, windows))] pub use server::run; -#[cfg(not(unix))] +/// Minimal Windows byte-stream client for integrations using protocol framing. +/// This does not start a daemon or implement the interactive TerminalClient CLI. +#[cfg(windows)] +pub use crate::transport::windows::Connection as PipeConnection; + +#[cfg(not(any(unix, windows)))] pub fn run(_directory: &Path) -> Result<()> { anyhow::bail!("persistent opencode-pty transport is not implemented on this platform") } diff --git a/src/daemon/server.rs b/src/daemon/server.rs index 6019966..b3603ad 100644 --- a/src/daemon/server.rs +++ b/src/daemon/server.rs @@ -22,41 +22,49 @@ pub fn run(directory: &Path) -> Result<()> { let service = Arc::new(TerminalService::default()); let shutdown = Arc::new(AtomicBool::new(false)); let mut handlers = Vec::<(Cancellation, thread::JoinHandle<()>)>::new(); - while !shutdown.load(Ordering::Acquire) { - if ownership - .lock() - .map_err(|_| anyhow!("ownership lock poisoned"))? - .tick(Instant::now()) - { - shutdown.store(true, Ordering::Release); - break; - } - for (_, handler) in handlers.extract_if(.., |(_, handler)| handler.is_finished()) { - let _ = handler.join(); - } - match listener.accept() { - Ok(Some(stream)) => { - let control = stream.cancellation()?; - let service = Arc::clone(&service); - let shutdown = Arc::clone(&shutdown); - let ownership = Arc::clone(&ownership); - let registration = registration.clone(); - let handle = thread::spawn(move || { - if let Err(error) = - handle_connection(stream, &service, ®istration, &shutdown, &ownership) - { - eprintln!("opencode-pty request failed: {error:#}"); - } - }); - handlers.push((control, handle)); + let result = (|| -> Result<()> { + while !shutdown.load(Ordering::Acquire) { + if ownership + .lock() + .map_err(|_| anyhow!("ownership lock poisoned"))? + .tick(Instant::now()) + { + shutdown.store(true, Ordering::Release); + break; } - Ok(None) => { - thread::sleep(Duration::from_millis(10)); + for (_, handler) in handlers.extract_if(.., |(_, handler)| handler.is_finished()) { + let _ = handler.join(); + } + match listener.accept() { + Ok(Some(stream)) => { + let control = stream.cancellation()?; + let service = Arc::clone(&service); + let shutdown = Arc::clone(&shutdown); + let ownership = Arc::clone(&ownership); + let registration = registration.clone(); + let handle = thread::spawn(move || { + if let Err(error) = handle_connection( + stream, + &service, + ®istration, + &shutdown, + &ownership, + ) { + eprintln!("opencode-pty request failed: {error:#}"); + } + }); + handlers.push((control, handle)); + } + Ok(None) => { + thread::sleep(Duration::from_millis(10)); + } + Err(error) => return Err(error.into()), } - Err(error) => return Err(error.into()), } - } + Ok(()) + })(); + shutdown.store(true, Ordering::Release); listener.stop(); // Unblock partial requests, owner reads, and backpressured subscriptions // before joining. PTY workers still use their existing termination path. @@ -77,11 +85,11 @@ pub fn run(directory: &Path) -> Result<()> { let _ = handler.join(); } drop(service); - let result = runtime.finish(); + let cleanup = runtime.finish(); drop(listener); let _ = cleanup_tx.send(()); let _ = watchdog.join(); - result + result.and(cleanup) } fn handle_connection( diff --git a/src/daemon/sweep.rs b/src/daemon/sweep.rs new file mode 100644 index 0000000..653c6ab --- /dev/null +++ b/src/daemon/sweep.rs @@ -0,0 +1,222 @@ +use std::fs::{self, OpenOptions}; +use std::path::Path; +use std::time::{Duration, SystemTime}; + +use anyhow::Result; +use fs2::FileExt; + +use super::{LOCK_FILE, REGISTRATION_FILE, Registration, platform, registration_path}; + +pub(super) const STALE_RUNTIME_AGE: Duration = Duration::from_secs(10 * 60); + +/// Removes sibling runtime directories left by daemons that crashed. +pub(super) fn stale_runtimes(directory: &Path) { + let Some(parent) = directory.parent() else { + return; + }; + let Ok(entries) = fs::read_dir(parent) else { + return; + }; + for entry in entries.flatten() { + let candidate = entry.path(); + if same_path(&candidate, directory) { + continue; + } + if let Err(error) = runtime(&candidate, SystemTime::now()) { + eprintln!( + "opencode-pty could not remove stale runtime {}: {error:#}", + candidate.display() + ); + } + } +} + +fn same_path(left: &Path, right: &Path) -> bool { + matches!( + (fs::canonicalize(left), fs::canonicalize(right)), + (Ok(left), Ok(right)) if left == right + ) +} + +pub(super) fn runtime(directory: &Path, now: SystemTime) -> Result<()> { + // Symlinks and Windows junctions are never followed or removed. + let metadata = fs::symlink_metadata(directory)?; + if !metadata.is_dir() || metadata.file_type().is_symlink() { + return Ok(()); + } + let age = now.duration_since(metadata.modified()?).unwrap_or_default(); + if age < STALE_RUNTIME_AGE { + return Ok(()); + } + let mut files = Vec::new(); + for entry in fs::read_dir(directory)? { + let entry = entry?; + let name = entry.file_name(); + let name = name.to_string_lossy(); + let known = name == LOCK_FILE + || name == REGISTRATION_FILE + || (name.starts_with("service.") && name.ends_with(".tmp")); + if !known || !entry.file_type()?.is_file() { + // Not a runtime directory this daemon created. + return Ok(()); + } + files.push(entry.path()); + } + + // Only a lock file or a parseable registration proves a daemon created this + // directory; anything else, including an empty directory, is left alone. + let lock = match OpenOptions::new() + .read(true) + .write(true) + .open(directory.join(LOCK_FILE)) + { + Ok(lock) => { + if lock.try_lock_exclusive().is_err() { + return Ok(()); + } + Some(lock) + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + let Some(registration) = fs::read(registration_path(directory)) + .ok() + .and_then(|data| serde_json::from_slice::(&data).ok()) + else { + return Ok(()); + }; + if platform::process_exists(registration.pid) { + return Ok(()); + } + None + } + Err(error) => return Err(error.into()), + }; + + platform::remove_stale_endpoint(&fs::canonicalize(directory)?)?; + // Remove the lock last so a racing daemon cannot claim a half-removed directory. + files.sort_by_key(|file| file.ends_with(LOCK_FILE)); + for file in files { + fs::remove_file(file)?; + } + // Windows deletes an open file only when its last handle closes. + drop(lock); + fs::remove_dir(directory)?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use super::*; + use crate::protocol::PROTOCOL_VERSION; + + fn random_id() -> String { + format!("{:032x}", rand::random::()) + } + + struct Root(PathBuf); + + impl Root { + fn new() -> Self { + let root = + std::env::temp_dir().join(format!("opencode-pty-sweep-test-{}", random_id())); + fs::create_dir_all(&root).unwrap(); + Self(root) + } + + fn runtime(&self, files: &[&str], pid: Option) -> PathBuf { + let directory = self.0.join(random_id()); + fs::create_dir(&directory).unwrap(); + for file in files { + fs::write(directory.join(file), b"").unwrap(); + } + if let Some(pid) = pid { + let registration = Registration { + instance_id: random_id(), + pid, + protocol: PROTOCOL_VERSION, + socket: PathBuf::from("/nonexistent.sock"), + token: random_id(), + }; + fs::write( + directory.join(REGISTRATION_FILE), + serde_json::to_vec(®istration).unwrap(), + ) + .unwrap(); + } + directory + } + } + + impl Drop for Root { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } + + fn later() -> SystemTime { + SystemTime::now() + STALE_RUNTIME_AGE + Duration::from_secs(1) + } + + fn dead_pid() -> u32 { + #[cfg(unix)] + let mut child = std::process::Command::new("/usr/bin/true").spawn().unwrap(); + #[cfg(windows)] + let mut child = std::process::Command::new("cmd.exe") + .args(["/C", "exit 0"]) + .spawn() + .unwrap(); + let pid = child.id(); + child.wait().unwrap(); + pid + } + + #[test] + fn sweep_removes_only_abandoned_old_runtimes() { + let root = Root::new(); + let abandoned = root.runtime(&[LOCK_FILE, REGISTRATION_FILE], None); + let young = root.runtime(&[LOCK_FILE], None); + let foreign = root.runtime(&[LOCK_FILE, "notes.txt"], None); + let locked = root.runtime(&[LOCK_FILE], None); + let held = fs::File::open(locked.join(LOCK_FILE)).unwrap(); + held.try_lock_exclusive().unwrap(); + + runtime(&abandoned, later()).unwrap(); + runtime(&young, SystemTime::now()).unwrap(); + runtime(&foreign, later()).unwrap(); + runtime(&locked, later()).unwrap(); + + assert!(!abandoned.exists()); + assert!(young.join(LOCK_FILE).exists()); + assert!(foreign.join("notes.txt").exists()); + assert!(locked.join(LOCK_FILE).exists()); + drop(held); + } + + #[test] + fn sweep_without_lock_file_checks_registered_pid() { + let root = Root::new(); + let live = root.runtime(&[], Some(std::process::id())); + let dead = root.runtime(&[], Some(dead_pid())); + + runtime(&live, later()).unwrap(); + runtime(&dead, later()).unwrap(); + + assert!(live.join(REGISTRATION_FILE).exists()); + assert!(!dead.exists()); + } + + #[test] + fn sweep_keeps_directories_without_daemon_evidence() { + let root = Root::new(); + let empty = root.runtime(&[], None); + let foreign = root.runtime(&[], None); + fs::write(foreign.join(REGISTRATION_FILE), br#"{"not":"ours"}"#).unwrap(); + let temporary = root.runtime(&["service.important.tmp"], None); + + for directory in [&empty, &foreign, &temporary] { + runtime(directory, later()).unwrap(); + assert!(directory.exists(), "{}", directory.display()); + } + } +} diff --git a/src/daemon/unix.rs b/src/daemon/unix.rs index 0ce9f8c..30e2a0c 100644 --- a/src/daemon/unix.rs +++ b/src/daemon/unix.rs @@ -4,18 +4,15 @@ use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::{FileTypeExt, MetadataExt, OpenOptionsExt, PermissionsExt}; use std::path::{Path, PathBuf}; use std::thread::{self, JoinHandle}; -use std::time::{Duration, SystemTime}; use anyhow::{Context, Result, anyhow}; use fs2::FileExt; use sha2::{Digest, Sha256}; -use super::{LOCK_FILE, REGISTRATION_FILE, Registration, read_registration, registration_path}; +use super::{LOCK_FILE, Registration, read_registration, registration_path}; use crate::protocol::PROTOCOL_VERSION; use crate::transport::Listener; -const STALE_RUNTIME_AGE: Duration = Duration::from_secs(10 * 60); - pub(super) struct Runtime { pub registration: Registration, pub cleanup: Cleanup, @@ -55,7 +52,7 @@ impl Runtime { write_registration(&directory, ®istration)?; let sweeper = { let directory = directory.clone(); - thread::spawn(move || sweep_stale_runtimes(&directory)) + thread::spawn(move || super::sweep::stale_runtimes(&directory)) }; Ok(( Self { @@ -128,109 +125,24 @@ impl SocketFile { } } -/// Removes sibling runtime directories left by daemons that crashed. -fn sweep_stale_runtimes(directory: &Path) { - let Some(parent) = directory.parent() else { - return; - }; - let Ok(entries) = fs::read_dir(parent) else { - return; +pub(super) fn process_exists(pid: u32) -> bool { + let Ok(pid) = i32::try_from(pid) else { + return false; }; - for entry in entries.flatten() { - let candidate = entry.path(); - if same_path(&candidate, directory) { - continue; - } - if let Err(error) = sweep_runtime(&candidate, SystemTime::now()) { - eprintln!( - "opencode-pty could not remove stale runtime {}: {error:#}", - candidate.display() - ); - } - } -} - -fn same_path(left: &Path, right: &Path) -> bool { - matches!( - (fs::canonicalize(left), fs::canonicalize(right)), - (Ok(left), Ok(right)) if left == right - ) + // SAFETY: signal zero only checks whether the PID exists. + let exists = unsafe { libc::kill(pid, 0) } == 0; + exists || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) } -fn sweep_runtime(directory: &Path, now: SystemTime) -> Result<()> { - let metadata = fs::symlink_metadata(directory)?; - if !metadata.is_dir() { - return Ok(()); - } - let age = now.duration_since(metadata.modified()?).unwrap_or_default(); - if age < STALE_RUNTIME_AGE { - return Ok(()); - } - let mut files = Vec::new(); - for entry in fs::read_dir(directory)? { - let entry = entry?; - let name = entry.file_name(); - let name = name.to_string_lossy(); - let known = name == LOCK_FILE - || name == REGISTRATION_FILE - || (name.starts_with("service.") && name.ends_with(".tmp")); - if !known || !entry.file_type()?.is_file() { - // Not a runtime directory this daemon created. - return Ok(()); - } - files.push(entry.path()); - } - - // Only a lock file or a parseable registration proves a daemon created this - // directory; anything else, including an empty directory, is left alone. - let _lock = match OpenOptions::new() - .read(true) - .write(true) - .open(directory.join(LOCK_FILE)) - { - Ok(lock) => { - if lock.try_lock_exclusive().is_err() { - return Ok(()); - } - Some(lock) - } - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - let Some(registration) = fs::read(registration_path(directory)) - .ok() - .and_then(|data| serde_json::from_slice::(&data).ok()) - else { - return Ok(()); - }; - if process_exists(registration.pid) { - return Ok(()); - } - None - } - Err(error) => return Err(error.into()), - }; - - let socket = socket_file(&socket_root(), &fs::canonicalize(directory)?); +/// Removes the socket of an abandoned runtime found by the shared sweep. +pub(super) fn remove_stale_endpoint(canonical_directory: &Path) -> Result<()> { + let socket = socket_file(&socket_root(), canonical_directory); if fs::symlink_metadata(&socket).is_ok_and(|metadata| metadata.file_type().is_socket()) { fs::remove_file(&socket)?; } - // Remove the lock last so a racing daemon cannot claim a half-removed directory. - files.sort_by_key(|file| file.ends_with(LOCK_FILE)); - for file in files { - fs::remove_file(file)?; - } - fs::remove_dir(directory)?; Ok(()) } -fn process_exists(pid: u32) -> bool { - let Ok(pid) = i32::try_from(pid) else { - return false; - }; - // SAFETY: signal zero only checks whether the PID exists. - let exists = unsafe { libc::kill(pid, 0) } == 0; - exists || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) -} - fn socket_path(directory: &Path) -> Result { let directory = fs::canonicalize(directory).context("failed to resolve PTY runtime directory")?; @@ -328,116 +240,22 @@ mod tests { fs::remove_dir_all(base).unwrap(); } - struct Root(PathBuf); - - impl Root { - fn new() -> Self { - let root = - std::env::temp_dir().join(format!("opencode-pty-sweep-test-{}", random_id())); - fs::create_dir_all(&root).unwrap(); - Self(root) - } - - fn runtime(&self, files: &[&str], pid: Option) -> PathBuf { - let directory = self.0.join(random_id()); - fs::create_dir(&directory).unwrap(); - for file in files { - fs::write(directory.join(file), b"").unwrap(); - } - if let Some(pid) = pid { - let registration = Registration { - instance_id: random_id(), - pid, - protocol: PROTOCOL_VERSION, - socket: PathBuf::from("/nonexistent.sock"), - token: random_id(), - }; - fs::write( - directory.join(REGISTRATION_FILE), - serde_json::to_vec(®istration).unwrap(), - ) - .unwrap(); - } - directory - } - } - - impl Drop for Root { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.0); - } - } - - fn later() -> SystemTime { - SystemTime::now() + STALE_RUNTIME_AGE + Duration::from_secs(1) - } - - fn dead_pid() -> u32 { - let mut child = std::process::Command::new("/usr/bin/true").spawn().unwrap(); - let pid = child.id(); - child.wait().unwrap(); - pid - } - - #[test] - fn sweep_removes_only_abandoned_old_runtimes() { - let root = Root::new(); - let abandoned = root.runtime(&[LOCK_FILE, REGISTRATION_FILE], None); - let young = root.runtime(&[LOCK_FILE], None); - let foreign = root.runtime(&[LOCK_FILE, "notes.txt"], None); - let locked = root.runtime(&[LOCK_FILE], None); - let held = fs::File::open(locked.join(LOCK_FILE)).unwrap(); - held.try_lock_exclusive().unwrap(); - - sweep_runtime(&abandoned, later()).unwrap(); - sweep_runtime(&young, SystemTime::now()).unwrap(); - sweep_runtime(&foreign, later()).unwrap(); - sweep_runtime(&locked, later()).unwrap(); - - assert!(!abandoned.exists()); - assert!(young.join(LOCK_FILE).exists()); - assert!(foreign.join("notes.txt").exists()); - assert!(locked.join(LOCK_FILE).exists()); - drop(held); - } - - #[test] - fn sweep_without_lock_file_checks_registered_pid() { - let root = Root::new(); - let live = root.runtime(&[], Some(std::process::id())); - let dead = root.runtime(&[], Some(dead_pid())); - - sweep_runtime(&live, later()).unwrap(); - sweep_runtime(&dead, later()).unwrap(); - - assert!(live.join(REGISTRATION_FILE).exists()); - assert!(!dead.exists()); - } - - #[test] - fn sweep_keeps_directories_without_daemon_evidence() { - let root = Root::new(); - let empty = root.runtime(&[], None); - let foreign = root.runtime(&[], None); - fs::write(foreign.join(REGISTRATION_FILE), br#"{"not":"ours"}"#).unwrap(); - let temporary = root.runtime(&["service.important.tmp"], None); - - for directory in [&empty, &foreign, &temporary] { - sweep_runtime(directory, later()).unwrap(); - assert!(directory.exists(), "{}", directory.display()); - } - } - #[test] fn sweep_removes_abandoned_socket() { - let root = Root::new(); - let abandoned = root.runtime(&[LOCK_FILE], None); + let root = std::env::temp_dir().join(format!("opencode-pty-sweep-test-{}", random_id())); + let abandoned = root.join(random_id()); + fs::create_dir_all(&abandoned).unwrap(); + fs::write(abandoned.join(LOCK_FILE), b"").unwrap(); let socket = socket_path(&abandoned).unwrap(); drop(UnixListener::bind(&socket).unwrap()); - sweep_runtime(&abandoned, later()).unwrap(); + let later = std::time::SystemTime::now() + + super::super::sweep::STALE_RUNTIME_AGE + + std::time::Duration::from_secs(1); + super::super::sweep::runtime(&abandoned, later).unwrap(); assert!(!abandoned.exists()); assert!(!socket.exists()); + fs::remove_dir_all(root).unwrap(); } } diff --git a/src/daemon/windows.rs b/src/daemon/windows.rs new file mode 100644 index 0000000..dd5159b --- /dev/null +++ b/src/daemon/windows.rs @@ -0,0 +1,402 @@ +use std::ffi::OsString; +use std::fs::{self, File}; +use std::io::{Read, Write}; +use std::os::windows::ffi::{OsStrExt, OsStringExt}; +use std::os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle}; +use std::path::{Path, PathBuf}; +use std::ptr::null_mut; +use std::thread::{self, JoinHandle}; + +use anyhow::{Context, Result, bail}; +use fs2::FileExt; +use windows_sys::Win32::Foundation::{ + ERROR_ACCESS_DENIED, ERROR_ALREADY_EXISTS, GENERIC_READ, GENERIC_WRITE, WAIT_TIMEOUT, +}; +use windows_sys::Win32::Storage::FileSystem::{ + BY_HANDLE_FILE_INFORMATION, CREATE_NEW, CreateDirectoryW, CreateFileW, DELETE, + FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_REPARSE_POINT, FILE_FLAG_BACKUP_SEMANTICS, + FILE_FLAG_OPEN_REPARSE_POINT, FILE_READ_ATTRIBUTES, FILE_RENAME_INFO, FILE_RENAME_INFO_0, + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_TRAVERSE, FileRenameInfoEx, + GetFileInformationByHandle, GetFinalPathNameByHandleW, OPEN_ALWAYS, OPEN_EXISTING, + READ_CONTROL, SetFileInformationByHandle, WRITE_DAC, +}; +use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, +}; +use windows_sys::Win32::System::WindowsProgramming::{ + FILE_RENAME_FLAG_POSIX_SEMANTICS, FILE_RENAME_FLAG_REPLACE_IF_EXISTS, +}; + +use super::{LOCK_FILE, REGISTRATION_FILE, Registration, registration_path, sweep}; +use crate::protocol::PROTOCOL_VERSION; +use crate::transport::Listener; +use crate::transport::windows::{owned_handle, security::PrivateSecurity}; + +pub(super) fn read_registration(directory: &Path) -> Result { + let security = PrivateSecurity::new()?; + let mut file = open( + ®istration_path(directory), + GENERIC_READ | READ_CONTROL, + OPEN_EXISTING, + FILE_SHARE_READ | FILE_SHARE_DELETE, + &security, + ) + .context("opencode-pty registration is unavailable")?; + check_kind(&file, false)?; + security.check_private(file.as_raw_handle())?; + let mut data = Vec::new(); + // Registration is tiny; do not allocate an unbounded stale/corrupt file. + Read::by_ref(&mut file) + .take(16 * 1024 + 1) + .read_to_end(&mut data)?; + if data.len() > 16 * 1024 { + bail!("PTY registration is too large"); + } + serde_json::from_slice(&data).context("invalid opencode-pty registration") +} + +pub(super) struct Runtime { + pub registration: Registration, + pub cleanup: Cleanup, + sweeper: JoinHandle<()>, + // Neither handle shares delete access, so the private directory and lock + // cannot be replaced while the daemon runs. + lock: File, + directory: File, +} + +impl Runtime { + pub fn bind(directory: &Path) -> Result<(Self, Listener)> { + if !directory.is_absolute() { + bail!("PTY runtime directory must be an absolute path"); + } + let security = PrivateSecurity::new()?; + if let Some(parent) = directory.parent() { + fs::create_dir_all(parent)?; + } + let wide = wide_path(directory)?; + // SAFETY: wide path and private descriptor are valid through creation. + if unsafe { CreateDirectoryW(wide.as_ptr(), &security.attributes()) } == 0 { + let error = std::io::Error::last_os_error(); + if error.raw_os_error() != Some(ERROR_ALREADY_EXISTS as i32) { + return Err(error.into()); + } + } + let directory_handle = open( + directory, + FILE_READ_ATTRIBUTES | FILE_TRAVERSE | READ_CONTROL | WRITE_DAC, + OPEN_EXISTING, + FILE_SHARE_READ | FILE_SHARE_WRITE, + &security, + )?; + check_kind(&directory_handle, true)?; + security.make_private(directory_handle.as_raw_handle())?; + // All later paths come from the held handle, never the caller's path. + let directory = held_directory_path(&directory_handle)?; + let lock = open( + &directory.join(LOCK_FILE), + GENERIC_READ | GENERIC_WRITE | READ_CONTROL | WRITE_DAC, + OPEN_ALWAYS, + FILE_SHARE_READ | FILE_SHARE_WRITE, + &security, + )?; + check_kind(&lock, false)?; + security.make_private(lock.as_raw_handle())?; + lock.try_lock_exclusive() + .context("another opencode-pty process already owns the service lock")?; + + let instance_id = format!("{:032x}", rand::random::()); + let registration = Registration { + socket: PathBuf::from(format!(r"\\.\pipe\opencode-pty-{instance_id}")), + instance_id, + pid: std::process::id(), + protocol: PROTOCOL_VERSION, + token: format!("{:032x}", rand::random::()), + }; + let listener = Listener::bind(®istration.socket)?; + write_registration(&directory, &directory_handle, ®istration, &security)?; + let sweeper = { + let directory = directory.clone(); + thread::spawn(move || sweep::stale_runtimes(&directory)) + }; + Ok(( + Self { + cleanup: Cleanup { + directory, + instance_id: registration.instance_id.clone(), + }, + registration, + sweeper, + lock, + directory: directory_handle, + }, + listener, + )) + } + + /// Removes this daemon's runtime state after the server has joined its + /// handlers. The caller keeps the pipe namespace until this returns. + pub fn finish(self) -> Result<()> { + let _ = self.sweeper.join(); + let result = self.cleanup.run(); + let directory = self.cleanup.directory; + // The protected handles deny deletion; release them first. A daemon that + // races in and locks a new lock file simply keeps the directory. + drop(self.lock); + drop(self.directory); + let _ = fs::remove_file(directory.join(LOCK_FILE)); + let _ = fs::remove_dir(&directory); + result + } +} + +/// Removes the registration only while it still belongs to this instance. +#[derive(Clone)] +pub(super) struct Cleanup { + directory: PathBuf, + instance_id: String, +} + +impl Cleanup { + pub fn run(&self) -> Result<()> { + if read_registration(&self.directory) + .is_ok_and(|current| current.instance_id == self.instance_id) + { + fs::remove_file(registration_path(&self.directory))?; + } + // A named pipe is not a filesystem entry. Its listener remains held until + // this registration cleanup has completed, then all handles are closed. + Ok(()) + } +} + +pub(super) fn process_exists(pid: u32) -> bool { + // SAFETY: a successful handle is adopted immediately; it is only waited on. + let handle = unsafe { OpenProcess(PROCESS_SYNCHRONIZE, 0, pid) }; + if handle.is_null() { + // Another user's live process cannot be opened; treat it as live. + return std::io::Error::last_os_error().raw_os_error() == Some(ERROR_ACCESS_DENIED as i32); + } + // SAFETY: OpenProcess returned a new, uniquely owned handle. + let handle = unsafe { OwnedHandle::from_raw_handle(handle) }; + // SAFETY: the owned process handle stays valid through this zero-timeout wait. + unsafe { WaitForSingleObject(handle.as_raw_handle(), 0) == WAIT_TIMEOUT } +} + +/// Named pipes vanish with their server process; there is no endpoint to remove. +pub(super) fn remove_stale_endpoint(_canonical_directory: &Path) -> Result<()> { + Ok(()) +} + +fn write_registration( + directory: &Path, + directory_handle: &File, + registration: &Registration, + security: &PrivateSecurity, +) -> Result<()> { + let temporary = directory.join(format!("service.{}.tmp", registration.instance_id)); + let result = (|| -> Result<()> { + let mut file = open( + &temporary, + GENERIC_WRITE | READ_CONTROL | DELETE, + CREATE_NEW, + 0, + security, + )?; + file.write_all(&serde_json::to_vec_pretty(registration)?)?; + file.sync_all()?; + publish(&file, directory_handle) + })(); + if result.is_err() { + let _ = fs::remove_file(temporary); + } + result +} + +fn publish(file: &File, directory: &File) -> Result<()> { + // The Win32 wrapper rejects a non-null RootDirectory, unlike the native + // NtSetInformationFile interface. Resolve the full destination from the held + // directory handle, never from the unvalidated caller path. + let name = held_directory_path(directory)? + .join(REGISTRATION_FILE) + .as_os_str() + .encode_wide() + .collect::>(); + let name_bytes = u32::try_from(size_of_val(name.as_slice()))?; + let bytes = size_of::() + .checked_add(name_bytes as usize) + .context("registration rename buffer overflow")?; + let buffer_bytes = u32::try_from(bytes)?; + // FILE_RENAME_INFO has a variable-length trailing UTF-16 name. usize gives + // this buffer the struct's alignment on both supported 64-bit architectures. + const { + assert!(align_of::() <= align_of::()); + } + let mut buffer = vec![0_usize; bytes.div_ceil(size_of::())]; + let allocation = buffer.as_mut_ptr().cast::(); + let info = allocation.cast::(); + // SAFETY: the zeroed, aligned allocation covers the header and complete name. + // Use raw pointers into the whole allocation, not a borrow of FileName[1] + // whose extent is smaller than the variable-sized trailing data. + unsafe { + (&raw mut (*info).Anonymous).write(FILE_RENAME_INFO_0 { + Flags: FILE_RENAME_FLAG_REPLACE_IF_EXISTS | FILE_RENAME_FLAG_POSIX_SEMANTICS, + }); + (&raw mut (*info).RootDirectory).write(null_mut()); + (&raw mut (*info).FileNameLength).write(name_bytes); + let destination = allocation + .add(std::mem::offset_of!(FILE_RENAME_INFO, FileName)) + .cast::(); + std::ptr::copy_nonoverlapping(name.as_ptr(), destination, name.len()); + // Unlike ordinary MoveFileEx replacement, POSIX semantics preserve old + // reader handles while new opens see the new file, with no missing-name + // interval. Source and destination directory stay held throughout; the + // source is never reopened and no inherited ACL/metadata merge occurs. + if SetFileInformationByHandle( + file.as_raw_handle(), + FileRenameInfoEx, + info.cast(), + buffer_bytes, + ) == 0 + { + return Err(std::io::Error::last_os_error()) + .context("failed to atomically publish PTY registration"); + } + } + Ok(()) +} + +fn held_directory_path(directory: &File) -> Result { + // SAFETY: query the normalized DOS path length for a live directory handle. + let length = unsafe { GetFinalPathNameByHandleW(directory.as_raw_handle(), null_mut(), 0, 0) }; + if length == 0 { + return Err(std::io::Error::last_os_error().into()); + } + let mut path = vec![0_u16; length as usize]; + // SAFETY: the buffer has the requested length and the directory stays held. + let written = unsafe { + GetFinalPathNameByHandleW(directory.as_raw_handle(), path.as_mut_ptr(), length, 0) + }; + if written == 0 { + return Err(std::io::Error::last_os_error().into()); + } + if written >= length { + bail!("PTY runtime directory path changed during publication"); + } + Ok(PathBuf::from(OsString::from_wide( + &path[..written as usize], + ))) +} + +fn open( + path: &Path, + access: u32, + creation: u32, + sharing: u32, + security: &PrivateSecurity, +) -> Result { + let path = wide_path(path)?; + // SAFETY: all pointers are valid; OPEN_REPARSE_POINT lets check_kind reject + // links rather than inspecting a different target after following them. + let handle = unsafe { + CreateFileW( + path.as_ptr(), + access, + sharing, + &security.attributes(), + creation, + FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, + null_mut(), + ) + }; + Ok(File::from(owned_handle(handle)?)) +} + +fn check_kind(file: &File, directory: bool) -> Result<()> { + let mut info = BY_HANDLE_FILE_INFORMATION::default(); + // SAFETY: file owns a live handle; info is valid writable output. + if unsafe { GetFileInformationByHandle(file.as_raw_handle(), &mut info) } == 0 { + return Err(std::io::Error::last_os_error().into()); + } + if info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + bail!("PTY storage must not be a reparse point"); + } + if (info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0) != directory { + bail!("unexpected PTY storage file type"); + } + Ok(()) +} + +fn wide_path(path: &Path) -> Result> { + let mut wide = path.as_os_str().encode_wide().collect::>(); + if wide.contains(&0) { + bail!("PTY storage path contains NUL"); + } + wide.push(0); + Ok(wide) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn registration_is_private_and_atomically_replaceable_with_a_reader_open() { + let directory = + std::env::temp_dir().join(format!("pty-registration-{:032x}", rand::random::())); + fs::create_dir(&directory).unwrap(); + let security = PrivateSecurity::new().unwrap(); + let directory_handle = open( + &directory, + FILE_READ_ATTRIBUTES | FILE_TRAVERSE, + OPEN_EXISTING, + FILE_SHARE_READ | FILE_SHARE_WRITE, + &security, + ) + .unwrap(); + let mut registration = Registration { + instance_id: "first".into(), + pid: 1, + protocol: PROTOCOL_VERSION, + socket: PathBuf::from(r"\\.\pipe\opencode-pty-first"), + token: "secret".into(), + }; + write_registration(&directory, &directory_handle, ®istration, &security).unwrap(); + let path = directory.join(REGISTRATION_FILE); + let mut old_reader = open( + &path, + GENERIC_READ | READ_CONTROL, + OPEN_EXISTING, + FILE_SHARE_READ | FILE_SHARE_DELETE, + &security, + ) + .unwrap(); + security.check_private(old_reader.as_raw_handle()).unwrap(); + registration.instance_id = "second".into(); + write_registration(&directory, &directory_handle, ®istration, &security).unwrap(); + assert_eq!(read_registration(&directory).unwrap().instance_id, "second"); + let mut old = String::new(); + old_reader.read_to_string(&mut old).unwrap(); + assert_eq!( + serde_json::from_str::(&old) + .unwrap() + .instance_id, + "first" + ); + drop(old_reader); + drop(directory_handle); + fs::remove_dir_all(directory).unwrap(); + } + + #[test] + fn process_liveness_uses_the_process_object() { + assert!(process_exists(std::process::id())); + let mut child = std::process::Command::new("cmd.exe") + .args(["/C", "exit 0"]) + .spawn() + .unwrap(); + let pid = child.id(); + child.wait().unwrap(); + assert!(!process_exists(pid)); + } +} diff --git a/src/lib.rs b/src/lib.rs index 363d583..7ab76b4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,7 +3,7 @@ pub mod daemon; mod ghostty; #[cfg(all(target_os = "linux", target_env = "musl"))] mod musl; -#[cfg(unix)] +#[cfg(any(unix, windows))] mod ownership; pub mod protocol; pub mod service; diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 81d0809..e7a7873 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -18,7 +18,7 @@ pub(crate) use unix::{Cancellation, Connection, Listener}; #[cfg(windows)] mod retained; -// The backend is exercised natively before the daemon entrypoint is enabled. #[cfg(windows)] -#[cfg_attr(not(test), allow(dead_code))] pub(crate) mod windows; +#[cfg(windows)] +pub(crate) use windows::{Cancellation, Connection, Listener}; diff --git a/src/transport/windows.rs b/src/transport/windows.rs index 1b177b2..f2762d7 100644 --- a/src/transport/windows.rs +++ b/src/transport/windows.rs @@ -70,6 +70,8 @@ impl Listener { let previous = std::mem::replace(&mut self.pending, next); Ok(Some(Connection { pipe: Arc::clone(&previous.operation.pipe), + read_timeout: None, + write_timeout: None, })) } @@ -159,8 +161,10 @@ impl Pipe { } } -pub(crate) struct Connection { +pub struct Connection { pipe: Arc, + read_timeout: Option, + write_timeout: Option, } impl Connection { @@ -186,6 +190,8 @@ impl Connection { Ok(handle) => { return Ok(Self { pipe: Arc::new(Pipe::new(handle)?), + read_timeout: None, + write_timeout: None, }); } Err(error) if error.raw_os_error() == Some(ERROR_PIPE_BUSY as i32) => { @@ -205,14 +211,22 @@ impl Connection { } } - pub fn cancellation(&self) -> io::Result { + pub fn set_read_timeout(&mut self, timeout: Option) { + self.read_timeout = timeout; + } + + pub fn set_write_timeout(&mut self, timeout: Option) { + self.write_timeout = timeout; + } + + pub(crate) fn cancellation(&self) -> io::Result { Ok(Cancellation(Arc::clone(&self.pipe))) } /// Protocol 7 has an explicit response/final event, not a pipe half-close. /// Retain the pipe until the client reads that frame and closes its end. /// An uncooperative peer is cancelled after a bounded grace period. - pub fn finish_response(&mut self) -> io::Result<()> { + pub(crate) fn finish_response(&mut self) -> io::Result<()> { match self.read_with_timeout(&mut [0], Some(COMPLETION_TIMEOUT)) { Ok(0) => Ok(()), Ok(_) => Err(io::Error::new( @@ -226,9 +240,11 @@ impl Connection { } } - pub fn monitor_disconnect(&self) -> io::Result { + pub(crate) fn monitor_disconnect(&self) -> io::Result { let mut reader = Self { pipe: Arc::clone(&self.pipe), + read_timeout: None, + write_timeout: None, }; let cancellation = self.cancellation()?; let (sender, disconnected) = bounded(1); @@ -282,7 +298,7 @@ impl Connection { impl Read for Connection { fn read(&mut self, bytes: &mut [u8]) -> io::Result { - self.read_with_timeout(bytes, None) + self.read_with_timeout(bytes, self.read_timeout) } } @@ -307,7 +323,7 @@ impl Write for Connection { })?; Ok(match ready { Some(count) => count, - None => operation.wait(None)?, + None => operation.wait(self.write_timeout)?, } as usize) } diff --git a/src/transport/windows/security.rs b/src/transport/windows/security.rs index 369c76f..57f8f37 100644 --- a/src/transport/windows/security.rs +++ b/src/transport/windows/security.rs @@ -2,12 +2,16 @@ use std::io; use std::os::windows::io::AsRawHandle; use std::ptr::null_mut; -use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::Foundation::{HANDLE, LocalFree}; use windows_sys::Win32::Security::Authorization::{ - ConvertSidToStringSidW, ConvertStringSecurityDescriptorToSecurityDescriptorW, SDDL_REVISION_1, + ConvertSidToStringSidW, ConvertStringSecurityDescriptorToSecurityDescriptorW, GetSecurityInfo, + SDDL_REVISION_1, SE_FILE_OBJECT, SetSecurityInfo, }; use windows_sys::Win32::Security::{ - GetTokenInformation, SECURITY_ATTRIBUTES, TOKEN_QUERY, TOKEN_USER, TokenUser, + ACCESS_ALLOWED_ACE, ACE_HEADER, DACL_SECURITY_INFORMATION, EqualSid, GetAce, + GetSecurityDescriptorControl, GetSecurityDescriptorDacl, GetSecurityDescriptorOwner, + GetTokenInformation, OWNER_SECURITY_INFORMATION, PROTECTED_DACL_SECURITY_INFORMATION, + SE_DACL_PROTECTED, SECURITY_ATTRIBUTES, TOKEN_QUERY, TOKEN_USER, TokenUser, }; use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; @@ -46,6 +50,123 @@ impl PrivateSecurity { bInheritHandle: 0, } } + + /// Existing files/directories are never adopted from a different owner. + /// Apply the protected current-user DACL before any secrets are published. + pub fn make_private(&self, handle: HANDLE) -> io::Result<()> { + self.check_owner(handle)?; + let mut present = 0; + let mut defaulted = 0; + let mut dacl = null_mut(); + // SAFETY: self owns a valid descriptor and returned DACL remains inside it. + if unsafe { GetSecurityDescriptorDacl(self.0, &mut present, &mut dacl, &mut defaulted) } + == 0 + { + return Err(io::Error::last_os_error()); + } + // SAFETY: the live handle is opened with WRITE_DAC; dacl is valid above. + let error = unsafe { + SetSecurityInfo( + handle, + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + null_mut(), + null_mut(), + dacl, + null_mut(), + ) + }; + if error != 0 { + return Err(io::Error::from_raw_os_error(error as i32)); + } + Ok(()) + } + + pub fn check_private(&self, handle: HANDLE) -> io::Result<()> { + let actual = self.check_owner(handle)?; + let mut present = 0; + let mut defaulted = 0; + let mut dacl = null_mut(); + let mut control = 0; + let mut revision = 0; + // SAFETY: actual owns a valid Windows descriptor throughout inspection. + if unsafe { GetSecurityDescriptorDacl(actual.0, &mut present, &mut dacl, &mut defaulted) } + == 0 + || unsafe { GetSecurityDescriptorControl(actual.0, &mut control, &mut revision) } == 0 + { + return Err(io::Error::last_os_error()); + } + if present == 0 || dacl.is_null() || control & SE_DACL_PROTECTED == 0 { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "unprotected PTY storage DACL", + )); + } + let mut ace = null_mut(); + // SAFETY: dacl is non-null and belongs to the live actual descriptor. + if unsafe { (*dacl).AceCount } != 1 || unsafe { GetAce(dacl, 0, &mut ace) } == 0 { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "PTY storage must grant only the current user", + )); + } + // ACCESS_ALLOWED_ACE_TYPE is zero. Other ACE layouts must not be cast. + // SAFETY: Windows returned an ACE from a validated security descriptor. + if unsafe { (*ace.cast::()).AceType } != 0 { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "unexpected PTY storage ACE", + )); + } + // SAFETY: an ACCESS_ALLOWED_ACE has an inline SID beginning at SidStart. + let sid = unsafe { (&raw mut (*ace.cast::()).SidStart).cast() }; + if !self.is_current_user(sid)? { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "PTY storage grants another principal", + )); + } + Ok(()) + } + + fn check_owner(&self, handle: HANDLE) -> io::Result { + let mut owner = null_mut(); + let mut descriptor = null_mut(); + // SAFETY: query a live file handle. Descriptor is a unique LocalFree allocation. + let error = unsafe { + GetSecurityInfo( + handle, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + &mut owner, + null_mut(), + null_mut(), + null_mut(), + &mut descriptor, + ) + }; + if error != 0 { + return Err(io::Error::from_raw_os_error(error as i32)); + } + let actual = Self(descriptor); + if owner.is_null() || !self.is_current_user(owner)? { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "PTY storage is not owned by the current user", + )); + } + Ok(actual) + } + + fn is_current_user(&self, sid: *mut core::ffi::c_void) -> io::Result { + let mut owner = null_mut(); + let mut defaulted = 0; + // SAFETY: self owns the descriptor; supplied SID belongs to another live descriptor. + if unsafe { GetSecurityDescriptorOwner(self.0, &mut owner, &mut defaulted) } == 0 { + return Err(io::Error::last_os_error()); + } + Ok(unsafe { EqualSid(owner, sid) } != 0) + } } impl Drop for PrivateSecurity { @@ -106,3 +227,61 @@ fn current_user_sid() -> io::Result { }; Ok(sid) } + +#[cfg(test)] +mod tests { + use super::*; + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::Foundation::{GENERIC_READ, GENERIC_WRITE}; + use windows_sys::Win32::Storage::FileSystem::{ + CREATE_NEW, CreateFileW, READ_CONTROL, WRITE_DAC, + }; + + #[test] + fn rejects_public_storage_acl_and_privatises_only_our_owned_file() { + let path = std::env::temp_dir().join(format!("pty-acl-{:032x}", rand::random::())); + let wide = path + .as_os_str() + .encode_wide() + .chain([0]) + .collect::>(); + let sid = current_user_sid().unwrap(); + let public = format!("O:{sid}D:P(A;;GA;;;WD)\0") + .encode_utf16() + .collect::>(); + let mut descriptor = null_mut(); + // SAFETY: valid SDDL; allocation is immediately adopted by the RAII owner. + assert_ne!( + unsafe { + ConvertStringSecurityDescriptorToSecurityDescriptorW( + public.as_ptr(), + SDDL_REVISION_1, + &mut descriptor, + null_mut(), + ) + }, + 0 + ); + let public = PrivateSecurity(descriptor); + // SAFETY: a unique temporary file, live private owner + deliberately public + // test DACL. No secrets are written while the DACL is permissive. + let file = owned_handle(unsafe { + CreateFileW( + wide.as_ptr(), + GENERIC_READ | GENERIC_WRITE | READ_CONTROL | WRITE_DAC, + 0, + &public.attributes(), + CREATE_NEW, + 0, + null_mut(), + ) + }) + .unwrap(); + let private = PrivateSecurity::new().unwrap(); + assert!(private.check_private(file.as_raw_handle()).is_err()); + private.make_private(file.as_raw_handle()).unwrap(); + private.check_private(file.as_raw_handle()).unwrap(); + drop(file); + std::fs::remove_file(path).unwrap(); + } +} diff --git a/tests/windows-daemon.rs b/tests/windows-daemon.rs new file mode 100644 index 0000000..31aede9 --- /dev/null +++ b/tests/windows-daemon.rs @@ -0,0 +1,389 @@ +#![cfg(windows)] + +// Direct-service and daemon tests share the runtime lane's real child fixture, +// rather than maintaining separate console setup. +#[path = "support/terminal_fixture.rs"] +mod terminal_fixture; + +use std::io::Write; +use std::os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::thread; +use std::time::{Duration, Instant}; + +use base64::Engine; +use opencode_pty::daemon::{PipeConnection, Registration}; +use opencode_pty::protocol::{ + AttachmentRole, Envelope, Request, Response, SubscriptionEvent, read_frame, + read_subscription_event, write_frame, +}; +use opencode_pty::service::{CreateTerminal, TerminalInfo}; +use terminal_fixture::{Command as FixtureCommand, Deadline, Fixture, TempDir}; +use windows_sys::Win32::Foundation::WAIT_OBJECT_0; +use windows_sys::Win32::System::Threading::{ + OpenProcess, PROCESS_SYNCHRONIZE, WaitForSingleObject, +}; + +struct Daemon { + child: Child, + registration: Registration, + directory: PathBuf, + root: TempDir, +} + +impl Daemon { + fn start() -> Self { + let root = TempDir::new(); + let directory = root.0.join("runtime"); + let mut child = spawn(&directory); + let registration = registration(&mut child, &directory); + Self { + child, + registration, + directory, + root, + } + } + + fn connect(&self) -> PipeConnection { + let mut stream = PipeConnection::connect(&self.registration.socket).unwrap(); + stream.set_read_timeout(Some(Duration::from_secs(5))); + stream.set_write_timeout(Some(Duration::from_secs(5))); + stream + } + + fn send(&self, stream: &mut PipeConnection, request: Request) -> Response { + write_frame( + &mut *stream, + &Envelope { + token: self.registration.token.clone(), + request, + }, + ) + .unwrap(); + read_frame(stream).unwrap() + } + + fn request(&self, request: Request) -> Response { + self.send(&mut self.connect(), request) + } + + fn own(&self, ticket: Option) -> (PipeConnection, Response) { + let mut stream = self.connect(); + let response = self.send( + &mut stream, + Request::Own { + instance_id: self.registration.instance_id.clone(), + ticket, + }, + ); + (stream, response) + } + + fn create(&self, fixture: &Fixture) -> TerminalInfo { + let CreateTerminal { + program, + args, + cwd, + title, + group_id, + env, + cols, + rows, + } = fixture.request(); + match self.request(Request::Create { + program, + args, + cwd, + title, + group_id, + env, + cols, + rows, + }) { + Response::Created { terminal } => terminal, + response => panic!("create: {response:?}"), + } + } + + fn subscribe(&self, id: u64, role: AttachmentRole) -> PipeConnection { + let mut stream = self.connect(); + assert!(matches!( + self.send( + &mut stream, + Request::Subscribe { + id, + offset: 0, + attachment_id: "daemon-test".into(), + role, + takeover: false, + } + ), + Response::Attached { .. } + )); + stream + } + + fn wait(&mut self) { + let status = wait(&mut self.child); + assert!(status.success(), "daemon failed: {status}"); + assert!( + !self.directory.exists(), + "runtime directory was not removed" + ); + assert!(PipeConnection::connect(&self.registration.socket).is_err()); + } +} + +impl Drop for Daemon { + fn drop(&mut self) { + if self.child.try_wait().ok().flatten().is_none() { + let _ = self.child.kill(); + let _ = self.child.wait(); + } + } +} + +fn spawn(directory: &Path) -> Child { + Command::new(env!("CARGO_BIN_EXE_opencode-pty")) + .args(["daemon", "--name"]) + .arg(directory.file_name().unwrap()) + .arg("--runtime-dir") + .arg(directory.parent().unwrap()) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .spawn() + .unwrap() +} + +fn registration(child: &mut Child, directory: &Path) -> Registration { + let deadline = Instant::now() + Duration::from_secs(4); + loop { + if let Ok(data) = std::fs::read(directory.join("service.json")) + && let Ok(registration) = serde_json::from_slice::(&data) + && registration.pid == child.id() + { + assert_eq!(registration.protocol, 7); + assert_eq!( + registration.socket, + PathBuf::from(format!( + r"\\.\pipe\opencode-pty-{}", + registration.instance_id + )) + ); + return registration; + } + assert!( + child.try_wait().unwrap().is_none(), + "daemon exited before registering" + ); + assert!(Instant::now() < deadline, "daemon did not register"); + thread::sleep(Duration::from_millis(10)); + } +} + +fn wait(child: &mut Child) -> std::process::ExitStatus { + let deadline = Instant::now() + Duration::from_secs(8); + loop { + if let Some(status) = child.try_wait().unwrap() { + return status; + } + assert!(Instant::now() < deadline, "daemon did not exit"); + thread::sleep(Duration::from_millis(10)); + } +} + +#[test] +fn authenticated_ownership_handoff_and_shutdown_use_real_pipes() { + let _deadline = Deadline::new(); + let mut daemon = Daemon::start(); + let mut invalid = daemon.connect(); + write_frame( + &mut invalid, + &Envelope { + token: "wrong".into(), + request: Request::Ping, + }, + ) + .unwrap(); + assert!( + matches!(read_frame(&mut invalid).unwrap(), Response::Error { message } if message == "authentication failed") + ); + drop(invalid); + assert!(matches!( + daemon.request(Request::Own { + instance_id: "wrong".into(), + ticket: None + }), + Response::Error { .. } + )); + let (mut owner, response) = daemon.own(None); + assert!(matches!(response, Response::Owned)); + assert!(matches!(daemon.own(None).1, Response::Error { .. })); + let Response::Handoff { ticket, expires_at } = daemon.send(&mut owner, Request::PrepareHandoff) + else { + panic!("handoff response"); + }; + assert!( + matches!(daemon.send(&mut owner, Request::PrepareHandoff), Response::Handoff { ticket: repeated, expires_at: deadline } if repeated == ticket && deadline == expires_at) + ); + // A valid ticket takes over even while the previous owner is connected. + let (mut successor, response) = daemon.own(Some(ticket.clone())); + assert!(matches!(response, Response::Owned)); + assert!(matches!( + daemon.send(&mut owner, Request::Shutdown), + Response::Error { message } if message == "owner connection has been superseded" + )); + drop(owner); + assert!(matches!(daemon.own(Some(ticket)).1, Response::Error { .. })); + assert!(matches!( + daemon.request(Request::Ping), + Response::Pong { .. } + )); + assert!(matches!( + daemon.send(&mut successor, Request::Shutdown), + Response::Ok + )); + drop(successor); // Protocol completion, not Unix half-close. + daemon.wait(); +} + +#[test] +fn service_lock_and_stale_registration_are_instance_scoped() { + let _deadline = Deadline::new(); + let mut daemon = Daemon::start(); + let mut duplicate = spawn(&daemon.directory); + assert!(!wait(&mut duplicate).success()); + let old = daemon.registration.clone(); + daemon.child.kill().unwrap(); + daemon.child.wait().unwrap(); + assert!(daemon.directory.join("service.json").exists()); + daemon.child = spawn(&daemon.directory); + daemon.registration = registration(&mut daemon.child, &daemon.directory); + assert_ne!(old.instance_id, daemon.registration.instance_id); + assert_ne!(old.token, daemon.registration.token); + assert_ne!(old.socket, daemon.registration.socket); + let (owner, response) = daemon.own(None); + assert!(matches!(response, Response::Owned)); + assert!(matches!(daemon.request(Request::Shutdown), Response::Ok)); + drop(owner); + daemon.wait(); +} + +#[test] +fn unclaimed_daemon_cancels_a_partial_request() { + let _deadline = Deadline::new(); + let mut daemon = Daemon::start(); + let mut partial = daemon.connect(); + partial.write_all(&[0, 0]).unwrap(); + daemon.wait(); +} + +#[test] +fn named_pipe_daemon_create_input_output_resize_and_shutdown() { + let _deadline = Deadline::new(); + let mut daemon = Daemon::start(); + let (owner, response) = daemon.own(None); + assert!(matches!(response, Response::Owned)); + let fixture = Fixture::new(); + // Exercise a path containing spaces/Unicode using the shared child fixture. + let executable = daemon.root.executable("daemon fixture.exe"); + let mut request = fixture.request(); + request.program = executable.to_str().unwrap().into(); + let CreateTerminal { + program, + args, + cwd, + title, + group_id, + env, + cols, + rows, + } = request; + let Response::Created { terminal } = daemon.request(Request::Create { + program, + args, + cwd, + title, + group_id, + env, + cols, + rows, + }) else { + panic!("create response"); + }; + let mut child = fixture.connect(); + let mut subscription = daemon.subscribe(terminal.id, AttachmentRole::Observer); + child.command(FixtureCommand::Output("\x1b[2J\x1b[Hdaemon-output".into())); + let mut bytes = Vec::new(); + while !String::from_utf8_lossy(&bytes).contains("daemon-output") { + if let SubscriptionEvent::Output { bytes: output, .. } = + read_subscription_event(&mut subscription).unwrap() + { + bytes.extend(output); + } + } + let input = b"real-input"; + assert!(matches!( + daemon.request(Request::Write { + id: terminal.id, + attachment_id: None, + data_base64: base64::engine::general_purpose::STANDARD.encode(input) + }), + Response::Ok + )); + assert_eq!( + child.command(FixtureCommand::Read(input.len())), + serde_json::json!(input) + ); + assert!(matches!( + daemon.request(Request::Resize { + id: terminal.id, + attachment_id: None, + cols: 93, + rows: 31 + }), + Response::Ok + )); + assert_eq!( + child.command(FixtureCommand::Size), + serde_json::json!([93, 31]) + ); + assert!( + matches!(daemon.request(Request::Snapshot { id: terminal.id }), Response::Snapshot { text, .. } if text.contains("daemon-output")) + ); + // Natural-exit/EOF ordering belongs to the runtime cleanup milestone. This + // basic daemon test deliberately exercises live-child operations + shutdown. + drop(subscription); + assert!(matches!(daemon.request(Request::Shutdown), Response::Ok)); + drop(owner); + daemon.wait(); +} + +#[test] +fn owner_loss_cancels_blocked_subscriber_and_partial_request() { + let _deadline = Deadline::new(); + let mut daemon = Daemon::start(); + let (owner, response) = daemon.own(None); + assert!(matches!(response, Response::Owned)); + let fixture = Fixture::new(); + let terminal = daemon.create(&fixture); + let mut child = fixture.connect(); + // SAFETY: open the live child for waiting; PID is only used to obtain a + // stable process handle for this assertion, not as terminal identity. + let process = unsafe { OpenProcess(PROCESS_SYNCHRONIZE, 0, terminal.pid.unwrap()) }; + assert!(!process.is_null()); + let process = unsafe { OwnedHandle::from_raw_handle(process) }; + let _blocked = daemon.subscribe(terminal.id, AttachmentRole::Controller); + let mut partial = daemon.connect(); + partial.write_all(&[0, 0]).unwrap(); + child.command(FixtureCommand::Output("x".repeat(256 * 1024))); + thread::sleep(Duration::from_millis(100)); + drop(owner); + daemon.wait(); + assert_eq!( + unsafe { WaitForSingleObject(process.as_raw_handle(), 3000) }, + WAIT_OBJECT_0 + ); +} From 4453740e9819a1dd4cc3960039eec1ce9f79a23b Mon Sep 17 00:00:00 2001 From: James Long Date: Thu, 8 Oct 2026 20:02:43 +0000 Subject: [PATCH 2/2] fix(daemon): verify the Windows pipe server before trusting it A crashed daemon leaves its registration behind, and its pipe name is then free for another local user to create. PipeConnection::connect_to checks GetNamedPipeServerProcessId against the registered PID before a client sends its token or input, and the docs require integrations to do the same. Also treat a failed liveness wait as live in the stale-runtime sweep, read the registration through the owner/ACL-checked reader in daemon tests, document that connections must be discarded after a timeout, and describe Windows storage and its NTFS/POSIX-rename requirement in README and SPEC. --- README.md | 14 ++++++++++--- SPEC.md | 10 +++++++++ src/daemon/windows.rs | 5 +++-- src/transport/windows.rs | 44 ++++++++++++++++++++++++++++++++++++++-- tests/windows-daemon.rs | 7 ++++--- 5 files changed, 70 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 92e7425..17106d7 100644 --- a/README.md +++ b/README.md @@ -91,9 +91,17 @@ file and runtime directory after releasing their handles. The abandoned-sibling sweep is shared with Unix; it checks liveness through the registered process object and has no endpoint file to remove. +Registration publication uses `FileRenameInfoEx` with POSIX rename semantics, +so the runtime directory must be on a local NTFS volume (Windows 10 1709 or +later); there is no non-atomic fallback. + The Windows Rust `TerminalClient` and interactive CLI are not ported. Integrations can use protocol 7 directly and the minimal `daemon::PipeConnection` byte-stream -helper (connect plus optional read/write timeouts). +helper. Connect with `PipeConnection::connect_to(socket, pid)`, which rejects a +pipe whose server is not the registered daemon process: after a crash the stale +registration's pipe name is free for another local user to create. Integrations +in other languages must make the same `GetNamedPipeServerProcessId` check before +sending the token. Discard a connection after any read/write timeout. ## Architecture @@ -354,8 +362,8 @@ git tag v0.1.0 push origin v0.1.0 ``` -Windows artifacts are intentionally excluded until the persistent transport -uses named pipes. Platform signing will be added later. +Release artifacts do not yet include Windows builds. Platform signing will be +added later. ## Current Limits diff --git a/SPEC.md b/SPEC.md index 41f634b..aaf8142 100644 --- a/SPEC.md +++ b/SPEC.md @@ -130,6 +130,16 @@ minutes old, contain only registration files, and have either a lock it can acquire or, without a lock file, a valid registration whose PID no longer exists. Directories without that evidence, including empty ones, are kept. +On Windows the endpoint is a random per-instance named pipe instead of a +socket. The runtime directory, lock, and registration carry a protected +current-user-only ACL, and registration is atomically replaced with POSIX +rename semantics. Clients verify the registration's owner and ACL, and check +that the pipe server process matches the registered PID before sending the +token. On exit the daemon removes its registration if still its own, then the +lock file and runtime directory; the forced-exit watchdog removes only the +registration, leaving the rest to a later sweep. The sweep checks liveness +through the registered process object. + OpenCode chooses a fresh UUID runtime directory for each server, independent of the database. It starts the daemon only when the first terminal is created. Only an explicit restart handoff descriptor lets a replacement server reuse diff --git a/src/daemon/windows.rs b/src/daemon/windows.rs index dd5159b..7bce7d9 100644 --- a/src/daemon/windows.rs +++ b/src/daemon/windows.rs @@ -10,7 +10,7 @@ use std::thread::{self, JoinHandle}; use anyhow::{Context, Result, bail}; use fs2::FileExt; use windows_sys::Win32::Foundation::{ - ERROR_ACCESS_DENIED, ERROR_ALREADY_EXISTS, GENERIC_READ, GENERIC_WRITE, WAIT_TIMEOUT, + ERROR_ACCESS_DENIED, ERROR_ALREADY_EXISTS, GENERIC_READ, GENERIC_WRITE, WAIT_OBJECT_0, }; use windows_sys::Win32::Storage::FileSystem::{ BY_HANDLE_FILE_INFORMATION, CREATE_NEW, CreateDirectoryW, CreateFileW, DELETE, @@ -179,8 +179,9 @@ pub(super) fn process_exists(pid: u32) -> bool { } // SAFETY: OpenProcess returned a new, uniquely owned handle. let handle = unsafe { OwnedHandle::from_raw_handle(handle) }; + // A failed wait is treated as live, so the sweep never deletes on doubt. // SAFETY: the owned process handle stays valid through this zero-timeout wait. - unsafe { WaitForSingleObject(handle.as_raw_handle(), 0) == WAIT_TIMEOUT } + unsafe { WaitForSingleObject(handle.as_raw_handle(), 0) != WAIT_OBJECT_0 } } /// Named pipes vanish with their server process; there is no endpoint to remove. diff --git a/src/transport/windows.rs b/src/transport/windows.rs index f2762d7..b3ccd57 100644 --- a/src/transport/windows.rs +++ b/src/transport/windows.rs @@ -23,8 +23,9 @@ use windows_sys::Win32::Storage::FileSystem::{ }; use windows_sys::Win32::System::IO::{CancelIoEx, GetOverlappedResult, OVERLAPPED}; use windows_sys::Win32::System::Pipes::{ - ConnectNamedPipe, CreateNamedPipeW, PIPE_READMODE_BYTE, PIPE_REJECT_REMOTE_CLIENTS, - PIPE_TYPE_BYTE, PIPE_UNLIMITED_INSTANCES, PIPE_WAIT, WaitNamedPipeW, + ConnectNamedPipe, CreateNamedPipeW, GetNamedPipeServerProcessId, PIPE_READMODE_BYTE, + PIPE_REJECT_REMOTE_CLIENTS, PIPE_TYPE_BYTE, PIPE_UNLIMITED_INSTANCES, PIPE_WAIT, + WaitNamedPipeW, }; use windows_sys::Win32::System::Threading::{ CreateEventW, INFINITE, SetEvent, WaitForMultipleObjects, WaitForSingleObject, @@ -211,6 +212,33 @@ impl Connection { } } + /// Connects only if the pipe server is the registered daemon process. + /// + /// Pipe names are visible to other local users. After a daemon crash its + /// registration remains, and another user could create a pipe with the + /// stale name; the endpoint's protections only last while the daemon holds + /// it. Clients must verify the server before sending credentials or input. + pub fn connect_to(endpoint: &Path, server_pid: u32) -> io::Result { + let connection = Self::connect(endpoint)?; + let mut pid = 0; + // SAFETY: the connection owns a live pipe handle; pid is valid output. + if unsafe { GetNamedPipeServerProcessId(connection.pipe.handle.as_raw_handle(), &mut pid) } + == 0 + { + return Err(io::Error::last_os_error()); + } + if pid != server_pid { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "named pipe server is not the registered opencode-pty daemon", + )); + } + Ok(connection) + } + + /// A timed-out operation is cancelled but may already have transferred + /// some bytes, so the protocol stream is no longer framed reliably. + /// Discard the connection after any timeout error. pub fn set_read_timeout(&mut self, timeout: Option) { self.read_timeout = timeout; } @@ -688,6 +716,18 @@ mod tests { monitor.finish(); } + #[test] + fn verified_connect_requires_the_expected_server_process() { + let endpoint = endpoint(); + let mut listener = Listener::bind(&endpoint).unwrap(); + let _verified = Connection::connect_to(&endpoint, std::process::id()).unwrap(); + let _server = accept(&mut listener); + let error = Connection::connect_to(&endpoint, std::process::id().wrapping_add(4)) + .err() + .expect("a different server process must be rejected"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + } + #[test] fn remote_and_non_pipe_endpoints_are_rejected() { for endpoint in [ diff --git a/tests/windows-daemon.rs b/tests/windows-daemon.rs index 31aede9..25bd53a 100644 --- a/tests/windows-daemon.rs +++ b/tests/windows-daemon.rs @@ -47,7 +47,8 @@ impl Daemon { } fn connect(&self) -> PipeConnection { - let mut stream = PipeConnection::connect(&self.registration.socket).unwrap(); + let mut stream = + PipeConnection::connect_to(&self.registration.socket, self.registration.pid).unwrap(); stream.set_read_timeout(Some(Duration::from_secs(5))); stream.set_write_timeout(Some(Duration::from_secs(5))); stream @@ -160,8 +161,8 @@ fn spawn(directory: &Path) -> Child { fn registration(child: &mut Child, directory: &Path) -> Registration { let deadline = Instant::now() + Duration::from_secs(4); loop { - if let Ok(data) = std::fs::read(directory.join("service.json")) - && let Ok(registration) = serde_json::from_slice::(&data) + // The checked reader also proves the published file is private. + if let Ok(registration) = opencode_pty::daemon::read_registration(directory) && registration.pid == child.id() { assert_eq!(registration.protocol, 7);