Skip to content
Open
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
55 changes: 49 additions & 6 deletions crates/goose-cli/src/commands/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,10 @@ fn find_binary(extract_dir: &Path, binary_name: &str) -> Option<PathBuf> {
/// On Windows we must rename the running exe (Windows allows rename but not
/// delete/overwrite of a locked file) then copy the new file in.
///
/// On Unix we can simply copy over the existing binary.
/// On Unix, we write to a temp file in the same directory to avoid ETXTBSY
/// ("Text file busy") on Linux, which blocks writing to a running executable.
/// Renaming then atomically replaces the directory entry without affecting
/// the file currently in use by the kernel.
fn replace_binary(new_binary: &Path, current_exe: &Path) -> Result<()> {
#[cfg(target_os = "windows")]
{
Expand Down Expand Up @@ -462,18 +465,30 @@ fn replace_binary(new_binary: &Path, current_exe: &Path) -> Result<()> {

#[cfg(not(target_os = "windows"))]
{
// On Unix, copy the new binary over the existing one
fs::copy(new_binary, current_exe)
.with_context(|| format!("Failed to copy new binary to {}", current_exe.display()))?;
// Write to a temp file and avoids ETXTBSY ("Text file busy") on Linux
// where the kernel refuses to open write a running executable.
let dest_dir = current_exe
.parent()
.context("Current executable has no parent directory")?;

let tmp_path = dest_dir.join(format!("goose-update-{}", std::process::id()));

fs::copy(new_binary, &tmp_path)
.with_context(|| format!("Failed to write update to {}", tmp_path.display()))?;
Comment on lines +474 to +477
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Create temp update file with collision-safe semantics

Fresh evidence in this revision: tmp_path is now derived from goose-update-{pid}, which is still deterministic. If a file (or symlink) with that name already exists in the install directory—e.g., from PID reuse, a stale artifact, or a shared writable bin dir—fs::copy will overwrite it and the subsequent rename will remove that entry, causing unintended data loss and potentially writing through a symlink target. Use an atomic create_new/randomized temp filename in dest_dir to guarantee uniqueness before copying.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not deterministic across runs, since different processes ID will be assigned to it. The goose update command is a one-off so, I think it's grand using goose-update-<pid>.


// Ensure the binary is executable
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let mut perms = fs::metadata(current_exe)?.permissions();
let mut perms = fs::metadata(&tmp_path)?.permissions();
perms.set_mode(0o755);
fs::set_permissions(current_exe, perms)?;
fs::set_permissions(&tmp_path, perms)?;
}

fs::rename(&tmp_path, current_exe).with_context(|| {
let _ = fs::remove_file(&tmp_path);
format!("Failed to replace binary at {}", current_exe.display())
})?;
}

Ok(())
Expand Down Expand Up @@ -601,6 +616,34 @@ mod tests {
assert_eq!(content, "new version");
}

// On Unix the replacement must go through a rename so that it avoids
// ETXTBSY on Linux.
#[cfg(not(target_os = "windows"))]
#[test]
fn test_replace_binary_unix_no_leftover_tmp() {
let tmp = tempdir().unwrap();
let new_bin = tmp.path().join("new_goose");
let current = tmp.path().join("goose");

fs::write(&new_bin, b"new version").unwrap();
fs::write(&current, b"old version").unwrap();

replace_binary(&new_bin, &current).unwrap();

assert_eq!(fs::read_to_string(&current).unwrap(), "new version");

// No stray goose-update-* temp files should remain.
let leftovers: Vec<_> = fs::read_dir(tmp.path())
.unwrap()
.flatten()
.filter(|e| e.file_name().to_string_lossy().starts_with("goose-update-"))
.collect();
assert!(
leftovers.is_empty(),
"stray temp files left behind: {leftovers:?}"
);
}

#[cfg(target_os = "windows")]
#[test]
fn test_replace_binary_windows_rename_away() {
Expand Down
Loading