diff --git a/po/de.po b/po/de.po index e54002510..693ae6bb9 100644 --- a/po/de.po +++ b/po/de.po @@ -1025,8 +1025,8 @@ msgid "Error reading script file '%s':" msgstr "Die Script-Datei '%s' kann nicht gelesen werden:" #, c-format -msgid "Error when renaming history file: %s" -msgstr "Fehler beim Umbenennen der Verlaufsdatei: %s" +msgid "Error when renaming file: %s" +msgstr "" #, c-format msgid "Error while reading file %ls\n" @@ -1249,14 +1249,6 @@ msgstr "" msgid "List or remove functions" msgstr "Funktionen auflisten oder entfernen" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "Sperren der Geschichts-Datei dauerte zu lang (%.3f Sekunden)." - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "Die Datei für universelle Variablen zu sperren dauerte zu lange (%.3f Sekunden)" - msgid "Logical operations are not supported, use `test` instead" msgstr "Logikoperationen werden nicht unterstützt, nimm stattdessen `test`" @@ -1519,6 +1511,10 @@ msgstr "Derzeit ausgewertete Funktion stoppen" msgid "Stop the innermost loop" msgstr "Innerste Schleife beenden" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Ereignisweitergabe vorübergehend blockieren" @@ -1623,22 +1619,10 @@ msgstr "Konnte Verzeichnis %ls abgeleitet von %ls nicht finden: '%ls'." msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "Die Universal-Variablen-Datei '%s' kann nicht geöffnet werden: %s" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "Konnte Datei '%ls' nicht zu '%ls' umbenennen: %s" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "Die Universal-Variablen-Datei '%ls' kann nicht geschrieben werden: %s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" @@ -79192,6 +79176,30 @@ msgstr "" msgid "~ expansion" msgstr "" +#, c-format +#~ msgid "Error when renaming history file: %s" +#~ msgstr "Fehler beim Umbenennen der Verlaufsdatei: %s" + +#, c-format +#~ msgid "Locking the history file took too long (%.3f seconds)." +#~ msgstr "Sperren der Geschichts-Datei dauerte zu lang (%.3f Sekunden)." + +#, c-format +#~ msgid "Locking the universal var file took too long (%.3f seconds)." +#~ msgstr "Die Datei für universelle Variablen zu sperren dauerte zu lange (%.3f Sekunden)" + +#, c-format +#~ msgid "Unable to open universal variable file '%s': %s" +#~ msgstr "Die Universal-Variablen-Datei '%s' kann nicht geöffnet werden: %s" + +#, c-format +#~ msgid "Unable to rename file from '%ls' to '%ls': %s" +#~ msgstr "Konnte Datei '%ls' nicht zu '%ls' umbenennen: %s" + +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "Die Universal-Variablen-Datei '%ls' kann nicht geschrieben werden: %s" + #~ msgid "builtin\n" #~ msgstr "Eingebauter Befehl\n" diff --git a/po/en.po b/po/en.po index 679c205e6..8430cf7c9 100644 --- a/po/en.po +++ b/po/en.po @@ -1023,7 +1023,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1246,14 +1246,6 @@ msgstr "" msgid "List or remove functions" msgstr "List or remove functions" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1516,6 +1508,10 @@ msgstr "Stop the currently evaluated function" msgid "Stop the innermost loop" msgstr "Stop the innermost loop" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Temporarily block delivery of events" @@ -1620,22 +1616,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "Unable to locate the %ls directory." -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "Unable to write to universal variables file '%ls': %s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" @@ -79188,6 +79172,10 @@ msgstr "" msgid "~ expansion" msgstr "" +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "Unable to write to universal variables file '%ls': %s" + #~ msgid "empty" #~ msgstr "empty" diff --git a/po/fr.po b/po/fr.po index b52fd2017..15e123669 100644 --- a/po/fr.po +++ b/po/fr.po @@ -1124,7 +1124,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1347,14 +1347,6 @@ msgstr "" msgid "List or remove functions" msgstr "Lister ou supprimer des fonctions" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "Le verrouillage du fichier d’historique a pris trop de temps (%.3f secondes)." - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "Le verrouillage du fichier des variables universel a pris trop de temps (%.3f secondes)." - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1617,6 +1609,10 @@ msgstr "Arrêter la fonction actuellement en évaluation" msgid "Stop the innermost loop" msgstr "Arrêter la boucle interne" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Bloquer temporairement la distribution des événements" @@ -1721,22 +1717,10 @@ msgstr "Impossible de localiser le dossier %ls obtenu de $%ls: '%ls'." msgid "Unable to locate the %ls directory." msgstr "Impossible de localiser le dossier %ls." -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "Impossible de renommer le fichier '%ls' en '%ls': %s" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "Impossible d’écrire dans le fichier de variables universelles '%ls': %s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" @@ -79289,6 +79273,22 @@ msgstr "" msgid "~ expansion" msgstr "" +#, c-format +#~ msgid "Locking the history file took too long (%.3f seconds)." +#~ msgstr "Le verrouillage du fichier d’historique a pris trop de temps (%.3f secondes)." + +#, c-format +#~ msgid "Locking the universal var file took too long (%.3f seconds)." +#~ msgstr "Le verrouillage du fichier des variables universel a pris trop de temps (%.3f secondes)." + +#, c-format +#~ msgid "Unable to rename file from '%ls' to '%ls': %s" +#~ msgstr "Impossible de renommer le fichier '%ls' en '%ls': %s" + +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "Impossible d’écrire dans le fichier de variables universelles '%ls': %s" + #~ msgid "Check that this terminal type is supported on this system." #~ msgstr "Vérifiez que votre type de terminal est supporté sur ce système" diff --git a/po/pl.po b/po/pl.po index bb9077060..f94fd17a3 100644 --- a/po/pl.po +++ b/po/pl.po @@ -1019,7 +1019,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1242,14 +1242,6 @@ msgstr "" msgid "List or remove functions" msgstr "Wypisz lub usuń funkcje" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1512,6 +1504,10 @@ msgstr "Zatrzymaj obecnie używaną funkcję" msgid "Stop the innermost loop" msgstr "" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "" @@ -1616,22 +1612,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index 605a79ada..4db94d587 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -1024,7 +1024,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1247,14 +1247,6 @@ msgstr "" msgid "List or remove functions" msgstr "Lista ou remove funções" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1517,6 +1509,10 @@ msgstr "Pára a função em execução" msgid "Stop the innermost loop" msgstr "Pára o laço mais interno" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Bloqueia temporariamente a entrega de eventos" @@ -1621,22 +1617,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" diff --git a/po/sv.po b/po/sv.po index 2387b9804..d2feb7704 100644 --- a/po/sv.po +++ b/po/sv.po @@ -1020,7 +1020,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1243,14 +1243,6 @@ msgstr "" msgid "List or remove functions" msgstr "Visa eller ta bort funktioner" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1513,6 +1505,10 @@ msgstr "Avbryt den nuvarande funktionen" msgid "Stop the innermost loop" msgstr "Avbryt den innersta loopen" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Blockera tillfälligt leverans av händelser" @@ -1617,22 +1613,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index 29cdcad93..1e2a97b4d 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -1017,8 +1017,8 @@ msgid "Error reading script file '%s':" msgstr "读取脚本文件 '%s'出错: " #, c-format -msgid "Error when renaming history file: %s" -msgstr "重命名历史文件时出错:%s" +msgid "Error when renaming file: %s" +msgstr "" #, c-format msgid "Error while reading file %ls\n" @@ -1241,14 +1241,6 @@ msgstr "" msgid "List or remove functions" msgstr "列出或移除函数" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "锁定历史文件花费了太长时间(%.3f 秒)." - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "锁定 通用 var 文件花费了太长时间 (%.3f 秒) ." - msgid "Logical operations are not supported, use `test` instead" msgstr "不支持逻辑操作,使用`test`替代" @@ -1511,6 +1503,10 @@ msgstr "停止当前求值的函数" msgid "Stop the innermost loop" msgstr "停止最内层的循环" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Temporarily block delivery of events" @@ -1615,22 +1611,10 @@ msgstr "无法定位来自 $ls 的 %ls 目录: %ls ." msgid "Unable to locate the %ls directory." msgstr "无法找到 %ls 目录 ." -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "无法打开通用变量文件'%s': %s" - #, c-format msgid "Unable to read input file: %s" msgstr "无法读取输入文件:%s" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "无法将文件从 '%ls' 重命名为 '%ls':%s" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "无法写入通用变量文件 '%ls':%s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "未打开的括号' ' " @@ -79187,6 +79171,30 @@ msgstr "缩放" msgid "~ expansion" msgstr "~ 扩大" +#, c-format +#~ msgid "Error when renaming history file: %s" +#~ msgstr "重命名历史文件时出错:%s" + +#, c-format +#~ msgid "Locking the history file took too long (%.3f seconds)." +#~ msgstr "锁定历史文件花费了太长时间(%.3f 秒)." + +#, c-format +#~ msgid "Locking the universal var file took too long (%.3f seconds)." +#~ msgstr "锁定 通用 var 文件花费了太长时间 (%.3f 秒) ." + +#, c-format +#~ msgid "Unable to open universal variable file '%s': %s" +#~ msgstr "无法打开通用变量文件'%s': %s" + +#, c-format +#~ msgid "Unable to rename file from '%ls' to '%ls': %s" +#~ msgstr "无法将文件从 '%ls' 重命名为 '%ls':%s" + +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "无法写入通用变量文件 '%ls':%s" + #~ msgid "builtin\n" #~ msgstr "内建\n" diff --git a/src/env/environment.rs b/src/env/environment.rs index dee847260..dcfd11261 100644 --- a/src/env/environment.rs +++ b/src/env/environment.rs @@ -8,7 +8,6 @@ use crate::common::{str2wcstring, unescape_string, wcs2zstring, UnescapeStringStyle}; use crate::env::{EnvMode, EnvVar, Statuses}; use crate::env_dispatch::{env_dispatch_init, env_dispatch_var_change}; -use crate::env_universal_common::CallbackDataList; use crate::event::Event; use crate::flog::FLOG; use crate::global_safety::RelaxedAtomicBool; @@ -351,22 +350,23 @@ pub fn universal_sync(&self, always: bool) -> Vec { } UVARS_LOCALLY_MODIFIED.store(false); - let mut callbacks = CallbackDataList::new(); - let changed = uvars().sync(&mut callbacks); + let (changed, callbacks) = uvars().sync(); if changed { default_notifier().post_notification(); } // React internally to changes to special variables like LANG, and populate on-variable events. let mut result = Vec::new(); - for callback in callbacks { - let name = callback.key; - env_dispatch_var_change(&name, self); - let evt = if callback.val.is_none() { - Event::variable_erase(name) - } else { - Event::variable_set(name) - }; - result.push(evt); + if let Some(callbacks) = callbacks { + for callback in callbacks { + let name = callback.key; + env_dispatch_var_change(&name, self); + let evt = if callback.val.is_none() { + Event::variable_erase(name) + } else { + Event::variable_set(name) + }; + result.push(evt); + } } result } @@ -792,8 +792,7 @@ pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool UVAR_SCOPE_IS_GLOBAL.store(true); } else { // Set up universal variables using the default path. - let mut callbacks = CallbackDataList::new(); - uvars().initialize(&mut callbacks); + let callbacks = uvars().initialize().unwrap_or_default(); for callback in callbacks { env_dispatch_var_change(&callback.key, vars); } diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index b241591d6..e0b6e4578 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -1,41 +1,24 @@ #![allow(clippy::bad_bit_mask)] use crate::common::{ - str2wcstring, timef, unescape_string, valid_var_name, wcs2zstring, UnescapeFlags, - UnescapeStringStyle, + unescape_string, valid_var_name, wcs2zstring, UnescapeFlags, UnescapeStringStyle, }; use crate::env::{EnvVar, EnvVarFlags, VarTable}; -use crate::fallback::fish_mkstemp_cloexec; -use crate::fds::{open_cloexec, wopen_cloexec}; use crate::flog::{FLOG, FLOGF}; +use crate::fs::{lock_and_load, rewrite_via_temporary_file, PotentialUpdate}; use crate::path::path_get_config; -use crate::path::{path_get_config_remoteness, DirRemoteness}; use crate::wchar::{decode_byte_from_char, prelude::*}; -use crate::wcstringutil::{join_strings, string_suffixes_string, LineIterator}; -use crate::wutil::{ - file_id_for_file, file_id_for_path, file_id_for_path_narrow, wdirname, wrealpath, wrename, - wstat, wunlink, FileId, INVALID_FILE_ID, -}; -use errno::{errno, Errno}; -use libc::{EINTR, LOCK_EX}; -use nix::{fcntl::OFlag, sys::stat::Mode}; +use crate::wcstringutil::{join_strings, LineIterator}; +use crate::wutil::{file_id_for_file, file_id_for_path_narrow, wrealpath, FileId, INVALID_FILE_ID}; use std::collections::hash_map::Entry; use std::collections::HashSet; use std::ffi::CString; use std::fs::File; use std::io::{Read, Write}; use std::mem::MaybeUninit; -use std::os::fd::AsRawFd; -use std::os::unix::prelude::MetadataExt; - -// Pull in the O_EXLOCK constant if it is defined, otherwise set it to 0. -#[cfg(any(apple, bsd))] -const O_EXLOCK: OFlag = OFlag::O_EXLOCK; - -#[cfg(not(any(apple, bsd)))] -const O_EXLOCK: OFlag = OFlag::empty(); /// Callback data, reflecting a change in universal variables. +#[derive(Clone, Debug, Eq, PartialEq)] pub struct CallbackData { // The name of the variable. pub key: WString, @@ -73,16 +56,21 @@ pub struct EnvUniversal { // Whether it's OK to save. This may be set to false if we discover that a future version of // fish wrote the uvars contents. + // Only update if last_read_file_id is updated as well. ok_to_save: bool, - // If true, attempt to flock the uvars file. - // This latches to false if the file is found to be remote, where flock may hang. - do_flock: bool, - // File id from which we last read. + // Only update if ok_to_save is updated as well. last_read_file_id: FileId, } +struct UniversalReadUpdate { + export_generation_increment: u64, + new_vars: VarTable, + callbacks: CallbackDataList, + ok_to_save: bool, +} + impl EnvUniversal { // Construct an empty universal variables. pub fn new() -> Self { @@ -93,7 +81,6 @@ pub fn new() -> Self { modified: Default::default(), export_generation: 1, ok_to_save: true, - do_flock: true, last_read_file_id: INVALID_FILE_ID, } } @@ -154,90 +141,93 @@ pub fn get_table(&self) -> &VarTable { /// Initialize this uvars for the default path. /// This should be called at most once on any given instance. - pub fn initialize(&mut self, callbacks: &mut CallbackDataList) { - // Set do_flock to false immediately if the default variable path is on a remote filesystem. - // See #7968. - if path_get_config_remoteness() == DirRemoteness::remote { - self.do_flock = false; - } - self.initialize_at_path(callbacks, default_vars_path()); + pub fn initialize(&mut self) -> Option { + self.initialize_at_path(default_vars_path()) } /// Initialize a this uvars for a given path. /// This is exposed for testing only. - pub fn initialize_at_path(&mut self, callbacks: &mut CallbackDataList, path: WString) { + pub fn initialize_at_path(&mut self, path: WString) -> Option { if path.is_empty() { - return; + return None; } assert!(!self.initialized(), "Already initialized"); self.vars_path = path; - if self.load_from_path(callbacks) { - // Successfully loaded from our normal path. - } + self.load_from_path() } - /// Reads and writes variables at the correct path. Returns true if modified variables were - /// written. - pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { + /// Reads and writes variables at the correct path. + /// If there is an existing variables file with an unknown format, parsing it is attempted, + /// but it will not be overwritten. + /// Returns whether data was read, and the callbacks. + pub fn sync(&mut self) -> (bool, Option) { if !self.initialized() { - return false; + return (false, None); } FLOG!(uvar_file, "universal log sync"); - // Our saving strategy: - // - // 1. Open the file, producing an fd. - // 2. Lock the file (may be combined with step 1 on systems with O_EXLOCK) - // 3. After taking the lock, check if the file at the given path is different from what we - // opened. If so, start over. - // 4. Read from the file. This can be elided if its dev/inode is unchanged since the last read - // 5. Open an adjacent temporary file - // 6. Write our changes to an adjacent file - // 7. Move the adjacent file into place via rename. This is assumed to be atomic. - // 8. Release the lock and close the file - // - // Consider what happens if Process 1 and 2 both do this simultaneously. Can there be data loss? - // Process 1 opens the file and then attempts to take the lock. Now, either process 1 will see - // the original file, or process 2's new file. If it sees the new file, we're OK: it's going to - // read from the new file, and so there's no data loss. If it sees the old file, then process 2 - // must have locked it (if process 1 locks it, switch their roles). The lock will block until - // process 2 reaches step 7; at that point process 1 will reach step 2, notice that the file has - // changed, and then start over. - // - // It's possible that the underlying filesystem does not support locks (lockless NFS). In this - // case, we risk data loss if two shells try to write their universal variables simultaneously. - // In practice this is unlikely, since uvars are usually written interactively. - // - // Prior versions of fish used a hard link scheme to support file locking on lockless NFS. The - // risk here is that if the process crashes or is killed while holding the lock, future - // instances of fish will not be able to obtain it. This seems to be a greater risk than that of - // data loss on lockless NFS. Users who put their home directory on lockless NFS are playing - // with fire anyways. // If we have no changes, just load. if self.modified.is_empty() { - self.load_from_path_narrow(callbacks); + let callbacks = self.load_from_path_narrow(); FLOG!(uvar_file, "universal log no modifications"); - return false; + return (false, callbacks); } - let directory = wdirname(&self.vars_path).to_owned(); - FLOG!(uvar_file, "universal log performing full sync"); - // Open the file. - let Some(mut vars_file) = self.open_and_acquire_lock() else { - FLOG!(uvar_file, "universal log open_and_acquire_lock() failed"); - return false; + let rewrite = |old_file: &File, + tmp_file: &mut File| + -> std::io::Result>> { + match self.load_from_file(old_file) { + Some(potential_update) => { + if potential_update.do_save { + let contents = Self::serialize_with_vars(&potential_update.data.new_vars); + tmp_file.write_all(&contents)?; + } + Ok(PotentialUpdate { + do_save: potential_update.do_save, + data: Some(potential_update.data), + }) + } + None => { + if self.ok_to_save { + let contents = Self::serialize_with_vars(&self.vars); + tmp_file.write_all(&contents)?; + } + Ok(PotentialUpdate { + do_save: self.ok_to_save, + data: None, + }) + } + } }; - // Read from it. - self.load_from_file(&mut vars_file, callbacks); - - if self.ok_to_save { - self.save(&directory) - } else { - true + let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone()); + match rewrite_via_temporary_file(&real_path, rewrite) { + Ok((file_id, potential_update)) => { + self.last_read_file_id = file_id; + self.ok_to_save = potential_update.do_save; + self.modified.clear(); + match potential_update.data { + Some(UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save, + }) => { + assert_eq!(potential_update.do_save, ok_to_save); + self.export_generation += export_generation_increment; + self.vars = new_vars; + (true, Some(callbacks)) + } + None => (true, None), + } + } + Err(e) => { + FLOG!(uvar_file, "universal log sync failed:", e); + (false, None) + } } } @@ -364,38 +354,66 @@ fn initialized(&self) -> bool { !self.vars_path.is_empty() } - fn load_from_path(&mut self, callbacks: &mut CallbackDataList) -> bool { + fn load_from_path(&mut self) -> Option { self.narrow_vars_path = wcs2zstring(&self.vars_path); - self.load_from_path_narrow(callbacks) + self.load_from_path_narrow() } - fn load_from_path_narrow(&mut self, callbacks: &mut CallbackDataList) -> bool { + fn load_from_path_narrow(&mut self) -> Option { // Check to see if the file is unchanged. We do this again in load_from_file, but this avoids // opening the file unnecessarily. if self.last_read_file_id != INVALID_FILE_ID && file_id_for_path_narrow(&self.narrow_vars_path) == self.last_read_file_id { FLOG!(uvar_file, "universal log sync elided based on fast stat()"); - return true; + return None; } - let Ok(mut file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) - else { - return false; - }; - FLOG!(uvar_file, "universal log reading from file"); - self.load_from_file(&mut file, callbacks); - true + match lock_and_load(&self.vars_path, |f| { + Ok(self.load_from_file(f).map(|update| update.data)) + }) { + Ok(( + file_id, + Some(UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save, + }), + )) => { + self.export_generation += export_generation_increment; + self.vars = new_vars; + self.ok_to_save = ok_to_save; + self.last_read_file_id = file_id; + Some(callbacks) + } + Ok((_, None)) => { + // WARNING: Do not update self.file_id here, as this would interfere with + // self.ok_to_save. + None + } + Err(e) => { + FLOG!(uvar_file, "Failed to load from universal variable file:", e); + None + } + } } - // Load environment variables from the opened [`File`] `file`. It must be mutable because we - // will read from the underlying fd. - fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) { + /// Load environment variables from the [`file`](`File`). + /// If the file has not been modified since the last read, [`None`] is returned. + /// In this case, `self.ok_to_save` needs to be consulted to decide whether to rewrite the file. + /// Otherwise, the [`Some`] variant contains updated data and indicates whether the file should + /// be rewritten.. + // IMPORTANT: Callers of this code assume that a return value of None means that the file id has + // not changed. Do not return None in other situations without modifying the callers + // accordingly. Otherwise, problems with self.ok_to_save are expected to occur. + fn load_from_file(&self, file: &File) -> Option> { // Get the dev / inode. let current_file_id = file_id_for_file(file); if current_file_id == self.last_read_file_id { FLOG!(uvar_file, "universal log sync elided based on fstat()"); + None } else { // Read a variables table from the file. let mut new_vars = VarTable::new(); @@ -403,176 +421,36 @@ fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) // Hacky: if the read format is in the future, avoid overwriting the file: never try to // save. - if format == UvarFormat::future { - self.ok_to_save = false; - } + let do_save = format != UvarFormat::future; // Announce changes and update our exports generation. - self.generate_callbacks_and_update_exports(&new_vars, callbacks); + let (export_generation_increment, callbacks) = + self.generate_callbacks_and_update_exports(&new_vars); // Acquire the new variables. - self.acquire_variables(new_vars); - self.last_read_file_id = current_file_id; + self.acquire_variables(&mut new_vars); + Some(PotentialUpdate { + do_save, + data: UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save: do_save, + }, + }) } } - // Functions concerned with saving. - fn open_and_acquire_lock(&mut self) -> Option { - // Attempt to open the file for reading at the given path, atomically acquiring a lock. On BSD, - // we can use O_EXLOCK. On Linux, we open the file, take a lock, and then compare fstat() to - // stat(); if they match, it means that the file was not replaced before we acquired the lock. - // - // We pass O_RDONLY with O_CREAT; this creates a potentially empty file. We do this so that we - // have something to lock on. - let mut locked_by_open = false; - let mut flags = OFlag::O_RDWR | OFlag::O_CREAT; - - if !O_EXLOCK.is_empty() && self.do_flock { - flags |= O_EXLOCK; - locked_by_open = true; - } - - loop { - let mut file = - match wopen_cloexec(&self.vars_path, flags, Mode::from_bits_truncate(0o644)) { - Ok(file) => file, - Err(nix::Error::EINTR) => continue, - Err(err) => { - if !O_EXLOCK.is_empty() { - if flags.contains(O_EXLOCK) - && [nix::Error::ENOTSUP, nix::Error::EOPNOTSUPP].contains(&err) - { - // Filesystem probably does not support locking. Give up on locking. - // Note that on Linux the two errno symbols have the same value but on BSD they're - // different. - flags &= !O_EXLOCK; - self.do_flock = false; - locked_by_open = false; - continue; - } - } - FLOG!( - error, - wgettext_fmt!( - "Unable to open universal variable file '%s': %s", - &self.vars_path, - err.to_string() - ) - ); - return None; - } - }; - - // Lock if we want to lock and open() didn't do it for us. - // If flock fails, give up on locking forever. - if self.do_flock && !locked_by_open { - if !flock_uvar_file(&mut file) { - self.do_flock = false; - } - } - - // Hopefully we got the lock. However, it's possible the file changed out from under us - // while we were waiting for the lock. Make sure that didn't happen. - if file_id_for_file(&file) != file_id_for_path(&self.vars_path) { - // Oops, it changed! Try again. - drop(file); - } else { - return Some(file); - } - } - } - - fn open_temporary_file( - &mut self, - directory: &wstr, - out_path: &mut WString, - ) -> Result { - // Create and open a temporary file for writing within the given directory. Try to create a - // temporary file, up to 10 times. We don't use mkstemps because we want to open it CLO_EXEC. - // This should almost always succeed on the first try. - assert!(!string_suffixes_string(L!("/"), directory)); - - let mut attempt = 0; - let tmp_name_template = directory.to_owned() + L!("/fishd.tmp.XXXXXX"); - let result = loop { - attempt += 1; - let result = fish_mkstemp_cloexec(wcs2zstring(&tmp_name_template)); - match (result, attempt) { - (Ok(r), _) => break r, - (Err(e), 10) => { - FLOG!( - error, - // We previously used to log a copy of the buffer we expected mk(o)stemp to - // update with the new path, but mkstemp(3) says the contents of the buffer - // are undefined in case of EEXIST, but left unchanged in case of EINVAL. So - // just log the original template we pass in to the function instead. - wgettext_fmt!( - "Unable to create temporary file '%ls': %s", - &tmp_name_template, - e.to_string() - ) - ); - return Err(e); - } - _ => continue, - } - }; - - *out_path = str2wcstring(result.1.as_bytes()); - Ok(result.0) - } - - /// Writes our state to the fd. path is provided only for error reporting. - fn write_to_file(&mut self, file: &mut File, path: &wstr) -> std::io::Result<()> { - let contents = Self::serialize_with_vars(&self.vars); - - let res = file.write_all(&contents); - match res.as_ref() { - Ok(()) => { - // Since we just wrote out this file, it matches our internal state; pretend we read from it. - self.last_read_file_id = file_id_for_file(file); - } - Err(err) => { - let error = Errno(err.raw_os_error().unwrap()); - FLOG!( - error, - wgettext_fmt!( - "Unable to write to universal variables file '%ls': %s", - path, - error.to_string() - ), - ); - } - } - - // We don't close the file. - res - } - - fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool { - let ret = wrename(src, dst); - if ret != 0 { - let error = errno(); - FLOG!( - error, - wgettext_fmt!( - "Unable to rename file from '%ls' to '%ls': %s", - src, - dst, - error.to_string() - ) - ); - } - ret == 0 - } - - // Given a variable table, generate callbacks representing the difference between our vars and - // the new vars. Also update our exports generation count as necessary. + /// Given a variable table, generate callbacks representing the difference between our vars and + /// the new vars. + /// Returns by how much the exports generation count should be incremented, as well as a + /// callback list. fn generate_callbacks_and_update_exports( - &mut self, + &self, new_vars: &VarTable, - callbacks: &mut CallbackDataList, - ) { + ) -> (u64, CallbackDataList) { + let mut export_generation_increment = 0; + let mut callbacks = CallbackDataList::new(); // Construct callbacks for erased values. for (key, value) in &self.vars { // Skip modified values. @@ -587,7 +465,7 @@ fn generate_callbacks_and_update_exports( val: None, }); if value.exports() { - self.export_generation += 1; + export_generation_increment += 1; } } } @@ -606,7 +484,7 @@ fn generate_callbacks_and_update_exports( let export_changed = old_exports != new_entry.exports(); let value_changed = existing.is_some_and(|v| v != new_entry); if export_changed || value_changed { - self.export_generation += 1; + export_generation_increment += 1; } if existing.is_none() || export_changed || value_changed { // Value is set for the first time, or has changed. @@ -616,11 +494,11 @@ fn generate_callbacks_and_update_exports( }); } } + (export_generation_increment, callbacks) } - // Given a variable table, copy unmodified values into self. - fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) { - // Copy modified values from existing vars to vars_to_acquire. + /// Copy modified values from existing vars to `vars_to_acquire`. + fn acquire_variables(&self, vars_to_acquire: &mut VarTable) { for key in &self.modified { match self.vars.get(key) { None => { @@ -628,15 +506,11 @@ fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) { vars_to_acquire.remove(key); } Some(src) => { - // The value has been modified. Copy it over. Note we can destructively modify the - // source entry in vars since we are about to get rid of this->vars entirely. + // The value has been modified. Copy it over. vars_to_acquire.insert(key.clone(), src.clone()); } } } - - // We have constructed all the callbacks and updated vars_to_acquire. Acquire it! - self.vars = vars_to_acquire; } fn populate_1_variable( @@ -747,81 +621,6 @@ fn read_message_internal(file: &File, vars: &mut VarTable) -> UvarFormat { Self::populate_variables(&contents, vars) } - - // Write our file contents. - // Return true on success, false on failure. - fn save(&mut self, directory: &wstr) -> bool { - use crate::common::ScopeGuard; - assert!(self.ok_to_save, "It's not OK to save"); - - // Open adjacent temporary file. - let mut private_file_path = WString::new(); - let Ok(mut private_file) = self.open_temporary_file(directory, &mut private_file_path) - else { - return false; - }; - // unlink pfp upon failure. In case of success, it (already) won't exist. - let delete_pfp = ScopeGuard::new(private_file_path, |path| { - wunlink(&path); - }); - let private_file_path = &delete_pfp; - - // Write to it. - if let Err(e) = self.write_to_file(&mut private_file, private_file_path) { - FLOG!(uvar_file, "universal log write_to_file() failed:", e); - return false; - } - - let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone()); - - // Ensure we maintain ownership and permissions (#2176). - if let Ok(md) = wstat(&real_path) { - // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73 - if unsafe { libc::fchown(private_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { - FLOG!(uvar_file, "universal log fchown() failed:", errno()); - } - if let Err(e) = private_file.set_permissions(md.permissions()) { - FLOG!(uvar_file, "universal log setting permissions failed:", e); - } - } - - // Linux by default stores the mtime with low precision, low enough that updates that occur - // in quick succession may result in the same mtime (even the nanoseconds field). So - // manually set the mtime of the new file to a high-precision clock. Note that this is only - // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other - // platforms we tend to notice the file has changed due to a different inode (or file size!) - // - // The current time within the Linux kernel is cached, and generally only updated on a timer - // interrupt. So if the timer interrupt is running at 10 milliseconds, the cached time will - // only be updated once every 10 milliseconds. - // - // It's probably worth finding a simpler solution to this. The tests ran into this, but it's - // unlikely to affect users. - #[cfg(any(target_os = "linux", target_os = "android"))] - { - let mut times: [libc::timespec; 2] = unsafe { std::mem::zeroed() }; - times[0].tv_nsec = libc::UTIME_OMIT; // don't change ctime - if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } == 0 { - unsafe { - libc::futimens(private_file.as_raw_fd(), ×[0]); - } - } - } - - // Apply new file. - if !self.move_new_vars_file_into_place(private_file_path, &real_path) { - FLOG!( - uvar_file, - "universal log move_new_vars_file_into_place() failed" - ); - return false; - } - - // Success at last. All of our modified variables have now been written out. - self.modified.clear(); - ScopeGuard::cancel(delete_pfp); - true - } } /// Return the default variable path, or an empty string on failure. @@ -1002,29 +801,6 @@ fn encode_serialized(vals: &[WString]) -> WString { join_strings(vals, UVAR_ARRAY_SEP) } -/// Try locking the file. -/// Return true on success, false on error. -fn flock_uvar_file(file: &mut File) -> bool { - let start_time = timef(); - while unsafe { libc::flock(file.as_raw_fd(), LOCK_EX) } == -1 { - if errno().0 != EINTR { - return false; // do nothing per issue #2149 - } - } - let duration = timef() - start_time; - if duration > 0.25 { - FLOG!( - warning, - wgettext_fmt!( - "Locking the universal var file took too long (%.3f seconds).", - duration - ) - ); - return false; - } - true -} - fn skip_spaces(mut s: &wstr) -> &wstr { while s.starts_with(L!(" ")) || s.starts_with(L!("\t")) { s = &s[1..]; diff --git a/src/fallback.rs b/src/fallback.rs index d57e1eee7..9c820716a 100644 --- a/src/fallback.rs +++ b/src/fallback.rs @@ -5,12 +5,8 @@ use crate::wchar::prelude::*; use crate::widecharwidth::{WcLookupTable, WcWidth}; -use errno::{errno, Errno}; use once_cell::sync::Lazy; use std::cmp; -use std::ffi::CString; -use std::fs::File; -use std::os::fd::FromRawFd; use std::sync::atomic::{AtomicIsize, Ordering}; /// Width of ambiguous East Asian characters and, as of TR11, all private-use characters. @@ -115,32 +111,6 @@ pub fn fish_wcswidth(s: &wstr) -> isize { result } -// Replacement for mkostemp(str, O_CLOEXEC) -// This uses mkostemp if available, -// otherwise it uses mkstemp followed by fcntl -pub fn fish_mkstemp_cloexec(name_template: CString) -> Result<(File, CString), Errno> { - let name = name_template.into_raw(); - #[cfg(not(apple))] - let fd = { - use libc::O_CLOEXEC; - unsafe { libc::mkostemp(name, O_CLOEXEC) } - }; - #[cfg(apple)] - let fd = { - use libc::{FD_CLOEXEC, F_SETFD}; - let fd = unsafe { libc::mkstemp(name) }; - if fd != -1 { - unsafe { libc::fcntl(fd, F_SETFD, FD_CLOEXEC) }; - } - fd - }; - if fd == -1 { - Err(errno()) - } else { - unsafe { Ok((File::from_raw_fd(fd), CString::from_raw(name))) } - } -} - pub fn wcscasecmp(lhs: &wstr, rhs: &wstr) -> cmp::Ordering { wcscasecmp_fuzzy(lhs, rhs, std::convert::identity) } diff --git a/src/flog.rs b/src/flog.rs index b3fdb203b..f9276c283 100644 --- a/src/flog.rs +++ b/src/flog.rs @@ -124,6 +124,8 @@ pub fn all_categories() -> Vec<&'static category_t> { (profile_history, "profile-history", "History performance measurements"); + (synced_file_access, "synced-file-access", "Synchronized file access"); + (iothread, "iothread", "Background IO thread events"); (fd_monitor, "fd-monitor", "FD monitor events"); diff --git a/src/fs.rs b/src/fs.rs new file mode 100644 index 000000000..dcd273cee --- /dev/null +++ b/src/fs.rs @@ -0,0 +1,432 @@ +use crate::{ + common::{str2wcstring, wcs2zstring, ScopeGuard}, + fds::wopen_cloexec, + path::{path_remoteness, DirRemoteness}, + wchar::prelude::*, + wutil::{ + file_id_for_file, file_id_for_path, wdirname, wrename, wunlink, FileId, INVALID_FILE_ID, + }, + FLOG, FLOGF, +}; +use errno::errno; +use libc::{c_int, fchown, flock, LOCK_EX, LOCK_SH}; +use nix::{fcntl::OFlag, sys::stat::Mode}; +use std::{ + ffi::CString, + fs::File, + io::Seek, + os::{ + fd::{AsRawFd, FromRawFd}, + unix::fs::MetadataExt, + }, +}; + +// Replacement for mkostemp(str, O_CLOEXEC) +// This uses mkostemp if available, +// otherwise it uses mkstemp followed by fcntl +fn fish_mkstemp_cloexec(name_template: CString) -> std::io::Result<(File, CString)> { + let name = name_template.into_raw(); + #[cfg(not(apple))] + let fd = { + use libc::O_CLOEXEC; + unsafe { libc::mkostemp(name, O_CLOEXEC) } + }; + #[cfg(apple)] + let fd = { + use libc::{FD_CLOEXEC, F_SETFD}; + let fd = unsafe { libc::mkstemp(name) }; + if fd != -1 { + unsafe { libc::fcntl(fd, F_SETFD, FD_CLOEXEC) }; + } + fd + }; + if fd == -1 { + Err(std::io::Error::from(errno())) + } else { + unsafe { Ok((File::from_raw_fd(fd), CString::from_raw(name))) } + } +} + +/// Creates a temporary file created according to the template and its name if successful. +pub fn create_temporary_file(name_template: &wstr) -> std::io::Result<(File, WString)> { + let (fd, c_string_template) = loop { + match fish_mkstemp_cloexec(wcs2zstring(name_template)) { + Ok(tmp_file_data) => break tmp_file_data, + Err(e) => match e.kind() { + std::io::ErrorKind::Interrupted => {} + _ => { + FLOG!( + error, + wgettext_fmt!( + "Unable to create temporary file '%ls': %s", + name_template, + e + ) + ); + + return Err(e); + } + }, + } + }; + Ok((fd, str2wcstring(c_string_template.to_bytes()))) +} + +/// Use this struct for all accesses to file which need mutual exclusion. +/// Otherwise, races on the file are possible. +/// The lock is released when this struct is dropped. +pub struct LockedFile { + /// This is the file which requires mutual exclusion. + /// It should only be accessed through this struct, + /// because the locks used here do not protect from other accesses to the file. + data_file: File, + /// The file descriptor of the parent directory, used for locking. + /// The lock is not placed on the data file directly due to issues with renaming. + /// If the data file is renamed after opening and before locking it, + /// There are two independent files around, whose locks do not interact. + /// In some cases this can be identified by checking file identifiers and timestamps, + /// but even with such checks races and corresponding file corruption can occur. + /// It is simpler to lock a different path, which does not change. + /// + /// We may fail to lock (e.g. on lockless NFS - see issue #685. + /// In that case, we proceed as if locking succeeded. + /// This might result in corruption, + /// but the alternative of not being able to access the file at all is not desirable either. + _locked_fd: File, +} + +pub const LOCKED_FILE_MODE: Mode = Mode::from_bits_truncate(0o600); + +pub enum LockingMode { + Shared, + Exclusive(WriteMethod), +} +pub enum WriteMethod { + Append, + RenameIntoPlace, +} + +impl LockingMode { + pub fn flock_op(&self) -> c_int { + match self { + Self::Shared => LOCK_SH, + Self::Exclusive(_) => LOCK_EX, + } + } + + pub fn file_flags(&self) -> OFlag { + match self { + Self::Shared => OFlag::O_RDONLY, + Self::Exclusive(WriteMethod::Append) => { + OFlag::O_WRONLY | OFlag::O_APPEND | OFlag::O_CREAT + } + Self::Exclusive(WriteMethod::RenameIntoPlace) => OFlag::O_RDONLY | OFlag::O_CREAT, + } + } +} + +impl LockedFile { + /// Creates a [`LockedFile`]. + /// Use this for any access to a which requires mutual exclusion, as it ensures correct locking. + /// Use [`LockingMode::Exclusive`] if you want to modify the file in any way. + /// Otherwise you should use [`LockingMode::Shared`]. + /// Two modes of modification are supported: + /// - Appending + /// - Writing to a temporary file which is then renamed into place. + /// File flags are derived from the [`LockingMode`]. + /// `file_name` should just be a name, not a full path. + pub fn new(locking_mode: LockingMode, file_path: &wstr) -> std::io::Result { + let dir_path = wdirname(file_path); + + if path_remoteness(dir_path) == DirRemoteness::remote { + return Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "Directory considered remote. Locking is disabled on remote file systems.", + )); + } + + // We start by locking the directory. + // This is required to avoid racing modifications by other threads/processes. + let dir_fd = wopen_cloexec(dir_path, OFlag::O_RDONLY, Mode::empty())?; + + // Try locking the directory. Retry if locking was interrupted. + while unsafe { flock(dir_fd.as_raw_fd(), locking_mode.flock_op()) } == -1 { + let err = std::io::Error::last_os_error(); + if err.kind() != std::io::ErrorKind::Interrupted { + return Err(err); + } + } + + // Open the data file + let data_file = wopen_cloexec(file_path, locking_mode.file_flags(), LOCKED_FILE_MODE)?; + + Ok(Self { + data_file, + _locked_fd: dir_fd, + }) + } + + pub fn get(&self) -> &File { + &self.data_file + } + + pub fn get_mut(&mut self) -> &mut File { + &mut self.data_file + } +} + +/// Runs the `load` function, which should see a consistent state of the file at `path`. +/// To ensure a consistent state, we use locks to prevent modifications by others. +/// If locking is unavailable, the `load` function might be executed multiple times, +/// until it manages a run without the file being modified in the meantime, or until the maximum +/// number of allowed attempts is reached. +/// If the file does not exist this function will return an error. +pub fn lock_and_load(path: &wstr, load: F) -> std::io::Result<(FileId, UserData)> +where + F: Fn(&File) -> std::io::Result, +{ + match LockedFile::new(LockingMode::Shared, path) { + Ok(locked_file) => { + return Ok(( + file_id_for_file(locked_file.get()), + load(locked_file.get())?, + )); + } + Err(e) => { + FLOGF!( + synced_file_access, + "Error acquiring shared lock on the directory of '%s': %s", + path, + e, + ); + // This function might be called when the file does not exist. + if e.kind() == std::io::ErrorKind::NotFound { + // There is no point in continuing in this function if the file does not + // exist. + return Err(e); + } + } + } + + FLOG!( + synced_file_access, + "flock-based locking is disabled. Using fallback implementation." + ); + + // Fallback implementation for situations where locking is unavailable. + let max_attempts = 1000; + for _ in 0..max_attempts { + let initial_file_id = file_id_for_path(path); + // If we cannot open the file, there is nothing we can do, + // so just return immediately. + let file = wopen_cloexec(path, OFlag::O_RDONLY, Mode::empty())?; + let loaded_data = match load(&file) { + Ok(update_data) => update_data, + Err(_) => { + // Retry if load function failed. Because we do not hold a lock, this might be + // caused by concurrent modifications. + continue; + } + }; + + let final_file_id = file_id_for_path(path); + if initial_file_id != final_file_id { + continue; + } + // If the file id did not change, we assume that we loaded a consistent state. + return Ok((final_file_id, loaded_data)); + } + Err(std::io::Error::new(std::io::ErrorKind::Other, "Failed to update the file. Locking is disabled, and the fallback code did not succeed within the permissible number of attempts.")) +} + +pub struct PotentialUpdate { + pub do_save: bool, + pub data: UserData, +} + +/// Use this function for updating a file based on its current content, +/// for files which might be accessed at the same time by other threads/processes. +/// The basic principle is to create a temporary file, take a lock on the file to be rewritten, +/// do the rewrite (which might involve reading the old file contents), +/// and finally release the lock. +/// Error handling (especially the case where locking is not possible) makes the implementation +/// non-straightforward. +/// +/// # Arguments +/// +/// - `path`: The path to the file which should be updated. +/// - `rewrite`: The function which handles reading from the file and writing to a temporary file. +/// The first argument is for the file to read from, the second for the temporary file to write to. +/// On success, the value returned by `rewrite` is included in this functions return value. Be +/// careful about side effects of `rewrite`. It might get executed multiple times. Try to avoid +/// side effects and instead extract any data you might need and return them on success. +/// Then, apply the desired side effects once this function has returned successfully. +/// +/// # Return value +/// +/// On success, the [`FileId`] of the rewritten file is returned, alongside the value returned by +/// `rewrite`. Note that if locking is unavailable, the [`FileId`] might be the id of a different +/// version of the file, which was written after this function renamed the temporary file to `path` +/// but before we obtained the [`FileId`] from `path`. This is a race condition we do not detect. +pub fn rewrite_via_temporary_file( + path: &wstr, + rewrite: F, +) -> std::io::Result<(FileId, PotentialUpdate)> +where + F: Fn(&File, &mut File) -> std::io::Result>, +{ + /// Updates the metadata of the `new_file` to match the `old_file`. + /// Also updates the mtime of the `new_file` to the current time manually, to work around + /// operating systems which do not update the internal time used for such time stamps + /// frequently enough. This is important for [`FileId`] comparisons. + fn update_metadata(old_file: &File, new_file: &File) { + // Ensure we maintain the ownership and permissions of the original (#2355). If the + // stat fails, we assume (hope) our default permissions are correct. This + // corresponds to e.g. someone running sudo -E as the very first command. If they + // did, it would be tricky to set the permissions correctly. (bash doesn't get this + // case right either). + if let Ok(md) = old_file.metadata() { + // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73 + if unsafe { fchown(new_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { + FLOG!( + synced_file_access, + "Error when changing ownership of file:", + errno::errno() + ); + } + if let Err(e) = new_file.set_permissions(md.permissions()) { + FLOG!(synced_file_access, "Error when changing mode of file:", e); + } + } else { + FLOG!(synced_file_access, "Could not get metadata for file"); + } + // Linux by default stores the mtime with low precision, low enough that updates that occur + // in quick succession may result in the same mtime (even the nanoseconds field). So + // manually set the mtime of the new file to a high-precision clock. Note that this is only + // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other + // platforms we tend to notice the file has changed due to a different inode. + // + // The current time within the Linux kernel is cached, and generally only updated on a timer + // interrupt. So if the timer interrupt is running at 10 milliseconds, the cached time will + // only be updated once every 10 milliseconds. + #[cfg(any(target_os = "linux", target_os = "android"))] + { + let mut times: [libc::timespec; 2] = unsafe { std::mem::zeroed() }; + times[0].tv_nsec = libc::UTIME_OMIT; // don't change atime + if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } == 0 { + unsafe { + // This accesses both times[0] and times[1]. Check `utimensat(2)` for details. + libc::futimens(new_file.as_raw_fd(), ×[0]); + } + } + } + } + + /// Renames a file from `old_name` to `new_name`. + fn rename(old_name: &wstr, new_name: &wstr) -> std::io::Result<()> { + if wrename(old_name, new_name) == -1 { + let error_number = errno::errno(); + FLOG!( + error, + wgettext_fmt!("Error when renaming file: %s", error_number.to_string()) + ); + return Err(std::io::Error::from(error_number)); + } + Ok(()) + } + + const TMP_FILE_SUFFIX: &wstr = L!(".XXXXXX"); + let tmp_file_template = path.to_owned() + TMP_FILE_SUFFIX; + let (tmp_file, tmp_name) = create_temporary_file(&tmp_file_template)?; + // Ensure that the temporary file is unlinked when this function returns. + let mut tmp_file = ScopeGuard::new(tmp_file, |_| { + wunlink(&tmp_name); + }); + + // We want to rewrite the file. + // To avoid issues with crashes during writing, + // we write to a temporary file and once we are done, this file is renamed such that it + // replaces the original file. + // To avoid races, we need to have exclusive access to the file for the entire + // duration, which we get via an exclusive lock on the parent directory. + // Taking a shared lock first and later upgrading to an exclusive one could result in a + // deadlock, so we take an exclusive one immediately. + match LockedFile::new(LockingMode::Exclusive(WriteMethod::RenameIntoPlace), path) { + Ok(locked_file) => { + let potential_update = rewrite(locked_file.get(), &mut tmp_file)?; + if potential_update.do_save { + update_metadata(locked_file.get(), &tmp_file); + rename(&tmp_name, path)?; + } + return Ok((file_id_for_path(path), potential_update)); + } + Err(e) => { + FLOGF!( + synced_file_access, + "Error acquiring exclusive lock on the directory of '%s': %s", + path, + e, + ); + } + } + + // If this is reached, we assume that locking is not available so we use a fallback + // implementation which tries to avoid race conditions, but in the case of contention it is + // possible that some writes are lost. + + FLOG!( + synced_file_access, + "flock-based locking is disabled. Using fallback implementation." + ); + + // Give up after this many unsuccessful attempts. + // TODO: which value, should this be a constant shared with other retrying logic? + let max_attempts = 1000; + for _ in 0..max_attempts { + // Truncate tmp_file for the next attempt to get rid of data potentially written in the + // previous iteration. + tmp_file.set_len(0)?; + tmp_file.seek(std::io::SeekFrom::Start(0)).unwrap(); + + // If the file does not exist yet, this will be `INVALID_FILE_ID`. + let initial_file_id = file_id_for_path(path); + // If we cannot open the file, there is nothing we can do, + // so just return immediately. + let old_file = wopen_cloexec(path, OFlag::O_RDONLY | OFlag::O_CREAT, LOCKED_FILE_MODE)?; + let opened_file_id = file_id_for_file(&old_file); + if initial_file_id != INVALID_FILE_ID && initial_file_id != opened_file_id { + // File ID changed (and not just because the file was created by us). + continue; + } + let Ok(potential_update) = rewrite(&old_file, &mut tmp_file) else { + // Retry if rewrite function failed. Because we do not hold a lock, this might be + // caused by concurrent modifications. + continue; + }; + + if potential_update.do_save { + update_metadata(&old_file, &tmp_file); + } + + let mut final_file_id = file_id_for_path(path); + if opened_file_id != final_file_id { + continue; + } + + // If we reach this point, the file ID did not change while we read the old file and wrote + // to `tmp_file`. Now we replace the old file with the `tmp_file`. + // Note that we cannot prevent races here. + // If the file is modified by someone else between the syscall for determining the [`FileId`] + // and the rename syscall, these modifications will be lost. + + if potential_update.do_save { + // Do not retry on rename failures, as it is unlikely that these will disappear if we retry. + rename(&tmp_name, path)?; + final_file_id = file_id_for_path(path); + } + // Note that this might not match the version of the file we just wrote. + // (If we did write.) + return Ok((final_file_id, potential_update)); + } + Err(std::io::Error::new(std::io::ErrorKind::Other, "Failed to update the file. Locking is disabled, and the fallback code did not succeed within the permissible number of attempts.")) +} diff --git a/src/history.rs b/src/history.rs index 89dafaba4..383dbcb5e 100644 --- a/src/history.rs +++ b/src/history.rs @@ -2,60 +2,50 @@ //! //! 1. All history files are append-only. Data, once written, is never modified. //! -//! 2. A history file may be re-written ("vacuumed"). This involves reading in the file and writing a -//! new one, while performing maintenance tasks: discarding items in an LRU fashion until we reach -//! the desired maximum count, removing duplicates, and sorting them by timestamp (eventually, not -//! implemented yet). The new file is atomically moved into place via rename(). +//! 2. A history file may be re-written ("vacuumed"). This involves reading in the file and writing +//! a new one, while performing maintenance tasks: discarding items in an LRU fashion until we +//! reach the desired maximum count, removing duplicates, and sorting them by timestamp +//! (eventually, not implemented yet). The new file is atomically moved into place via `rename()`. //! -//! 3. History files are mapped in via mmap(). Before the file is mapped, the file takes a fcntl read -//! lock. The purpose of this lock is to avoid seeing a transient state where partial data has been -//! written to the file. +//! 3. History files are mapped in via `mmap()`. This allows only storing one `usize` per item (its +//! offset), and lazily loading items on demand, which reduces memory consumption. //! -//! 4. History is appended to under a fcntl write lock. -//! -//! 5. The chaos_mode boolean can be set to true to do things like lower buffer sizes which can -//! trigger race conditions. This is useful for testing. -//! -//! Locking on remote filesystems may hang for an unacceptably long time. For that reason, fish -//! does not take locks on the file if it believes the history file is on a remote filesystem, -//! or if the mmap fails with ENODEV, or if the first lock attempt takes excessively long. -//! Eliding locks means that two concurrent shell sessions with a remote history file may, in -//! rare cases with multiple simultaneous shell sessions, lose a history item; this is -//! considered preferable to hanging the the shell waiting for a lock. +//! 4. Accesses to the history file need to be synchronized. This is achieved by functionality in +//! `src/fs.rs`. By default, `flock()` is used for locking. If that is unavailable, an imperfect +//! fallback solution attempts to detect races and retries if a race is detected. use crate::{ - common::cstr2wcstring, env::EnvVar, wcstringutil::trim, - wutil::fileid::file_id_for_path_or_error, + common::cstr2wcstring, + env::EnvVar, + fs::{ + lock_and_load, rewrite_via_temporary_file, LockedFile, LockingMode, PotentialUpdate, + WriteMethod, LOCKED_FILE_MODE, + }, + wcstringutil::trim, }; use std::{ borrow::Cow, collections::{BTreeMap, HashMap, HashSet}, ffi::CString, fs::File, - io::{BufRead, Read, Seek, SeekFrom, Write}, + io::{BufRead, Read, Write}, mem::MaybeUninit, num::NonZeroUsize, ops::ControlFlow, - os::{fd::AsRawFd, unix::fs::MetadataExt}, sync::{Arc, Mutex, MutexGuard}, time::{Duration, SystemTime, UNIX_EPOCH}, }; use bitflags::bitflags; -use libc::{fchown, flock, EINTR, LOCK_EX, LOCK_SH, LOCK_UN}; use lru::LruCache; use nix::{fcntl::OFlag, sys::stat::Mode}; use rand::Rng; use crate::{ ast::{self, Kind, Node}, - common::{ - str2wcstring, unescape_string, valid_var_name, wcs2zstring, CancelChecker, - UnescapeStringStyle, - }, + common::{str2wcstring, unescape_string, valid_var_name, CancelChecker, UnescapeStringStyle}, env::{EnvMode, EnvStack, Environment}, expand::{expand_one, ExpandFlags}, - fallback::fish_mkstemp_cloexec, fds::wopen_cloexec, flog::{FLOG, FLOGF}, global_safety::RelaxedAtomicBool, @@ -64,18 +54,13 @@ operation_context::{OperationContext, EXPANSION_LIMIT_BACKGROUND}, parse_constants::{ParseTreeFlags, StatementDecoration}, parse_util::{parse_util_detect_errors, parse_util_unescape_wildcards}, - path::{ - path_get_config, path_get_data, path_get_data_remoteness, path_is_valid, DirRemoteness, - }, + path::{path_get_config, path_get_data, path_is_valid}, threads::{assert_is_background_thread, iothread_perform}, util::{find_subslice, get_rng}, wchar::prelude::*, wcstringutil::subsequence_in_string, wildcard::{wildcard_match, ANY_STRING}, - wutil::{ - file_id_for_file, file_id_for_path, wgettext_fmt, wrealpath, wrename, wstat, wunlink, - FileId, INVALID_FILE_ID, - }, + wutil::{file_id_for_file, wgettext_fmt, wrealpath, wstat, wunlink, FileId, INVALID_FILE_ID}, }; mod file; @@ -139,14 +124,6 @@ pub enum SearchDirection { /// Default buffer size for flushing to the history file. const HISTORY_OUTPUT_BUFFER_SIZE: usize = 64 * 1024; -/// The file access mode we use for creating history files -const HISTORY_FILE_MODE: Mode = Mode::from_bits_truncate(0o600); - -/// How many times we retry to save -/// Saving may fail if the file is modified in between our opening -/// the file and taking the lock -const MAX_SAVE_TRIES: usize = 1024; - pub const VACUUM_FREQUENCY: usize = 25; /// Output the contents `buffer` to `file` and clear the `buffer`. @@ -219,22 +196,6 @@ fn add_item(&mut self, item: HistoryItem) { } } -/// Returns the path for the history file for the given `session_id`, or `None` if it could not be -/// loaded. If `suffix` is provided, append that suffix to the path; this is used for temporary files. -fn history_filename(session_id: &wstr, suffix: &wstr) -> Option { - if session_id.is_empty() { - return None; - } - - let mut result = path_get_data()?; - - result.push('/'); - result.push_utfstr(session_id); - result.push_utfstr(L!("_history")); - result.push_utfstr(suffix); - Some(result) -} - pub type PathList = Vec; pub type HistoryIdentifier = u64; @@ -403,21 +364,38 @@ struct HistoryImpl { old_item_offsets: Vec, } -/// If set, we gave up on file locking because it took too long. -/// Note this is shared among all history instances. -static ABANDONED_LOCKING: RelaxedAtomicBool = RelaxedAtomicBool::new(false); - impl HistoryImpl { + /// Returns the canonical path for the history file, or `Ok(None)` in private mode. + /// An error is returned if obtaining the data directory fails. + /// Because the `path_get_data` function does not return error information, + /// we cannot provide more detail about the reason for the failure here. + fn history_file_path(&self) -> std::io::Result> { + if self.name.is_empty() { + return Ok(None); + } + + let Some(mut path) = path_get_data() else { + return Err(std::io::Error::new(std::io::ErrorKind::NotFound, "Error obtaining data directory. This is a manually constructed error which does not indicate why this happened.")); + }; + + path.push('/'); + path.push_utfstr(&self.name); + path.push_utfstr(L!("_history")); + if let Some(canonicalized_path) = wrealpath(&path) { + Ok(Some(canonicalized_path)) + } else { + Err(std::io::Error::new( + std::io::ErrorKind::Other, + format!("wrealpath failed to produce a canonical version of '{path}'."), + )) + } + } + /// Add a new history item to the end. If `pending` is set, the item will not be returned by /// `item_at_index()` until a call to `resolve_pending()`. Pending items are tracked with an /// offset into the array of new items, so adding a non-pending item has the effect of resolving /// all pending items. - fn add( - &mut self, - item: HistoryItem, - pending: bool, /*=false*/ - do_save: bool, /*=true*/ - ) { + fn add(&mut self, item: HistoryItem, pending: bool, do_save: bool) { // We use empty items as sentinels to indicate the end of history. // Do not allow them to be added (#6032). if item.contents.is_empty() { @@ -499,34 +477,18 @@ fn load_old_if_needed(&mut self) { self.loaded_old = true; let _profiler = TimeProfiler::new("load_old"); - if let Some(filename) = history_filename(&self.name, L!("")) { - let Ok(mut file) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { - return; - }; - - // Take a read lock to guard against someone else appending. This is released after - // getting the file's length. We will read the file after releasing the lock, but that's - // not a problem, because we never modify already written data. In short, the purpose of - // this lock is to ensure we don't see the file size change mid-update. - // - // We may fail to lock (e.g. on lockless NFS - see issue #685. In that case, we proceed - // as if it did not fail. The risk is that we may get an incomplete history item; this - // is unlikely because we only treat an item as valid if it has a terminating newline. - let locked = unsafe { Self::maybe_lock_file(&mut file, LOCK_SH) }; - self.file_contents = HistoryFileContents::create(&mut file); - self.history_file_id = if self.file_contents.is_some() { - file_id_for_file(&file) - } else { - INVALID_FILE_ID - }; - if locked { - unsafe { - Self::unlock_file(&mut file); + if let Ok(Some(history_path)) = self.history_file_path() { + match lock_and_load(&history_path, HistoryFileContents::create) { + Ok((file_id, file_contents)) => { + self.file_contents = Some(file_contents); + self.history_file_id = file_id; + let _profiler = TimeProfiler::new("populate_from_file_contents"); + self.populate_from_file_contents(); + } + Err(e) => { + FLOG!(history_file, "Error reading from history file:", e); } } - - let _profiler = TimeProfiler::new("populate_from_file_contents"); - self.populate_from_file_contents(); } } @@ -575,8 +537,11 @@ fn remove_ephemeral_items(&mut self) { } /// Given an existing history file, write a new history file to `dst`. - /// Returns false on error, true on success - fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut File) -> bool { + fn rewrite_to_temporary_file( + &self, + existing_file: &File, + dst: &mut File, + ) -> std::io::Result<()> { // We are reading FROM existing_file and writing TO dst // Make an LRU cache to save only the last N elements. @@ -584,32 +549,29 @@ fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut // Read in existing items (which may have changed out from underneath us, so don't trust our // old file contents). - if let Some(existing_file) = existing_file { - if let Some(local_file) = HistoryFileContents::create(existing_file) { - let mut cursor = 0; - while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) { - // Try decoding an old item. - let Some(old_item) = local_file.decode_item(offset) else { - continue; - }; + if let Ok(local_file) = HistoryFileContents::create(existing_file) { + let mut cursor = 0; + while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) { + // Try decoding an old item. + let Some(old_item) = local_file.decode_item(offset) else { + continue; + }; - // If old item is newer than session always erase if in deleted. - if old_item.timestamp() > self.boundary_timestamp { - if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) { - continue; - } - lru.add_item(old_item); - } else { - // If old item is older and in deleted items don't erase if added by - // clear_session. - if old_item.is_empty() - || self.deleted_items.get(old_item.str()) == Some(&false) - { - continue; - } - // Add this old item. - lru.add_item(old_item); + // If old item is newer than session always erase if in deleted. + if old_item.timestamp() > self.boundary_timestamp { + if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) { + continue; } + lru.add_item(old_item); + } else { + // If old item is older and in deleted items don't erase if added by + // clear_session. + if old_item.is_empty() || self.deleted_items.get(old_item.str()) == Some(&false) + { + continue; + } + // Add this old item. + lru.add_item(old_item); } } } @@ -654,168 +616,47 @@ fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut "Error writing to temporary history file:", err ); - - false + Err(err) } else { - true + Ok(()) } } /// Saves history by rewriting the file. - fn save_internal_via_rewrite(&mut self) { + fn save_internal_via_rewrite(&mut self, history_path: &wstr) -> std::io::Result<()> { FLOGF!( history, "Saving %lu items via rewrite", self.new_items.len() - self.first_unwritten_new_item_index ); - // We want to rewrite the file, while holding the lock for as briefly as possible - // To do this, we speculatively write a file, and then lock and see if our original file changed - // Repeat until we succeed or give up - let Some(possibly_indirect_target_name) = history_filename(&self.name, L!("")) else { - return; - }; - let Some(tmp_name_template) = history_filename(&self.name, L!(".XXXXXX")) else { - return; - }; + let rewrite = + |old_file: &File, tmp_file: &mut File| -> std::io::Result> { + self.rewrite_to_temporary_file(old_file, tmp_file)?; + Ok(PotentialUpdate { + do_save: true, + data: (), + }) + }; - // If the history file is a symlink, we want to rewrite the real file so long as we can find it. - let target_name = - wrealpath(&possibly_indirect_target_name).unwrap_or(possibly_indirect_target_name); + let (file_id, _) = rewrite_via_temporary_file(history_path, rewrite)?; + self.history_file_id = file_id; - // Make our temporary file - let Some((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { - return; - }; - let mut done = false; - for _i in 0..MAX_SAVE_TRIES { - if done { - break; - } + // We've saved everything, so we have no more unsaved items. + self.first_unwritten_new_item_index = self.new_items.len(); - let target_file_before = wopen_cloexec( - &target_name, - OFlag::O_RDONLY | OFlag::O_CREAT, - HISTORY_FILE_MODE, - ); - if let Err(err) = target_file_before { - FLOG!(history_file, "Error opening history file:", err); - } + // We deleted our deleted items. + self.deleted_items.clear(); - let orig_file_id = target_file_before - .as_ref() - .map(file_id_for_file) - .unwrap_or(INVALID_FILE_ID); + // Our history has been written to the file, so clear our state so we can re-reference the + // file. + self.clear_file_state(); - // Open any target file, but do not lock it right away - if !self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) { - // Failed to write, no good - break; - } - - // The crux! We rewrote the history file; see if the history file changed while we - // were rewriting it. Make an effort to take the lock before checking, to avoid racing. - // If the open fails, then proceed; this may be because there is no current history - let mut new_file_id = INVALID_FILE_ID; - - let mut target_file_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty()); - if let Ok(target_file_after) = target_file_after.as_mut() { - // critical to take the lock before checking file IDs, - // and hold it until after we are done replacing. - // Also critical to check the file at the path, NOT based on our fd. - // It's only OK to replace the file while holding the lock. - // Note any lock is released when target_file_after is closed. - unsafe { - Self::maybe_lock_file(target_file_after, LOCK_EX); - } - new_file_id = match file_id_for_path_or_error(&target_name) { - Ok(file_id) => file_id, - Err(err) => { - if err.kind() != std::io::ErrorKind::NotFound { - FLOG!(history_file, "Error re-opening history file:", err); - } - INVALID_FILE_ID - } - } - } - - let can_replace_file = new_file_id == orig_file_id || new_file_id == INVALID_FILE_ID; - if !can_replace_file { - // The file has changed, so we're going to re-read it - // Truncate our tmp_file so we can reuse it - if let Err(err) = tmp_file.set_len(0) { - FLOG!( - history_file, - "Error when truncating temporary history file:", - err - ); - } - if let Err(err) = tmp_file.seek(SeekFrom::Start(0)) { - FLOG!( - history_file, - "Error resetting cursor in temporary history file:", - err - ); - } - } else { - // The file is unchanged, or the new file doesn't exist or we can't read it - // We also attempted to take the lock, so we feel confident in replacing it - - // Ensure we maintain the ownership and permissions of the original (#2355). If the - // stat fails, we assume (hope) our default permissions are correct. This - // corresponds to e.g. someone running sudo -E as the very first command. If they - // did, it would be tricky to set the permissions correctly. (bash doesn't get this - // case right either). - if let Ok(target_file_after) = target_file_after.as_ref() { - if let Ok(md) = target_file_after.metadata() { - // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73 - if unsafe { fchown(tmp_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { - FLOG!( - history_file, - "Error when changing ownership of history file:", - errno::errno() - ); - } - if let Err(e) = tmp_file.set_permissions(md.permissions()) { - FLOG!(history_file, "Error when changing mode of history file:", e); - } - } - } - - // Slide it into place - if wrename(&tmp_name, &target_name) == -1 { - FLOG!( - error, - wgettext_fmt!( - "Error when renaming history file: %s", - errno::errno().to_string() - ) - ); - } - - // We did it - done = true; - } - } - - // Ensure we never leave the old file around - let _ = wunlink(&tmp_name); - - if done { - // We've saved everything, so we have no more unsaved items. - self.first_unwritten_new_item_index = self.new_items.len(); - - // We deleted our deleted items. - self.deleted_items.clear(); - - // Our history has been written to the file, so clear our state so we can re-reference the - // file. - self.clear_file_state(); - } + Ok(()) } /// Saves history by appending to the file. - fn save_internal_via_appending(&mut self) -> std::io::Result<()> { + fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Result<()> { FLOGF!( history, "Saving %lu items via appending", @@ -824,65 +665,20 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // No deleting allowed. assert!(self.deleted_items.is_empty()); - // If the file is different (someone vacuumed it) then we need to update our mmap. - let mut file_changed = false; + let mut locked_history_file = + LockedFile::new(LockingMode::Exclusive(WriteMethod::Append), history_path)?; - // Get the path to the real history file. - let Some(history_path) = history_filename(&self.name, L!("")) else { - // No history should be saved if fish_history is set to the empty string, - // so nothing needs to be done here. - return Ok(()); - }; + // Check if the file was modified since it was last read. + let file_id = file_id_for_file(locked_history_file.get()); + let file_changed = file_id != self.history_file_id; - // We are going to open the file, lock it, append to it, and then close it - // After locking it, we need to stat the file at the path; if there is a new file there, it - // means the file was replaced and we have to try again. - // Limit our max tries so we don't do this forever. - let mut num_attempts = 0; - let mut history_file = loop { - if num_attempts == MAX_SAVE_TRIES { - return Err(std::io::Error::new( - std::io::ErrorKind::Other, - "Number of unsuccessful attempts to open history file exceeds maximum.", - )); - } - // Return immediately if an error occurs here. - let mut file = wopen_cloexec( - &history_path, - OFlag::O_WRONLY | OFlag::O_APPEND, - Mode::empty(), - )?; - - // Exclusive lock on the entire file. This is released when we close the file (below). This - // may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that - // we may get interleaved history items, which is considered better than no history, or - // forcing everything through the slow copy-move mode. We try to minimize this possibility - // by writing with O_APPEND. - unsafe { - Self::maybe_lock_file(&mut file, LOCK_EX); - } - let file_id = file_id_for_file(&file); - if file_id_for_path(&history_path) == file_id { - // File IDs match, so the file we opened is still at that path - // We're going to use this fd - if file_id != self.history_file_id { - file_changed = true; - } - break file; - } - num_attempts += 1; - }; - - // We (hopefully successfully) took the exclusive lock. Append to the file. + // We took the exclusive lock. Append to the file. // Note that this is sketchy for a few reasons: // - Another shell may have appended its own items with a later timestamp, so our file may // no longer be sorted by timestamp. // - Another shell may have appended the same items, so our file may now contain // duplicates. // - // We cannot modify any previous parts of our file, because other instances may be reading - // those portions. We can only append. - // // Originally we always rewrote the file on saving, which avoided both of these problems. // However, appending allows us to save history after every command, which is nice! // @@ -906,7 +702,8 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // the file system. // Flushing and syncing each iteration adds overhead, // but hopefully there are not that many items to write when appending. - res = drain_buffer_into_file_and_flush(&mut buffer, &mut history_file); + res = drain_buffer_into_file_and_flush(&mut buffer, locked_history_file.get_mut()); + if res.is_err() { break; } @@ -918,11 +715,11 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // Since we just modified the file, update our history_file_id to match its current state // Otherwise we'll think the file has been changed by someone else the next time we go to // write. - // We don't update the mapping since we only appended to the file, and everything we + // We don't update `self.file_contents` since we only appended to the file, and everything we // appended remains in our new_items - self.history_file_id = file_id_for_file(&history_file); + self.history_file_id = file_id; - drop(history_file); + drop(locked_history_file); // If someone has replaced the file, forget our file state. if file_changed { @@ -941,30 +738,41 @@ fn save(&mut self, vacuum: bool) { return; } - if history_filename(&self.name, L!("")).is_none() { + // Compact our new items so we don't have duplicates. + self.compact_new_items(); + + if self.name.is_empty() { // We're in the "incognito" mode. Pretend we've saved the history. self.first_unwritten_new_item_index = self.new_items.len(); self.deleted_items.clear(); self.clear_file_state(); + return; } - // Compact our new items so we don't have duplicates. - self.compact_new_items(); + let history_path = match self.history_file_path() { + Ok(history_path) => history_path.unwrap(), + Err(e) => { + FLOG!(history, "Saving history failed:", e); + return; + } + }; // Try saving. If we have items to delete, we have to rewrite the file. If we do not, we can // append to it. let mut ok = false; if !vacuum && self.deleted_items.is_empty() { // Try doing a fast append. - if let Err(e) = self.save_internal_via_appending() { - FLOG!(history, "Appending failed", e); + if let Err(e) = self.save_internal_via_appending(&history_path) { + FLOG!(history, "Appending to history failed:", e); } else { ok = true; } } if !ok { // We did not or could not append; rewrite the file ("vacuum" it). - self.save_internal_via_rewrite(); + if let Err(e) = self.save_internal_via_rewrite(&history_path) { + FLOG!(history, "Rewriting history failed:", e) + } } } @@ -975,7 +783,7 @@ fn save_unless_disabled(&mut self) { return; } - // We may or may not vacuum. We try to vacuum every kVacuumFrequency items, but start the + // We may or may not vacuum. We try to vacuum every `VACUUM_FREQUENCY` items, but start the // countdown at a random number so that even if the user never runs more than 25 commands, we'll // eventually vacuum. If countdown_to_vacuum is None, it means we haven't yet picked a value for // the counter. @@ -1040,7 +848,8 @@ fn is_empty(&mut self) -> bool { } else { // If we have not loaded old items, don't actually load them (which may be expensive); just // stat the file and see if it exists and is nonempty. - let Some(where_) = history_filename(&self.name, L!("")) else { + + let Ok(Some(where_)) = self.history_file_path() else { return true; }; @@ -1096,7 +905,7 @@ fn clear(&mut self) { self.deleted_items.clear(); self.first_unwritten_new_item_index = 0; self.old_item_offsets.clear(); - if let Some(filename) = history_filename(&self.name, L!("")) { + if let Ok(Some(filename)) = self.history_file_path() { wunlink(&filename); } self.clear_file_state(); @@ -1117,7 +926,7 @@ fn clear_session(&mut self) { /// file to the new history file. /// The new contents will automatically be re-mapped later. fn populate_from_config_path(&mut self) { - let Some(new_file) = history_filename(&self.name, L!("")) else { + let Ok(Some(new_file)) = self.history_file_path() else { return; }; @@ -1140,7 +949,7 @@ fn populate_from_config_path(&mut self) { let mut dst_file = match wopen_cloexec( &new_file, OFlag::O_WRONLY | OFlag::O_CREAT, - HISTORY_FILE_MODE, + LOCKED_FILE_MODE, ) { Ok(file) => file, Err(err) => { @@ -1337,71 +1146,6 @@ fn size(&mut self) -> usize { let old_item_count = self.old_item_offsets.len(); return new_item_count + old_item_count; } - - /// Maybe lock a history file. - /// Returns `true` if successful, `false` if locking was skipped. - /// - /// # Safety - /// - /// `fd` and `lock_type` must be valid arguments to `flock(2)`. - unsafe fn maybe_lock_file(file: &mut File, lock_type: libc::c_int) -> bool { - assert!(lock_type & LOCK_UN == 0, "Do not use lock_file to unlock"); - - // Don't lock if it took too long before, if we are simulating a failing lock, or if our history - // is on a remote filesystem. - if ABANDONED_LOCKING.load() { - return false; - } - if CHAOS_MODE.load() { - return false; - } - if path_get_data_remoteness() == DirRemoteness::remote { - return false; - } - - let (ok, start_time) = loop { - let start_time = SystemTime::now(); - if unsafe { flock(file.as_raw_fd(), lock_type) } != -1 { - break (true, start_time); - } - if errno::errno().0 != EINTR { - break (false, start_time); - } - }; - if let Ok(duration) = start_time.elapsed() { - if duration > Duration::from_millis(250) { - FLOG!( - warning, - wgettext_fmt!( - "Locking the history file took too long (%.3f seconds).", - duration.as_secs_f64() - ) - ); - ABANDONED_LOCKING.store(true); - } - } - ok - } - - /// Unlock a history file. - /// - /// # Safety - /// - /// `fd` must be a valid argument to `flock(2)` with `LOCK_UN`. - unsafe fn unlock_file(file: &mut File) { - libc::flock(file.as_raw_fd(), LOCK_UN); - } -} - -// Returns the fd of an opened temporary file, or None on failure. -fn create_temporary_file(name_template: &wstr) -> Option<(File, WString)> { - for _attempt in 0..10 { - let narrow_str = wcs2zstring(name_template); - if let Ok((fd, narrow_str)) = fish_mkstemp_cloexec(narrow_str) { - return Some((fd, str2wcstring(narrow_str.to_bytes()))); - } - } - None } fn string_could_be_path(potential_path: &wstr) -> bool { @@ -1524,7 +1268,7 @@ fn imp(&self) -> MutexGuard { /// Privately add an item. If pending, the item will not be returned by history searches until a /// call to resolve_pending. Any trailing ephemeral items are dropped. /// Exposed for testing. - pub fn add(&self, item: HistoryItem, pending: bool /*=false*/) { + pub fn add(&self, item: HistoryItem, pending: bool) { self.imp().add(item, pending, true) } @@ -2087,7 +1831,3 @@ pub fn in_private_mode(vars: &dyn Environment) -> bool { /// Whether to force the read path instead of mmap. This is useful for testing. static NEVER_MMAP: RelaxedAtomicBool = RelaxedAtomicBool::new(false); - -/// Whether we're in maximum chaos mode, useful for testing. -/// This causes things like locks to fail. -pub static CHAOS_MODE: RelaxedAtomicBool = RelaxedAtomicBool::new(false); diff --git a/src/history/file.rs b/src/history/file.rs index 7afa5a429..734635150 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -5,7 +5,7 @@ fs::File, io::{Read, Seek, SeekFrom, Write}, ops::{Deref, DerefMut}, - os::fd::{AsRawFd, RawFd}, + os::fd::AsRawFd, time::{Duration, SystemTime, UNIX_EPOCH}, }; @@ -15,7 +15,7 @@ use crate::{ common::{str2wcstring, subslice_position, wcs2string}, flog::FLOG, - path::{path_get_config_remoteness, DirRemoteness}, + path::{path_get_data_remoteness, DirRemoteness}, }; /// History file types. @@ -43,11 +43,19 @@ unsafe fn new(ptr: *mut u8, len: usize) -> Self { Self { ptr, len } } - /// Map a region `[0, len)` from an `fd`. - /// Returns [`None`] on failure. - pub fn map_file(fd: RawFd, len: usize) -> std::io::Result { - assert!(len != 0); - let ptr = unsafe { mmap(std::ptr::null_mut(), len, PROT_READ, MAP_PRIVATE, fd, 0) }; + /// Map a region `[0, len)` from a locked file. + pub fn map_file(file: &File, len: usize) -> std::io::Result { + let ptr = unsafe { + mmap( + std::ptr::null_mut(), + len, + PROT_READ, + MAP_PRIVATE, + file.as_raw_fd(), + 0, + ) + }; + if ptr == MAP_FAILED { return Err(std::io::Error::last_os_error()); } @@ -57,12 +65,7 @@ pub fn map_file(fd: RawFd, len: usize) -> std::io::Result { } /// Map anonymous memory of a given length. - /// Returns [`None`] on failure. - pub fn map_anon(len: usize) -> Option { - if len == 0 { - return None; - } - + pub fn map_anon(len: usize) -> std::io::Result { let ptr = unsafe { mmap( std::ptr::null_mut(), @@ -74,11 +77,11 @@ pub fn map_anon(len: usize) -> Option { ) }; if ptr == MAP_FAILED { - return None; + return Err(std::io::Error::last_os_error()); } // SAFETY: mmap of `len` was successful and returned `ptr` - Some(unsafe { Self::new(ptr.cast(), len) }) + Ok(unsafe { Self::new(ptr.cast(), len) }) } } @@ -113,39 +116,44 @@ pub struct HistoryFileContents { } impl HistoryFileContents { - /// Construct a history file contents from a File reference. - pub fn create(file: &mut File) -> Option { + /// Construct a history file contents from a [`File`] reference. + pub fn create(mut history_file: &File) -> std::io::Result { // Check that the file is seekable, and its size. - let len: usize = file.seek(SeekFrom::End(0)).ok()?.try_into().ok()?; - if len == 0 { - return None; - } - let map_anon = |file: &mut File, len: usize| { + let len: usize = match history_file.seek(SeekFrom::End(0))?.try_into() { + Ok(len) => len, + Err(err) => { + return Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + format!("Cannot convert u64 to usize: {err}"), + )) + } + }; + let map_anon = |mut file: &File, len: usize| -> std::io::Result { let mut region = MmapRegion::map_anon(len)?; // If we mapped anonymous memory, we have to read from the file. - file.seek(SeekFrom::Start(0)).ok()?; - read_zero_padded(&mut *file, region.as_mut()).ok()?; - Some(region) + file.seek(SeekFrom::Start(0))?; + file.read_exact(&mut region)?; + Ok(region) }; let region = if should_mmap() { - match MmapRegion::map_file(file.as_raw_fd(), len) { + match MmapRegion::map_file(history_file, len) { Ok(region) => region, - Err(err) if err.raw_os_error() == Some(ENODEV) => { - // Our mmap failed with ENODEV, which means the underlying - // filesystem does not support mapping. Treat this as a hint - // that the filesystem is remote, and so disable locks for - // the history file. - super::ABANDONED_LOCKING.store(true); - // Create an anonymous mapping and read() the file into it. - map_anon(file, len)? + Err(err) => { + if err.raw_os_error() == Some(ENODEV) { + // Our mmap failed with ENODEV, which means the underlying + // filesystem does not support mapping. + // Create an anonymous mapping and read() the file into it. + map_anon(history_file, len)? + } else { + return Err(err); + } } - Err(_err) => return None, } } else { - map_anon(file, len)? + map_anon(history_file, len)? }; - region.try_into().ok() + region.try_into() } /// Decode an item at a given offset. @@ -184,13 +192,17 @@ fn infer_file_type(contents: &[u8]) -> HistoryFileType { } impl TryFrom for HistoryFileContents { - type Error = (); + type Error = std::io::Error; - fn try_from(region: MmapRegion) -> Result { + fn try_from(region: MmapRegion) -> std::io::Result { let type_ = infer_file_type(®ion); if type_ == HistoryFileType::Fish1_x { - FLOG!(error, "unsupported history file format 1.x"); - return Err(()); + let error_message = "unsupported history file format 1.x"; + FLOG!(error, error_message); + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + error_message, + )); } Ok(Self { region }) } @@ -220,7 +232,7 @@ pub fn append_history_item_to_buffer(item: &HistoryItem, buffer: &mut Vec) { } } -/// Check if we should mmap the fd. +/// Check if we should mmap the file. /// Don't try mmap() on non-local filesystems. fn should_mmap() -> bool { if super::NEVER_MMAP.load() { @@ -228,23 +240,7 @@ fn should_mmap() -> bool { } // mmap only if we are known not-remote. - path_get_config_remoteness() != DirRemoteness::remote -} - -/// Read from `fd` to fill `dest`, zeroing any unused space. -fn read_zero_padded(file: &mut File, mut dest: &mut [u8]) -> std::io::Result<()> { - while !dest.is_empty() { - match file.read(dest) { - Ok(0) => break, - Ok(amt) => { - dest = &mut dest[amt..]; - } - Err(e) if e.kind() == std::io::ErrorKind::Interrupted => continue, - Err(err) => return Err(err), - } - } - dest.fill(0u8); - Ok(()) + path_get_data_remoteness() != DirRemoteness::remote } fn replace_all(s: &mut Vec, needle: &[u8], replacement: &[u8]) { diff --git a/src/lib.rs b/src/lib.rs index 77c0f9f1d..02958330f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -48,6 +48,7 @@ pub mod fds; pub mod flog; pub mod fork_exec; +pub mod fs; pub mod function; pub mod future; pub mod future_feature_flags; diff --git a/src/path.rs b/src/path.rs index 55f671a88..e41762add 100644 --- a/src/path.rs +++ b/src/path.rs @@ -667,7 +667,7 @@ fn create_dir_all_with_mode>(path: P, mode: u32) -> st } /// Return whether the given path is on a remote filesystem. -fn path_remoteness(path: &wstr) -> DirRemoteness { +pub fn path_remoteness(path: &wstr) -> DirRemoteness { let narrow = wcs2zstring(path); #[cfg(target_os = "linux")] { diff --git a/src/tests/env_universal_common.rs b/src/tests/env_universal_common.rs index 71341cefd..e01acd275 100644 --- a/src/tests/env_universal_common.rs +++ b/src/tests/env_universal_common.rs @@ -3,7 +3,7 @@ use crate::common::ScopeGuard; use crate::common::ENCODE_DIRECT_BASE; use crate::env::{EnvVar, EnvVarFlags, VarTable}; -use crate::env_universal_common::{CallbackDataList, EnvUniversal, UvarFormat}; +use crate::env_universal_common::{EnvUniversal, UvarFormat}; use crate::reader::{reader_pop, reader_push, ReaderConfig}; use crate::tests::prelude::*; use crate::threads::{iothread_drain_all, iothread_perform}; @@ -15,15 +15,14 @@ fn test_universal_helper(x: usize) { let _cleanup = test_init(); - let mut callbacks = CallbackDataList::new(); let mut uvars = EnvUniversal::new(); - uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); + uvars.initialize_at_path(UVARS_TEST_PATH.to_owned()); for j in 0..UVARS_PER_THREAD { let key = sprintf!("key_%d_%d", x, j); let val = sprintf!("val_%d_%d", x, j); uvars.set(&key, EnvVar::new(val, EnvVarFlags::empty())); - let synced = uvars.sync(&mut callbacks); + let (synced, _) = uvars.sync(); assert!( synced, "Failed to sync universal variables after modification" @@ -32,7 +31,7 @@ fn test_universal_helper(x: usize) { // Last step is to delete the first key. uvars.remove(&sprintf!("key_%d_%d", x, 0)); - let synced = uvars.sync(&mut callbacks); + let (synced, _) = uvars.sync(); assert!(synced, "Failed to sync universal variables after deletion"); } @@ -54,8 +53,7 @@ fn test_universal() { iothread_drain_all(&mut reader); let mut uvars = EnvUniversal::new(); - let mut callbacks = CallbackDataList::new(); - uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); + uvars.initialize_at_path(UVARS_TEST_PATH.to_owned()); for i in 0..threads { for j in 0..UVARS_PER_THREAD { @@ -219,11 +217,25 @@ fn test_universal_parsing_legacy() { fn test_universal_callbacks() { let _cleanup = test_init(); std::fs::create_dir_all("test/fish_uvars_test/").unwrap(); - let mut callbacks = CallbackDataList::new(); let mut uvars1 = EnvUniversal::new(); let mut uvars2 = EnvUniversal::new(); - uvars1.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); - uvars2.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); + let mut callbacks = uvars1 + .initialize_at_path(UVARS_TEST_PATH.to_owned()) + .unwrap_or_default(); + callbacks.append( + &mut uvars2 + .initialize_at_path(UVARS_TEST_PATH.to_owned()) + .unwrap_or_default(), + ); + + macro_rules! sync { + ($uvars:expr) => { + let (_, cb_opt) = $uvars.sync(); + if let Some(mut cb) = cb_opt { + callbacks.append(&mut cb); + } + }; + } let noflags = EnvVarFlags::empty(); @@ -236,8 +248,8 @@ fn test_universal_callbacks() { uvars1.set(L!("kappa"), EnvVar::new(L!("1").to_owned(), noflags)); // uvars1.set(L!("omicron"), EnvVar::new(L!("1").to_owned(), noflags)); // - uvars1.sync(&mut callbacks); - uvars2.sync(&mut callbacks); + sync!(uvars1); + sync!(uvars2); // Change uvars1. uvars1.set(L!("alpha"), EnvVar::new(L!("2").to_owned(), noflags)); // changes value @@ -247,7 +259,7 @@ fn test_universal_callbacks() { ); // changes export uvars1.remove(L!("delta")); // erases value uvars1.set(L!("epsilon"), EnvVar::new(L!("1").to_owned(), noflags)); // changes nothing - uvars1.sync(&mut callbacks); + sync!(uvars1); // Change uvars2. It should treat its value as correct and ignore changes from uvars1. uvars2.set(L!("lambda"), EnvVar::new(L!("1").to_owned(), noflags)); // same value @@ -255,7 +267,7 @@ fn test_universal_callbacks() { // Now see what uvars2 sees. callbacks.clear(); - uvars2.sync(&mut callbacks); + sync!(uvars2); // Sort them to get them in a predictable order. callbacks.sort_by(|a, b| a.key.cmp(&b.key)); @@ -308,12 +320,12 @@ fn test_universal_ok_to_save() { "UVARS_TEST_PATH should be readable" ); - let mut cbs = CallbackDataList::new(); let mut uvars = EnvUniversal::new(); - uvars.initialize_at_path(&mut cbs, UVARS_TEST_PATH.to_owned()); + uvars + .initialize_at_path(UVARS_TEST_PATH.to_owned()) + .unwrap_or_default(); assert!(!uvars.is_ok_to_save(), "Should not be OK to save"); - uvars.sync(&mut cbs); - cbs.clear(); + uvars.sync(); assert!(!uvars.is_ok_to_save(), "Should still not be OK to save"); uvars.set( L!("SOMEVAR"), diff --git a/src/tests/history.rs b/src/tests/history.rs index d0a7e319a..db1942f07 100644 --- a/src/tests/history.rs +++ b/src/tests/history.rs @@ -272,8 +272,6 @@ fn test_history_races() { // Ensure history is clear. History::new(L!("race_test")).clear(); - // history::CHAOS_MODE.store(true); - let mut children = Vec::with_capacity(RACE_COUNT); for i in 0..RACE_COUNT { children.push(std::thread::spawn(move || { @@ -295,7 +293,6 @@ fn test_history_races() { // Ensure that we got sane, sorted results. let hist = History::new(L!("race_test")); - history::CHAOS_MODE.store(false); // History is enumerated from most recent to least // Every item should be the last item in some array diff --git a/src/wutil/tests.rs b/src/wutil/tests.rs index 46723affb..7ac0ab701 100644 --- a/src/wutil/tests.rs +++ b/src/wutil/tests.rs @@ -1,11 +1,10 @@ -use crate::fds::AutoCloseFd; use crate::tests::prelude::*; use crate::util::get_rng; +use crate::{fds::AutoCloseFd, fs::create_temporary_file}; use libc::{c_void, O_CREAT, O_RDWR, O_TRUNC, SEEK_SET}; use rand::Rng; -use std::{ffi::CString, ptr}; - -use crate::fallback::fish_mkstemp_cloexec; +use std::ffi::CString; +use std::ptr; use super::*; @@ -62,8 +61,8 @@ fn test_wdirname_wbasename() { #[serial] fn test_wwrite_to_fd() { let _cleanup = test_init(); - let (_fd, filename) = - fish_mkstemp_cloexec(CString::new("/tmp/fish_test_wwrite.XXXXXX").unwrap()).unwrap(); + let (_file, filename) = create_temporary_file(L!("/tmp/fish_test_wwrite.XXXXXX")).unwrap(); + let filename = CString::new(filename.to_string()).unwrap(); let mut rng = get_rng(); let sizes = [1, 2, 3, 5, 13, 23, 64, 128, 255, 4096, 4096 * 2]; for &size in &sizes {