Don't redundantly unlink() temporary file after renaming

There is an unlikely issue if two shells are concurrently rewriting the
history file:
- fish A runs rename("fish_history.DEADBEEF") (rewriting a history file with)
- fish B starts rewriting the history file; since "fish_history.DEADBEEF" no longer exists, it can in theory use that filename
- fish A runs wunlink("fish_history.DEADBEEF"), destroying fish B's work

Fix this by not calling wunlink() iff we successfully rename it.

[ja: add commit message and fix "!do_save" case]
This commit is contained in:
Daniel Rainer
2025-08-08 17:30:26 +02:00
committed by Johannes Altmanninger
parent f59e5884bf
commit 188282a27b

View File

@@ -480,6 +480,18 @@ fn try_rewriting<F, UserData>(
let tmp_file_template = path.to_owned() + TMP_FILE_SUFFIX;
let (tmp_file, tmp_name) = create_temporary_file(&tmp_file_template)?;
let result = try_rewriting(path, rewrite, &tmp_name, tmp_file);
wunlink(&tmp_name);
// Do not leave the tmpfile around.
// Note that we do not unlink when renaming succeeded.
// In that case, it would be unnecessary, because the file will no longer exist at the path,
// but it also enables a race condition, where after renaming succeeded, a different fish
// instance creates a tmpfile with the same name (unlikely but possible), which we would then
// delete here.
if result.is_err()
|| result
.as_ref()
.is_ok_and(|(_file_id, potential_update)| !potential_update.do_save)
{
wunlink(&tmp_name);
}
result
}