Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions UNSAFE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions crates/git/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
1 change: 1 addition & 0 deletions crates/git/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions crates/git/src/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub enum Status {
Invalid,
Cancelled,
Conflict,
Timeout,
Other,
}

Expand All @@ -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,
}
}
Expand Down
42 changes: 29 additions & 13 deletions crates/git/src/reads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -711,7 +711,12 @@ fn run_git(
) -> Result<GitOutput, u8> {
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")
Expand Down Expand Up @@ -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));
};
Expand All @@ -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: `<flag> <old> <new> <ref>`, where
/// flag is ` ` fast-forward, `+` forced, `-` pruned, `*` new, `!` rejected,
/// `=` up to date.
Expand Down
42 changes: 42 additions & 0 deletions crates/git/tests/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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] {
Expand Down
2 changes: 2 additions & 0 deletions crates/server/src/yas_git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
}
}
Expand Down
Loading