diff --git a/Cargo.lock b/Cargo.lock index 72edf88f..ed210a6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8293,6 +8293,7 @@ dependencies = [ "flate2", "gix", "imara-diff", + "libc", "notify", "yas-fssync", ] diff --git a/UNSAFE.md b/UNSAFE.md index 086ebb72..9ec59a65 100644 --- a/UNSAFE.md +++ b/UNSAFE.md @@ -142,6 +142,10 @@ To prevent this, every long-lived service `Command::new()…spawn()` in [`crates `prctl(PR_SET_PDEATHSIG, …)` is async-signal-safe and is invoked inside the closure passed to `CommandExt::pre_exec`. The `unsafe` block is confined to the closure body — the outer `pdeathsig_hook()` function is safe. SIGTERM (not SIGKILL) is used so children can clean up their own resources before exiting. Both modules are `#[cfg(target_os = "linux")]`, so there is no cross-platform shim needed. +## Git fetch process-group kill in `git` + +`run_git` in [`crates/git/src/reads.rs`](crates/git/src/reads.rs) starts `git fetch` in its own process group (`CommandExt::process_group(0)`). On timeout or cancellation, `kill_git` calls `libc::kill(-pid, SIGKILL)` so that what git started (ssh, a credential helper, a local upload-pack) dies with it, then reaps git. The pid comes from the still-unreaped child, so the group it names cannot have been recycled. Unix only; on Windows only git itself is killed. + ## `sd_notify` socket address in `sd-notify` [`crates/sd-notify/src/lib.rs`](crates/sd-notify/src/lib.rs) constructs a `sockaddr_un` by hand and calls `libc::socket` / `libc::sendto` to deliver `READY=1` to systemd. We do not link `libsystemd`. The whole module is `#[cfg(target_os = "linux")]` (other OSes get a no-op stub) since systemd is Linux-only and constants like `SOCK_CLOEXEC` and `MSG_NOSIGNAL` aren't portable to macOS. The unsafe surface is small and one-shot: build `sockaddr_un`, send one datagram, close the fd. The invariants: the path from `NOTIFY_SOCKET` is length-checked against `sun_path` before any byte is written so we never overflow the address buffer; only paths starting with `/` (filesystem) or `@` (Linux abstract namespace, leading byte rewritten to NUL) are accepted, anything else is silently dropped; the computed `addr_len` matches the documented kernel encoding for each form (sa_family + path bytes for abstract, sa_family + path bytes + trailing NUL for filesystem); the fd is opened with `SOCK_CLOEXEC` so it can't leak into PTY children; and every error path is best-effort — a missing `NOTIFY_SOCKET`, a truncated path, a failing `socket()`, or a failing `sendto()` all fall through to a normal return so the daemon never aborts on a notification failure. diff --git a/crates/git/Cargo.toml b/crates/git/Cargo.toml index eca01282..eaf65923 100644 --- a/crates/git/Cargo.toml +++ b/crates/git/Cargo.toml @@ -27,3 +27,6 @@ notify = "8" # `GIT binary patch` bodies are zlib streams, as git writes them. Already in # the tree via gix's own object store, so this adds an edge, not a crate. flate2 = "1" + +[target.'cfg(unix)'.dependencies] +libc = "0.2" diff --git a/crates/git/src/model.rs b/crates/git/src/model.rs index a7fe4057..d33f2447 100644 --- a/crates/git/src/model.rs +++ b/crates/git/src/model.rs @@ -20,6 +20,7 @@ pub const GIT_STATUS_CANCELLED: u8 = 8; pub const GIT_STATUS_OTHER: u8 = 9; pub const GIT_STATUS_CONFLICT: u8 = 11; pub const GIT_STATUS_NO_MERGE_BASE: u8 = 12; +pub const GIT_STATUS_TIMEOUT: u8 = 13; pub const GIT_OID_FORMAT_SHA1: u8 = 0; pub const GIT_OID_FORMAT_SHA256: u8 = 1; diff --git a/crates/git/src/native.rs b/crates/git/src/native.rs index fa1e1b9e..929cc8d8 100644 --- a/crates/git/src/native.rs +++ b/crates/git/src/native.rs @@ -27,6 +27,7 @@ pub enum Status { Invalid, Cancelled, Conflict, + Timeout, Other, } @@ -41,6 +42,7 @@ impl Status { engine::GIT_STATUS_INVALID => Self::Invalid, engine::GIT_STATUS_CANCELLED => Self::Cancelled, engine::GIT_STATUS_CONFLICT | engine::GIT_STATUS_NO_MERGE_BASE => Self::Conflict, + engine::GIT_STATUS_TIMEOUT => Self::Timeout, _ => Self::Other, } } diff --git a/crates/git/src/reads.rs b/crates/git/src/reads.rs index 950b904b..ea70f07e 100644 --- a/crates/git/src/reads.rs +++ b/crates/git/src/reads.rs @@ -9,13 +9,13 @@ use crate::model::{ GIT_FETCH_REF_TAG_UPDATE, GIT_FOUND_BARE, GIT_FOUND_LINKED, GIT_OID_NONE, GIT_REFLOG_OLDEST_FIRST, GIT_REFLOG_TRUNCATED, GIT_STATUS_CANCELLED, GIT_STATUS_INVALID, GIT_STATUS_NOT_FOUND, GIT_STATUS_OK, GIT_STATUS_OTHER, GIT_STATUS_PERMISSION, - GIT_STATUS_WRONG_TYPE, GIT_WORKTREE_BARE, GIT_WORKTREE_CURRENT, GIT_WORKTREE_DETACHED, - GIT_WORKTREE_LOCKED, GIT_WORKTREE_MAIN, GIT_WORKTREE_PRUNABLE, GIT_WORKTREES_TRUNCATED, - GitBlameRecord, GitBlameRequest, GitDiscoverRecord, GitFetchRecord, GitFetchRequest, - GitReflogRecord, GitReflogRequest, GitWorktreeRecord, GitWorktreesRequest, OwnedGitBlameRecord, - OwnedGitDiscoverRecord, OwnedGitFetchRecord, OwnedGitReflogRecord, OwnedGitWorktreeRecord, - Response, blame_response, discover_response, fetch_response, push_git_blame_record, - push_git_discover_record, push_git_fetch_record, push_git_reflog_record, + GIT_STATUS_TIMEOUT, GIT_STATUS_WRONG_TYPE, GIT_WORKTREE_BARE, GIT_WORKTREE_CURRENT, + GIT_WORKTREE_DETACHED, GIT_WORKTREE_LOCKED, GIT_WORKTREE_MAIN, GIT_WORKTREE_PRUNABLE, + GIT_WORKTREES_TRUNCATED, GitBlameRecord, GitBlameRequest, GitDiscoverRecord, GitFetchRecord, + GitFetchRequest, GitReflogRecord, GitReflogRequest, GitWorktreeRecord, GitWorktreesRequest, + OwnedGitBlameRecord, OwnedGitDiscoverRecord, OwnedGitFetchRecord, OwnedGitReflogRecord, + OwnedGitWorktreeRecord, Response, blame_response, discover_response, fetch_response, + push_git_blame_record, push_git_discover_record, push_git_fetch_record, push_git_reflog_record, push_git_worktree_record, reflog_response, worktrees_response, }; @@ -711,7 +711,12 @@ fn run_git( ) -> Result { use std::io::Read as _; use std::process::{Command, Stdio}; - let mut child = Command::new("git") + let mut command = Command::new("git"); + // Its own process group, so a kill reaches what git runs: ssh, a + // credential helper, a local upload-pack. + #[cfg(unix)] + std::os::unix::process::CommandExt::process_group(&mut command, 0); + let mut child = command .current_dir(dir) .args(args) .env("GIT_TERMINAL_PROMPT", "0") @@ -756,14 +761,12 @@ fn run_git( Err(_) => return Err(GIT_STATUS_OTHER), } if cancel.is_cancelled() { - let _ = child.kill(); - let _ = child.wait(); + kill_git(&mut child); return Err(GIT_STATUS_CANCELLED); } if std::time::Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); - return Err(GIT_STATUS_OTHER); + kill_git(&mut child); + return Err(GIT_STATUS_TIMEOUT); } std::thread::sleep(std::time::Duration::from_millis(20)); }; @@ -774,6 +777,19 @@ fn run_git( }) } +/// Kill git and the process group it leads, then reap it. +fn kill_git(child: &mut std::process::Child) { + #[cfg(unix)] + if let Ok(pid) = libc::pid_t::try_from(child.id()) { + // SAFETY: plain syscall on the process group this child leads. + unsafe { + libc::kill(-pid, libc::SIGKILL); + } + } + let _ = child.kill(); + let _ = child.wait(); +} + /// One `git fetch --porcelain` line: ` `, where /// flag is ` ` fast-forward, `+` forced, `-` pruned, `*` new, `!` rejected, /// `=` up to date. diff --git a/crates/git/tests/engine.rs b/crates/git/tests/engine.rs index 78a9d285..b94c26f3 100644 --- a/crates/git/tests/engine.rs +++ b/crates/git/tests/engine.rs @@ -514,6 +514,48 @@ fn semantic_fetch_reports_local_remote_ref_updates() { })); } +#[cfg(unix)] +#[test] +fn fetch_timeout_reports_timeout_and_kills_what_git_started() { + if !native::fetch_available() { + return; + } + let source = Repository::new(); + source.git(&["commit", "-q", "--allow-empty", "-m", "remote"]); + let consumer = Repository::new(); + consumer.git(&["remote", "add", "origin", &source.remote_path()]); + // The remote side stalls past the deadline, leaving its pid behind. + let pid_file = consumer.0.join("upload-pack.pid"); + consumer.git(&[ + "config", + "remote.origin.uploadpack", + &format!( + "echo $$ > '{}'; sleep 30; git-upload-pack", + pid_file.display() + ), + ]); + let (handle, _) = native::open_path(&consumer.0).unwrap(); + let error = handle + .native_fetch("origin", &[], 0, 500, &Cancel::default()) + .unwrap_err(); + assert_eq!(error.status, Status::Timeout); + + let pid = std::fs::read_to_string(&pid_file).unwrap(); + let alive = || { + Command::new("kill") + .args(["-0", pid.trim()]) + .stderr(std::process::Stdio::null()) + .status() + .unwrap() + .success() + }; + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while alive() && std::time::Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(20)); + } + assert!(!alive(), "upload-pack {} outlived the fetch", pid.trim()); +} + #[test] fn status_keeps_tracked_files_and_ancestors_observable_despite_ignore_rules() { for untracked in [false, true] { diff --git a/crates/server/src/yas_git.rs b/crates/server/src/yas_git.rs index 1790b7a9..0f93bb0b 100644 --- a/crates/server/src/yas_git.rs +++ b/crates/server/src/yas_git.rs @@ -790,6 +790,7 @@ fn native_error(error: yas_git::native::Failure) -> Error { yas_git::native::Status::ResourceExhausted => Status::ResourceExhausted, yas_git::native::Status::Cancelled => Status::Cancelled, yas_git::native::Status::Conflict => Status::Conflict, + yas_git::native::Status::Timeout => Status::Timeout, yas_git::native::Status::Other => Status::Internal, }, error.detail, @@ -805,6 +806,7 @@ fn native_status(status: yas_git::native::Status) -> Status { yas_git::native::Status::ResourceExhausted => Status::ResourceExhausted, yas_git::native::Status::Cancelled => Status::Cancelled, yas_git::native::Status::Conflict => Status::Conflict, + yas_git::native::Status::Timeout => Status::Timeout, yas_git::native::Status::Other => Status::Internal, } }