From f40e31f6756400c6306dbc142f1cd1313cbb4922 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Sun, 25 May 2025 16:08:18 +0200 Subject: [PATCH 01/14] Return callbacks via return value Callbacks are generated by `src/env_universal_common.rs:generate_callbacks_and_update_exports`. This function only pushes to the `Vec`, so the content of the `Vec` passed to it does not matter within this function. The generated callbacks are used in `src/env/environment.rs:universal_sync`, which calls `generate_callbacks_and_update_exports` via `EnvUniversal::sync`, (optionally `EnvUniversal::load_from_path_narrow`), `EnvUniversal::load_from_file`. The only other code making use of these callbacks is in tests. Because the only real use passes an empty `Vec`, there is no need to to pass the `Vec` as a mutable reference to the function at all. Instead, we create the `Vec` in `generate_callbacks_and_update_exports` and return it from there, which is what the new code does. This change is made because we want loading from a file to be performed without mutating data which does not come directly from the file. Then, we can safely retry loading from the file as many times as we want, without worrying about side-effects on our data structures. We want to be able to do this in the case where we cannot properly lock the file and fall back to lockless reading, where we check file metadata before and after reading to detect modifications, and retry if modifications are detected. This fallback logic is not in place yet, and further changes are required for side-effect free loading. --- src/env/environment.rs | 27 +++++++------- src/env_universal_common.rs | 58 ++++++++++++++----------------- src/tests/env_universal_common.rs | 48 +++++++++++++++---------- 3 files changed, 70 insertions(+), 63 deletions(-) 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..cc41338b7 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -154,34 +154,32 @@ 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) { + pub fn initialize(&mut self) -> Option { // 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()); + 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 { + pub fn sync(&mut self) -> (bool, Option) { if !self.initialized() { - return false; + return (false, None); } FLOG!(uvar_file, "universal log sync"); @@ -216,9 +214,9 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { // 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(); @@ -228,16 +226,16 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { // 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; + return (false, None); }; // Read from it. - self.load_from_file(&mut vars_file, callbacks); + let callbacks = self.load_from_file(&mut vars_file); if self.ok_to_save { - self.save(&directory) + (self.save(&directory), callbacks) } else { - true + (true, callbacks) } } @@ -364,38 +362,38 @@ 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; + return None; }; FLOG!(uvar_file, "universal log reading from file"); - self.load_from_file(&mut file, callbacks); - true + self.load_from_file(&mut file) } // 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) { + fn load_from_file(&mut self, file: &mut 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(); @@ -408,11 +406,12 @@ fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) } // Announce changes and update our exports generation. - self.generate_callbacks_and_update_exports(&new_vars, callbacks); + let 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; + Some(callbacks) } } @@ -568,11 +567,8 @@ fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool { // Given a variable table, generate callbacks representing the difference between our vars and // the new vars. Also update our exports generation count as necessary. - fn generate_callbacks_and_update_exports( - &mut self, - new_vars: &VarTable, - callbacks: &mut CallbackDataList, - ) { + fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> CallbackDataList { + let mut callbacks = CallbackDataList::new(); // Construct callbacks for erased values. for (key, value) in &self.vars { // Skip modified values. @@ -616,6 +612,7 @@ fn generate_callbacks_and_update_exports( }); } } + callbacks } // Given a variable table, copy unmodified values into self. @@ -628,8 +625,7 @@ 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()); } } 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"), From f2f54919af631cb1499fd2ccb005f6ebe7c97eab Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Sun, 25 May 2025 17:37:51 +0200 Subject: [PATCH 02/14] Remove unnecessary `mut` specifier --- src/env_universal_common.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index cc41338b7..6751e491a 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -224,13 +224,13 @@ pub fn sync(&mut self) -> (bool, Option) { FLOG!(uvar_file, "universal log performing full sync"); // Open the file. - let Some(mut vars_file) = self.open_and_acquire_lock() else { + let Some(vars_file) = self.open_and_acquire_lock() else { FLOG!(uvar_file, "universal log open_and_acquire_lock() failed"); return (false, None); }; // Read from it. - let callbacks = self.load_from_file(&mut vars_file); + let callbacks = self.load_from_file(&vars_file); if self.ok_to_save { (self.save(&directory), callbacks) @@ -377,18 +377,16 @@ fn load_from_path_narrow(&mut self) -> Option { return None; } - let Ok(mut file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) - else { + let Ok(file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { return None; }; FLOG!(uvar_file, "universal log reading from file"); - self.load_from_file(&mut file) + self.load_from_file(&file) } - // 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) -> Option { + /// Load environment variables from the opened [`file`](`File`). + fn load_from_file(&mut 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 { From 9222381769d1593b9d8a5fb3d993599181466824 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Sun, 25 May 2025 17:46:20 +0200 Subject: [PATCH 03/14] Update `self.vars` outside of `acquire_variables` This is done as a step towards enabling loading from the variables file without affecting internal variables, such that retrying becomes possible without issues. --- src/env_universal_common.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 6751e491a..ca2b856cf 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -407,7 +407,9 @@ fn load_from_file(&mut self, file: &File) -> Option { let callbacks = self.generate_callbacks_and_update_exports(&new_vars); // Acquire the new variables. - self.acquire_variables(new_vars); + self.acquire_variables(&mut new_vars); + // We have constructed all the callbacks and updated vars_to_acquire. Acquire it! + self.vars = new_vars; self.last_read_file_id = current_file_id; Some(callbacks) } @@ -613,9 +615,8 @@ fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> Call 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(&mut self, vars_to_acquire: &mut VarTable) { for key in &self.modified { match self.vars.get(key) { None => { @@ -628,9 +629,6 @@ fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) { } } } - - // We have constructed all the callbacks and updated vars_to_acquire. Acquire it! - self.vars = vars_to_acquire; } fn populate_1_variable( From b4d0538892136f1c5e15357d426527ee939f6333 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Sun, 25 May 2025 17:32:52 +0200 Subject: [PATCH 04/14] Reduce state mutation in `load_from_file` This is another step towards making the `load_from_file` function callable without modifying internal data of `self`. Instead, the required updates for a successful load should be returned. For now, `self.last_read_file_id` is still modified within `load_from_file`, which means it still needs a mutable reference to `self`. --- src/env_universal_common.rs | 94 ++++++++++++++++++++++++++++--------- 1 file changed, 71 insertions(+), 23 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index ca2b856cf..39c613173 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -83,6 +83,13 @@ pub struct EnvUniversal { 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 { @@ -229,13 +236,30 @@ pub fn sync(&mut self) -> (bool, Option) { return (false, None); }; - // Read from it. - let callbacks = self.load_from_file(&vars_file); - - if self.ok_to_save { - (self.save(&directory), callbacks) - } else { - (true, callbacks) + // TODO: proper locking + match self.load_from_file(&vars_file) { + 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; + if self.ok_to_save { + (self.save(&directory), Some(callbacks)) + } else { + (true, Some(callbacks)) + } + } + None => { + if self.ok_to_save { + (self.save(&directory), None) + } else { + (true, None) + } + } } } @@ -382,11 +406,27 @@ fn load_from_path_narrow(&mut self) -> Option { }; FLOG!(uvar_file, "universal log reading from file"); - self.load_from_file(&file) + // TODO: proper locking + match self.load_from_file(&file) { + 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; + Some(callbacks) + } + None => None, + } } /// Load environment variables from the opened [`file`](`File`). - fn load_from_file(&mut self, file: &File) -> Option { + /// If successful, [`None`] indicates that no updates were necessary, whereas the [`Some`] + /// variant contains updated data. + fn load_from_file(&mut 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 { @@ -399,19 +439,21 @@ fn load_from_file(&mut self, file: &File) -> Option { // 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 ok_to_save = format != UvarFormat::future; // Announce changes and update our exports generation. - let callbacks = self.generate_callbacks_and_update_exports(&new_vars); + let (export_generation_increment, callbacks) = + self.generate_callbacks_and_update_exports(&new_vars); // Acquire the new variables. self.acquire_variables(&mut new_vars); - // We have constructed all the callbacks and updated vars_to_acquire. Acquire it! - self.vars = new_vars; self.last_read_file_id = current_file_id; - Some(callbacks) + Some(UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save, + }) } } @@ -565,9 +607,15 @@ fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool { 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. - fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> CallbackDataList { + /// 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( + &self, + new_vars: &VarTable, + ) -> (u64, CallbackDataList) { + let mut export_generation_increment = 0; let mut callbacks = CallbackDataList::new(); // Construct callbacks for erased values. for (key, value) in &self.vars { @@ -583,7 +631,7 @@ fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> Call val: None, }); if value.exports() { - self.export_generation += 1; + export_generation_increment += 1; } } } @@ -602,7 +650,7 @@ fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> Call 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. @@ -612,11 +660,11 @@ fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> Call }); } } - callbacks + (export_generation_increment, callbacks) } /// Copy modified values from existing vars to `vars_to_acquire`. - fn acquire_variables(&mut self, vars_to_acquire: &mut VarTable) { + fn acquire_variables(&self, vars_to_acquire: &mut VarTable) { for key in &self.modified { match self.vars.get(key) { None => { From 537b1c3dd5de082f88cb8f36c8c89926aa96032d Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Sun, 25 May 2025 18:37:35 +0200 Subject: [PATCH 05/14] Extract code for creating temporary files There is no need for separate implementations for history and uvar file handling. Having a shared implementation ensures that creating temporary files is handled consistently and removes clutter from the source files which want to create temporary files. --- src/env_universal_common.rs | 61 ++++++++----------------------------- src/fallback.rs | 30 ------------------ src/fs.rs | 58 +++++++++++++++++++++++++++++++++++ src/history.rs | 21 ++----------- src/lib.rs | 1 + src/wutil/tests.rs | 11 +++---- 6 files changed, 79 insertions(+), 103 deletions(-) create mode 100644 src/fs.rs diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 39c613173..b47116f99 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -1,17 +1,16 @@ #![allow(clippy::bad_bit_mask)] use crate::common::{ - str2wcstring, timef, unescape_string, valid_var_name, wcs2zstring, UnescapeFlags, - UnescapeStringStyle, + timef, 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::create_temporary_file; 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::wcstringutil::{join_strings, 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, @@ -523,47 +522,7 @@ fn open_and_acquire_lock(&mut self) -> Option { } } - 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. + /// Writes our state to the [`file`](`File`). 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); @@ -795,10 +754,14 @@ fn save(&mut self, directory: &wstr) -> bool { 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; + let tmp_name_template = directory.to_owned() + L!("/fishd.tmp.XXXXXX"); + let (mut private_file, private_file_path) = match create_temporary_file(&tmp_name_template) + { + Ok(tmp_file_data) => tmp_file_data, + Err(e) => { + FLOG!(warning, "Could not create temporary file:", e); + return false; + } }; // unlink pfp upon failure. In case of success, it (already) won't exist. let delete_pfp = ScopeGuard::new(private_file_path, |path| { 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/fs.rs b/src/fs.rs new file mode 100644 index 000000000..3e926f510 --- /dev/null +++ b/src/fs.rs @@ -0,0 +1,58 @@ +use crate::{ + common::{str2wcstring, wcs2zstring}, + wchar::prelude::*, + FLOG, +}; +use errno::errno; +use std::{ffi::CString, fs::File, os::fd::FromRawFd}; + +// 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()))) +} diff --git a/src/history.rs b/src/history.rs index 89dafaba4..85eb4a9a4 100644 --- a/src/history.rs +++ b/src/history.rs @@ -24,7 +24,7 @@ //! considered preferable to hanging the the shell waiting for a lock. use crate::{ - common::cstr2wcstring, env::EnvVar, wcstringutil::trim, + common::cstr2wcstring, env::EnvVar, fs::create_temporary_file, wcstringutil::trim, wutil::fileid::file_id_for_path_or_error, }; use std::{ @@ -49,13 +49,9 @@ 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, @@ -684,7 +680,7 @@ fn save_internal_via_rewrite(&mut self) { wrealpath(&possibly_indirect_target_name).unwrap_or(possibly_indirect_target_name); // Make our temporary file - let Some((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { + let Ok((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { return; }; let mut done = false; @@ -1393,17 +1389,6 @@ unsafe fn unlock_file(file: &mut File) { } } -// 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 { // Assume that things with leading dashes aren't paths. return !(potential_path.is_empty() || potential_path.starts_with('-')); 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/wutil/tests.rs b/src/wutil/tests.rs index 46723affb..92d472c47 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 (_fd, 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 { From fbb2fcdb068f82dedee77cd3d0dcda4c5d7cfc19 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Mon, 26 May 2025 00:25:55 +0200 Subject: [PATCH 06/14] Improve error handling in `src/history.rs` This allows for more informative error handling. In some cases, the `?` operator can now be applied sensibly instead of more verbose local error handling. --- src/history.rs | 94 ++++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 41 deletions(-) diff --git a/src/history.rs b/src/history.rs index 85eb4a9a4..50ec6c76a 100644 --- a/src/history.rs +++ b/src/history.rs @@ -215,20 +215,27 @@ 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 { +/// Returns the path for the history file for the given `session_id` +/// if `session_id` is non-empty. +/// If it is empty, `Ok(None)` will be returned. +/// An error is returned if obtaining the data directory failed. +/// Because the `path_get_data` function does not return error information, +/// we cannot provide more detail about the reason for the failure here. +/// If `suffix` is provided, append that suffix to the path; this is used for temporary files. +fn history_filename(session_id: &wstr, suffix: &wstr) -> std::io::Result> { if session_id.is_empty() { - return None; + return Ok(None); } - let mut result = path_get_data()?; + let Some(mut result) = 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.")); + }; result.push('/'); result.push_utfstr(session_id); result.push_utfstr(L!("_history")); result.push_utfstr(suffix); - Some(result) + Ok(Some(result)) } pub type PathList = Vec; @@ -495,8 +502,8 @@ 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 { + if let Ok(Some(history_path)) = history_filename(&self.name, L!("")) { + let Ok(mut file) = wopen_cloexec(&history_path, OFlag::O_RDONLY, Mode::empty()) else { return; }; @@ -572,7 +579,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: Option<&mut 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. @@ -650,39 +661,34 @@ 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) -> std::io::Result<()> { + // 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 Ok(()); + }; + let tmp_name_template = history_filename(&self.name, L!(".XXXXXX"))?.unwrap(); + 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; - }; - // 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); // Make our temporary file - let Ok((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { - return; - }; + let (mut tmp_file, tmp_name) = create_temporary_file(&tmp_name_template)?; let mut done = false; for _i in 0..MAX_SAVE_TRIES { if done { @@ -704,8 +710,11 @@ fn save_internal_via_rewrite(&mut self) { .unwrap_or(INVALID_FILE_ID); // 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) { + if let Err(err) = + self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) + { // Failed to write, no good + FLOG!(history_file, "Error writing to temporary file:", err); break; } @@ -808,10 +817,17 @@ fn save_internal_via_rewrite(&mut self) { // file. self.clear_file_state(); } + Ok(()) } /// Saves history by appending to the file. fn save_internal_via_appending(&mut self) -> std::io::Result<()> { + // Get the path to the real history file. + let Some(history_path) = history_filename(&self.name, L!(""))? else { + return Ok(()); + }; + let history_path = wrealpath(&history_path).unwrap_or(history_path); + FLOGF!( history, "Saving %lu items via appending", @@ -823,13 +839,6 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // If the file is different (someone vacuumed it) then we need to update our mmap. let mut file_changed = false; - // 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(()); - }; - // 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. @@ -937,7 +946,7 @@ fn save(&mut self, vacuum: bool) { return; } - if history_filename(&self.name, L!("")).is_none() { + 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(); @@ -953,14 +962,16 @@ fn save(&mut self, vacuum: bool) { 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); + 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() { + FLOG!(history, "Rewriting history failed:", e) + } } } @@ -971,7 +982,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. @@ -1036,7 +1047,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_)) = history_filename(&self.name, L!("")) else { return true; }; @@ -1092,7 +1104,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)) = history_filename(&self.name, L!("")) { wunlink(&filename); } self.clear_file_state(); @@ -1113,7 +1125,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)) = history_filename(&self.name, L!("")) else { return; }; From da426a1b039365904bdbf0c059d37d2d742691c7 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Mon, 26 May 2025 00:58:51 +0200 Subject: [PATCH 07/14] Improve error handling in `src/history/file.rs` Use `std::io::Result` instead of `Option` as the return type where appropriate, to allow for better-informed error handling. Remove explicit checks about the length to be mapped being 0. From `mmap(3)`: > If len is zero, mmap() shall fail and no mapping shall be established. --- src/history.rs | 4 +- src/history/file.rs | 99 ++++++++++++++++++++++++++------------------- 2 files changed, 59 insertions(+), 44 deletions(-) diff --git a/src/history.rs b/src/history.rs index 50ec6c76a..c7df092c2 100644 --- a/src/history.rs +++ b/src/history.rs @@ -516,7 +516,7 @@ fn load_old_if_needed(&mut self) { // 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.file_contents = HistoryFileContents::create(&mut file).ok(); self.history_file_id = if self.file_contents.is_some() { file_id_for_file(&file) } else { @@ -592,7 +592,7 @@ fn rewrite_to_temporary_file( // 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) { + 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. diff --git a/src/history/file.rs b/src/history/file.rs index 7afa5a429..6d4be6e5b 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}, }; @@ -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,47 @@ 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(history_file: &mut 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 = |file: &mut 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))?; + read_zero_padded(&mut *file, region.as_mut())?; + 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. 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(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 +195,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 +235,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() { @@ -231,7 +246,7 @@ fn should_mmap() -> bool { path_get_config_remoteness() != DirRemoteness::remote } -/// Read from `fd` to fill `dest`, zeroing any unused space. +/// Read from `file` 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) { From a20712b51da050c543c098ee3b831b71b0e1d773 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Wed, 4 Jun 2025 00:11:32 +0200 Subject: [PATCH 08/14] Avoid mutable references for mmap creation This is preparatory work for refactoring the file synchronization approach. `read_exact` will fail if the file length does not match the expected one, which means zero padding is useless. (On any reasonable OS it was also useless before, because anonymous memory mapping would be zero-pages.) --- src/history.rs | 2 +- src/history/file.rs | 22 +++------------------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/history.rs b/src/history.rs index c7df092c2..35f576cff 100644 --- a/src/history.rs +++ b/src/history.rs @@ -516,7 +516,7 @@ fn load_old_if_needed(&mut self) { // 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).ok(); + self.file_contents = HistoryFileContents::create(&file).ok(); self.history_file_id = if self.file_contents.is_some() { file_id_for_file(&file) } else { diff --git a/src/history/file.rs b/src/history/file.rs index 6d4be6e5b..1659dcbd2 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -117,7 +117,7 @@ pub struct HistoryFileContents { impl HistoryFileContents { /// Construct a history file contents from a [`File`] reference. - pub fn create(history_file: &mut File) -> std::io::Result { + pub fn create(mut history_file: &File) -> std::io::Result { // Check that the file is seekable, and its size. let len: usize = match history_file.seek(SeekFrom::End(0))?.try_into() { Ok(len) => len, @@ -128,11 +128,11 @@ pub fn create(history_file: &mut File) -> std::io::Result { )) } }; - let map_anon = |file: &mut File, len: usize| -> std::io::Result { + 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))?; - read_zero_padded(&mut *file, region.as_mut())?; + file.read_exact(&mut region)?; Ok(region) }; let region = if should_mmap() { @@ -246,22 +246,6 @@ fn should_mmap() -> bool { path_get_config_remoteness() != DirRemoteness::remote } -/// Read from `file` 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(()) -} - fn replace_all(s: &mut Vec, needle: &[u8], replacement: &[u8]) { let mut offset = 0; while let Some(relpos) = subslice_position(&s[offset..], needle) { From 33b651ad91ee7a4015250411f4d522e166145e8a Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Wed, 4 Jun 2025 00:20:59 +0200 Subject: [PATCH 09/14] Get remoteness of correct directory The history file is stored in the data dir, not the config dir, so the remoteness of the former is the one which matters. --- src/history/file.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/history/file.rs b/src/history/file.rs index 1659dcbd2..e7cd671dd 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -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. @@ -243,7 +243,7 @@ fn should_mmap() -> bool { } // mmap only if we are known not-remote. - path_get_config_remoteness() != DirRemoteness::remote + path_get_data_remoteness() != DirRemoteness::remote } fn replace_all(s: &mut Vec, needle: &[u8], replacement: &[u8]) { From ccb9c8225ffb0785e25af277e7025f7d0404ab83 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Wed, 4 Jun 2025 00:40:53 +0200 Subject: [PATCH 10/14] Remove `CHAOS_MODE` This is dead code. It is never set to true. --- src/history.rs | 7 ------- src/tests/history.rs | 3 --- 2 files changed, 10 deletions(-) diff --git a/src/history.rs b/src/history.rs index 35f576cff..8db59cbfb 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1360,9 +1360,6 @@ unsafe fn maybe_lock_file(file: &mut File, lock_type: libc::c_int) -> bool { if ABANDONED_LOCKING.load() { return false; } - if CHAOS_MODE.load() { - return false; - } if path_get_data_remoteness() == DirRemoteness::remote { return false; } @@ -2084,7 +2081,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/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 From f438e80f9b9dcd6a7d31e60582c35f02ef6569c5 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Sun, 15 Jun 2025 18:21:02 +0200 Subject: [PATCH 11/14] Use shared file locking logic Both history files and universal variables files are accessed by multiple processes, which need a way to synchronize their accesses. This synchronization logic is mixed in with the logic for reading and updating the files' contents, which results in messy code, duplicated locking implementations, and inconsistencies. Moreover, the existing implementations are flawed which has resulted in file corruption (e.g. https://github.com/fish-shell/fish-shell/issues/10300). The new approach separates the synchronization logic from the rest. There are two approaches to synchronization. - The primary one is using `flock(2)` to lock the directory containing the file which requires synchronized access. We do not lock the file holding the data directly because this file might be replaced, which can result in locking succeeding when it should block. Locking the directory solves this problem. To avoid inconsistent file states, changes are first written to a temporary file, which is then renamed into place while still holding the lock. - In some situations `flock` locks are unavailable or take a long time. This mostly applies to remote file systems. If we think that a directory is located on a remote file system, we do not attempt to use `flock`. As a fallback, we have a lockless approach, which uses file metadata (device, inode, size, ctime, mtime) to identify file versions. We then read from the file, write the updated data to a temporary file, check if the file id for the path is the same as before we started reading, and if so we rename the temporary file such that it replaces the old file. Note that races are possible between the file id check and the rename syscall. If we detect a file id mismatch, we retry, up to a predetermined number of attempts. The operations which should be performed are passed to the functions handling synchronization as `Fn`s. Because we might have to run these operations repeatedly when retrying, they should be executable arbitrarily often without causing side-effects relevant to the program. This requires some changes to functions accessing these files. In many cases, they have to work with non-mutable references, which requires that they return the data which should be updated in the program state, instead of directly assigning to the appropriate location. Locking via `O_EXLOCK`, which was used for the universal variable file, is no longer supported. That version of locking locks the file operated on directly, instead of its directory. According to the man pages of {Open,Free,Net}BSD and macOS, these locks have flock semantics. So if flock is available, we can use it, and if it is not, the `O_EXLOCK` flag does not help. --- po/de.po | 52 ++-- po/en.po | 30 +-- po/fr.po | 42 ++-- po/pl.po | 26 +- po/pt_BR.po | 26 +- po/sv.po | 26 +- po/zh_CN.po | 52 ++-- src/env_universal_common.rs | 422 ++++++++----------------------- src/flog.rs | 2 + src/fs.rs | 397 ++++++++++++++++++++++++++++- src/history.rs | 489 ++++++++++-------------------------- src/history/file.rs | 6 +- src/path.rs | 2 +- src/wutil/tests.rs | 2 +- 14 files changed, 738 insertions(+), 836 deletions(-) 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_universal_common.rs b/src/env_universal_common.rs index b47116f99..c9e5e2628 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -1,40 +1,28 @@ #![allow(clippy::bad_bit_mask)] use crate::common::{ - 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::fds::{open_cloexec, wopen_cloexec}; use crate::flog::{FLOG, FLOGF}; -use crate::fs::create_temporary_file; +use crate::fs::{lock_and_load, rewrite_via_temporary_file, PotentialUpdate}; +use crate::global_safety::RelaxedAtomicBool; 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, 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::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(); +static LOCK_UVAR_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true); /// Callback data, reflecting a change in universal variables. +#[derive(Clone, Debug, Eq, PartialEq)] pub struct CallbackData { // The name of the variable. pub key: WString, @@ -72,13 +60,11 @@ 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, } @@ -99,7 +85,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, } } @@ -161,10 +146,10 @@ 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) -> Option { - // Set do_flock to false immediately if the default variable path is on a remote filesystem. + // Set `LOCK_UVAR_FILE` 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; + LOCK_UVAR_FILE.store(false); } self.initialize_at_path(default_vars_path()) } @@ -181,43 +166,16 @@ pub fn initialize_at_path(&mut self, path: WString) -> Option self.load_from_path() } - /// Reads and writes variables at the correct path. Returns true if modified variables were - /// written. + /// 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, 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() { let callbacks = self.load_from_path_narrow(); @@ -225,40 +183,60 @@ pub fn sync(&mut self) -> (bool, Option) { return (false, callbacks); } - let directory = wdirname(&self.vars_path).to_owned(); - FLOG!(uvar_file, "universal log performing full sync"); - // Open the file. - let Some(vars_file) = self.open_and_acquire_lock() else { - FLOG!(uvar_file, "universal log open_and_acquire_lock() failed"); - return (false, None); - }; - - // TODO: proper locking - match self.load_from_file(&vars_file) { - 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; - if self.ok_to_save { - (self.save(&directory), Some(callbacks)) - } else { - (true, Some(callbacks)) + 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, + }) } } - None => { - if self.ok_to_save { - (self.save(&directory), None) - } else { - (true, None) + }; + + let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone()); + match rewrite_via_temporary_file(&real_path, &LOCK_UVAR_FILE, 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) + } } } @@ -400,32 +378,46 @@ fn load_from_path_narrow(&mut self) -> Option { return None; } - let Ok(file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { - return None; - }; - FLOG!(uvar_file, "universal log reading from file"); - // TODO: proper locking - match self.load_from_file(&file) { - Some(UniversalReadUpdate { - export_generation_increment, - new_vars, - callbacks, - ok_to_save, - }) => { + match lock_and_load(&self.vars_path, &LOCK_UVAR_FILE, |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) } - None => None, + 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`). - /// If successful, [`None`] indicates that no updates were necessary, whereas the [`Some`] - /// variant contains updated data. - fn load_from_file(&mut self, file: &File) -> Option { + /// 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 { @@ -438,7 +430,7 @@ fn load_from_file(&mut self, file: &File) -> Option { // Hacky: if the read format is in the future, avoid overwriting the file: never try to // save. - let ok_to_save = format != UvarFormat::future; + let do_save = format != UvarFormat::future; // Announce changes and update our exports generation. let (export_generation_increment, callbacks) = @@ -446,126 +438,18 @@ fn load_from_file(&mut self, file: &File) -> Option { // Acquire the new variables. self.acquire_variables(&mut new_vars); - self.last_read_file_id = current_file_id; - Some(UniversalReadUpdate { - export_generation_increment, - new_vars, - callbacks, - ok_to_save, + 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); - } - } - } - - /// Writes our state to the [`file`](`File`). 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. /// Returns by how much the exports generation count should be incremented, as well as a @@ -746,85 +630,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 tmp_name_template = directory.to_owned() + L!("/fishd.tmp.XXXXXX"); - let (mut private_file, private_file_path) = match create_temporary_file(&tmp_name_template) - { - Ok(tmp_file_data) => tmp_file_data, - Err(e) => { - FLOG!(warning, "Could not create temporary file:", e); - 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. @@ -1005,29 +810,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/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 index 3e926f510..28afdb873 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1,10 +1,26 @@ use crate::{ - common::{str2wcstring, wcs2zstring}, + common::{str2wcstring, wcs2zstring, ScopeGuard}, + fds::wopen_cloexec, + global_safety::RelaxedAtomicBool, + path::{path_remoteness, DirRemoteness}, wchar::prelude::*, - FLOG, + wutil::{ + file_id_for_file, file_id_for_path, wdirname, wrename, wunlink, FileId, INVALID_FILE_ID, + }, + FLOG, FLOGF, }; use errno::errno; -use std::{ffi::CString, fs::File, os::fd::FromRawFd}; +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, @@ -56,3 +72,378 @@ pub fn create_temporary_file(name_template: &wstr) -> std::io::Result<(File, WSt }; 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, + locking_enabled: &RelaxedAtomicBool, + load: F, +) -> std::io::Result<(FileId, UserData)> +where + F: Fn(&File) -> std::io::Result, +{ + if locking_enabled.load() { + 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. + // Do not abandon locking is such cases. + 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); + } else { + FLOG!(synced_file_access, "Disabling locking."); + locking_enabled.store(false); + } + } + } + } + + 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. +/// - `locking_enabled`: To indicate whether locking should be attempted. This function might +/// update the value stored in `locking_enabled`. +/// - `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, + locking_enabled: &RelaxedAtomicBool, + 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. + if locking_enabled.load() { + // 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, + ); + locking_enabled.store(false); + } + } + } + + // 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 8db59cbfb..d180f0164 100644 --- a/src/history.rs +++ b/src/history.rs @@ -2,47 +2,41 @@ //! //! 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, fs::create_temporary_file, 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; @@ -60,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; @@ -135,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`. @@ -215,29 +196,6 @@ fn add_item(&mut self, item: HistoryItem) { } } -/// Returns the path for the history file for the given `session_id` -/// if `session_id` is non-empty. -/// If it is empty, `Ok(None)` will be returned. -/// An error is returned if obtaining the data directory failed. -/// Because the `path_get_data` function does not return error information, -/// we cannot provide more detail about the reason for the failure here. -/// If `suffix` is provided, append that suffix to the path; this is used for temporary files. -fn history_filename(session_id: &wstr, suffix: &wstr) -> std::io::Result> { - if session_id.is_empty() { - return Ok(None); - } - - let Some(mut result) = 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.")); - }; - - result.push('/'); - result.push_utfstr(session_id); - result.push_utfstr(L!("_history")); - result.push_utfstr(suffix); - Ok(Some(result)) -} - pub type PathList = Vec; pub type HistoryIdentifier = u64; @@ -406,11 +364,33 @@ 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 @@ -502,34 +482,22 @@ fn load_old_if_needed(&mut self) { self.loaded_old = true; let _profiler = TimeProfiler::new("load_old"); - if let Ok(Some(history_path)) = history_filename(&self.name, L!("")) { - let Ok(mut file) = wopen_cloexec(&history_path, 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(&file).ok(); - 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, + &LOCK_HISTORY_FILE, + 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(); } } @@ -578,10 +546,9 @@ 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>, + existing_file: &File, dst: &mut File, ) -> std::io::Result<()> { // We are reading FROM existing_file and writing TO dst @@ -591,32 +558,29 @@ fn rewrite_to_temporary_file( // 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 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 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); } } } @@ -669,13 +633,9 @@ fn rewrite_to_temporary_file( /// Saves history by rewriting the file. fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { - // 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 { + let Some(history_path) = self.history_file_path()? else { return Ok(()); }; - let tmp_name_template = history_filename(&self.name, L!(".XXXXXX"))?.unwrap(); FLOGF!( history, @@ -683,150 +643,43 @@ fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { self.new_items.len() - self.first_unwritten_new_item_index ); - // 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 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: (), + }) + }; - // Make our temporary file - let (mut tmp_file, tmp_name) = create_temporary_file(&tmp_name_template)?; - let mut done = false; - for _i in 0..MAX_SAVE_TRIES { - if done { - break; - } + let (file_id, _) = rewrite_via_temporary_file(&history_path, &LOCK_HISTORY_FILE, rewrite)?; + self.history_file_id = file_id; - 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've saved everything, so we have no more unsaved items. + self.first_unwritten_new_item_index = self.new_items.len(); - let orig_file_id = target_file_before - .as_ref() - .map(file_id_for_file) - .unwrap_or(INVALID_FILE_ID); + // We deleted our deleted items. + self.deleted_items.clear(); - // Open any target file, but do not lock it right away - if let Err(err) = - self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) - { - // Failed to write, no good - FLOG!(history_file, "Error writing to temporary file:", err); - break; - } + // Our history has been written to the file, so clear our state so we can re-reference the + // file. + self.clear_file_state(); - // 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<()> { - // Get the path to the real history file. - let Some(history_path) = history_filename(&self.name, L!(""))? else { + let Some(history_path) = self.history_file_path()? else { return Ok(()); }; - let history_path = wrealpath(&history_path).unwrap_or(history_path); + + if !LOCK_HISTORY_FILE.load() { + return Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "Appending is not supported when locking is disabled.", + )); + } FLOGF!( history, @@ -836,58 +689,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)?; - // 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(), - )?; + // 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; - // 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! // @@ -911,7 +726,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; } @@ -923,11 +739,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 { @@ -946,6 +762,9 @@ fn save(&mut self, vacuum: bool) { return; } + // 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(); @@ -953,16 +772,13 @@ fn save(&mut self, vacuum: bool) { self.clear_file_state(); } - // Compact our new items so we don't have duplicates. - self.compact_new_items(); - // 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 to history failed", e); + FLOG!(history, "Appending to history failed:", e); } else { ok = true; } @@ -1048,7 +864,7 @@ fn is_empty(&mut self) -> bool { // 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 Ok(Some(where_)) = history_filename(&self.name, L!("")) else { + let Ok(Some(where_)) = self.history_file_path() else { return true; }; @@ -1104,7 +920,7 @@ fn clear(&mut self) { self.deleted_items.clear(); self.first_unwritten_new_item_index = 0; self.old_item_offsets.clear(); - if let Ok(Some(filename)) = history_filename(&self.name, L!("")) { + if let Ok(Some(filename)) = self.history_file_path() { wunlink(&filename); } self.clear_file_state(); @@ -1125,7 +941,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 Ok(Some(new_file)) = history_filename(&self.name, L!("")) else { + let Ok(Some(new_file)) = self.history_file_path() else { return; }; @@ -1148,7 +964,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) => { @@ -1345,57 +1161,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 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); - } } fn string_could_be_path(potential_path: &wstr) -> bool { @@ -2081,3 +1846,5 @@ 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); + +static LOCK_HISTORY_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true); diff --git a/src/history/file.rs b/src/history/file.rs index e7cd671dd..44efe9013 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -144,7 +144,11 @@ pub fn create(mut history_file: &File) -> std::io::Result { // 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); + FLOG!( + history_file, + "Remote file system detected. Disabling history file locking.", + ); + super::LOCK_HISTORY_FILE.store(false); // Create an anonymous mapping and read() the file into it. map_anon(history_file, len)? } else { 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/wutil/tests.rs b/src/wutil/tests.rs index 92d472c47..7ac0ab701 100644 --- a/src/wutil/tests.rs +++ b/src/wutil/tests.rs @@ -61,7 +61,7 @@ fn test_wdirname_wbasename() { #[serial] fn test_wwrite_to_fd() { let _cleanup = test_init(); - let (_fd, filename) = create_temporary_file(L!("/tmp/fish_test_wwrite.XXXXXX")).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]; From 77738fd6466ac69504fb23baab4de3d3fa4e624e Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Wed, 4 Jun 2025 00:50:21 +0200 Subject: [PATCH 12/14] Remove obsolete comments If arguments always have the same value they do not need to be arguments. For `HistoryImpl::add`, the comments are incorrect (assuming they should indicate the value of the argument), since both arguments can be true or false independently. `History::add` is only called at one location in a test with a constant value of `false` for `pending`. This might mean that the parameter could be deleted, or maybe even the entire function, if testing can work without it. --- src/history.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/history.rs b/src/history.rs index d180f0164..a1ca59c25 100644 --- a/src/history.rs +++ b/src/history.rs @@ -395,12 +395,7 @@ fn history_file_path(&self) -> std::io::Result> { /// `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() { @@ -1283,7 +1278,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) } From 8cbcfc0b3a9e7ced5cd30e22a061183e86a83904 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Wed, 18 Jun 2025 21:00:33 +0200 Subject: [PATCH 13/14] Stop caching whether to lock The cached information might become outdated. It is important that all fish processes use the same mutual exclusion logic (either `flock`-based or the fallback), because the two methods do not provide mutual exclusion from one another. Avoiding caching makes the behavior independent on previous system states, resulting in fish instances performing file operations at the same time to use the same locking logic. More detailed discussion in https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2134543447 --- src/env_universal_common.rs | 13 +---- src/fs.rs | 95 +++++++++++++++---------------------- src/history.rs | 17 +------ src/history/file.rs | 9 +--- 4 files changed, 44 insertions(+), 90 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index c9e5e2628..e0b6e4578 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -6,9 +6,7 @@ use crate::env::{EnvVar, EnvVarFlags, VarTable}; use crate::flog::{FLOG, FLOGF}; use crate::fs::{lock_and_load, rewrite_via_temporary_file, PotentialUpdate}; -use crate::global_safety::RelaxedAtomicBool; 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, LineIterator}; use crate::wutil::{file_id_for_file, file_id_for_path_narrow, wrealpath, FileId, INVALID_FILE_ID}; @@ -19,8 +17,6 @@ use std::io::{Read, Write}; use std::mem::MaybeUninit; -static LOCK_UVAR_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true); - /// Callback data, reflecting a change in universal variables. #[derive(Clone, Debug, Eq, PartialEq)] pub struct CallbackData { @@ -146,11 +142,6 @@ 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) -> Option { - // Set `LOCK_UVAR_FILE` to false immediately if the default variable path is on a remote filesystem. - // See #7968. - if path_get_config_remoteness() == DirRemoteness::remote { - LOCK_UVAR_FILE.store(false); - } self.initialize_at_path(default_vars_path()) } @@ -213,7 +204,7 @@ pub fn sync(&mut self) -> (bool, Option) { }; let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone()); - match rewrite_via_temporary_file(&real_path, &LOCK_UVAR_FILE, rewrite) { + 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; @@ -379,7 +370,7 @@ fn load_from_path_narrow(&mut self) -> Option { } FLOG!(uvar_file, "universal log reading from file"); - match lock_and_load(&self.vars_path, &LOCK_UVAR_FILE, |f| { + match lock_and_load(&self.vars_path, |f| { Ok(self.load_from_file(f).map(|update| update.data)) }) { Ok(( diff --git a/src/fs.rs b/src/fs.rs index 28afdb873..dcd273cee 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1,7 +1,6 @@ use crate::{ common::{str2wcstring, wcs2zstring, ScopeGuard}, fds::wopen_cloexec, - global_safety::RelaxedAtomicBool, path::{path_remoteness, DirRemoteness}, wchar::prelude::*, wutil::{ @@ -182,39 +181,29 @@ pub fn get_mut(&mut self) -> &mut File { /// 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, - locking_enabled: &RelaxedAtomicBool, - load: F, -) -> std::io::Result<(FileId, UserData)> +pub fn lock_and_load(path: &wstr, load: F) -> std::io::Result<(FileId, UserData)> where F: Fn(&File) -> std::io::Result, { - if locking_enabled.load() { - 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. - // Do not abandon locking is such cases. - 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); - } else { - FLOG!(synced_file_access, "Disabling locking."); - locking_enabled.store(false); - } + 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); } } } @@ -266,8 +255,6 @@ pub struct PotentialUpdate { /// # Arguments /// /// - `path`: The path to the file which should be updated. -/// - `locking_enabled`: To indicate whether locking should be attempted. This function might -/// update the value stored in `locking_enabled`. /// - `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 @@ -283,7 +270,6 @@ pub struct PotentialUpdate { /// 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, - locking_enabled: &RelaxedAtomicBool, rewrite: F, ) -> std::io::Result<(FileId, PotentialUpdate)> where @@ -361,29 +347,26 @@ fn rename(old_name: &wstr, new_name: &wstr) -> std::io::Result<()> { // 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. - if locking_enabled.load() { - // 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, - ); - locking_enabled.store(false); + // 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, + ); } } diff --git a/src/history.rs b/src/history.rs index a1ca59c25..add7932e4 100644 --- a/src/history.rs +++ b/src/history.rs @@ -478,11 +478,7 @@ fn load_old_if_needed(&mut self) { let _profiler = TimeProfiler::new("load_old"); if let Ok(Some(history_path)) = self.history_file_path() { - match lock_and_load( - &history_path, - &LOCK_HISTORY_FILE, - HistoryFileContents::create, - ) { + 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; @@ -647,7 +643,7 @@ fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { }) }; - let (file_id, _) = rewrite_via_temporary_file(&history_path, &LOCK_HISTORY_FILE, rewrite)?; + let (file_id, _) = rewrite_via_temporary_file(&history_path, rewrite)?; self.history_file_id = file_id; // We've saved everything, so we have no more unsaved items. @@ -669,13 +665,6 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { return Ok(()); }; - if !LOCK_HISTORY_FILE.load() { - return Err(std::io::Error::new( - std::io::ErrorKind::Unsupported, - "Appending is not supported when locking is disabled.", - )); - } - FLOGF!( history, "Saving %lu items via appending", @@ -1841,5 +1830,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); - -static LOCK_HISTORY_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true); diff --git a/src/history/file.rs b/src/history/file.rs index 44efe9013..734635150 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -141,14 +141,7 @@ pub fn create(mut history_file: &File) -> std::io::Result { 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. - FLOG!( - history_file, - "Remote file system detected. Disabling history file locking.", - ); - super::LOCK_HISTORY_FILE.store(false); + // filesystem does not support mapping. // Create an anonymous mapping and read() the file into it. map_anon(history_file, len)? } else { From 977459949f16ff88b3b4100a47ffc16899df2e70 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Wed, 18 Jun 2025 21:04:27 +0200 Subject: [PATCH 14/14] Obtain history file path in `HistoryImpl::save` Eliminates some code duplication between the two different saving implementations. These changes are based on https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2149438316. One extra change is included here, namely the early return on an empty file name, indicating private mode. Without this, `history_path.unwrap()` fails in some tests. Returning early is probably what we want in such situations anyway. --- src/history.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/history.rs b/src/history.rs index add7932e4..383dbcb5e 100644 --- a/src/history.rs +++ b/src/history.rs @@ -623,11 +623,7 @@ fn rewrite_to_temporary_file( } /// Saves history by rewriting the file. - fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { - let Some(history_path) = self.history_file_path()? else { - return Ok(()); - }; - + fn save_internal_via_rewrite(&mut self, history_path: &wstr) -> std::io::Result<()> { FLOGF!( history, "Saving %lu items via rewrite", @@ -643,7 +639,7 @@ fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { }) }; - let (file_id, _) = rewrite_via_temporary_file(&history_path, rewrite)?; + let (file_id, _) = rewrite_via_temporary_file(history_path, rewrite)?; self.history_file_id = file_id; // We've saved everything, so we have no more unsaved items. @@ -660,11 +656,7 @@ fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { } /// Saves history by appending to the file. - fn save_internal_via_appending(&mut self) -> std::io::Result<()> { - let Some(history_path) = self.history_file_path()? else { - return Ok(()); - }; - + fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Result<()> { FLOGF!( history, "Saving %lu items via appending", @@ -674,7 +666,7 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { assert!(self.deleted_items.is_empty()); let mut locked_history_file = - LockedFile::new(LockingMode::Exclusive(WriteMethod::Append), &history_path)?; + LockedFile::new(LockingMode::Exclusive(WriteMethod::Append), history_path)?; // Check if the file was modified since it was last read. let file_id = file_id_for_file(locked_history_file.get()); @@ -754,14 +746,23 @@ fn save(&mut self, vacuum: bool) { self.first_unwritten_new_item_index = self.new_items.len(); self.deleted_items.clear(); self.clear_file_state(); + return; } + 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() { + if let Err(e) = self.save_internal_via_appending(&history_path) { FLOG!(history, "Appending to history failed:", e); } else { ok = true; @@ -769,7 +770,7 @@ fn save(&mut self, vacuum: bool) { } if !ok { // We did not or could not append; rewrite the file ("vacuum" it). - if let Err(e) = self.save_internal_via_rewrite() { + if let Err(e) = self.save_internal_via_rewrite(&history_path) { FLOG!(history, "Rewriting history failed:", e) } }