Mirrors/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell synced 2025-01-13 21:44:16 +00:00
Code Issues Projects Releases Packages Wiki Activity

Revert "Lock history file before reading it"

Browse source 
Commit 5db0bd5 (Lock history file before reading it, 2024-10-09)
rewrites the history file in place instead of using rename().
By writing to the same file (with the same inode), it corrupts
our memory-mapped snapshot; mmap(3) says:

> It is unspecified whether modifications to the underlying object done
> after the MAP_PRIVATE mapping is established are visible through the
> MAP_PRIVATE mapping.

Revert it (it was misguided anyway).

Closes #10777
Closes #10782
This commit is contained in:
Johannes Altmanninger 2024-10-14 10:58:45 +02:00
parent fbf0ad98af
commit a2dc0ef377
3 changed files with 163 additions and 135 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

255
src/history.rs
View file

@ -141,6 +141,8 @@ const HISTORY_FILE_MODE: Mode = Mode::from_bits_truncate(0o600);
/// the file and taking the lock /// the file and taking the lock
const MAX_SAVE_TRIES: usize = 1024; const MAX_SAVE_TRIES: usize = 1024;
pub const VACUUM_FREQUENCY: usize = 25;
/// If the size of `buffer` is at least `min_size`, output the contents `buffer` to `fd`, /// If the size of `buffer` is at least `min_size`, output the contents `buffer` to `fd`,
/// and clear the string. /// and clear the string.
fn flush_to_fd(buffer: &mut Vec<u8>, fd: RawFd, min_size: usize) -> std::io::Result<()> { fn flush_to_fd(buffer: &mut Vec<u8>, fd: RawFd, min_size: usize) -> std::io::Result<()> {
@ -556,48 +558,46 @@ impl HistoryImpl {
usize::min(self.first_unwritten_new_item_index, self.new_items.len()); usize::min(self.first_unwritten_new_item_index, self.new_items.len());
} }
fn read_items(&self, existing_file: &mut File) -> LruCache<WString, HistoryItem> { /// Given the fd of an existing history file, write a new history file to `dst_fd`.
/// Returns false on error, true on success
fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut File) -> bool {
// We are reading FROM existing_fd and writing TO dst_fd
// Make an LRU cache to save only the last N elements. // Make an LRU cache to save only the last N elements.
let mut lru = LruCache::new(HISTORY_SAVE_MAX); let mut lru = LruCache::new(HISTORY_SAVE_MAX);
// Read in existing items (which may have changed out from underneath us, so don't trust our // Read in existing items (which may have changed out from underneath us, so don't trust our
// old file contents). // old file contents).
let Some(local_file) = HistoryFileContents::create(existing_file) else { if let Some(existing_file) = existing_file {
return lru; if let Some(local_file) = HistoryFileContents::create(existing_file) {
}; let mut cursor = 0;
let mut cursor = 0; while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) {
while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) { // Try decoding an old item.
// Try decoding an old item. let Some(old_item) = local_file.decode_item(offset) else {
let Some(old_item) = local_file.decode_item(offset) else { continue;
continue; };
};
// If old item is newer than session always erase if in deleted. // If old item is newer than session always erase if in deleted.
if old_item.timestamp() > self.boundary_timestamp { if old_item.timestamp() > self.boundary_timestamp {
if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) { if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) {
continue; 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);
}
} }
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);
} }
} }
lru
}
/// Write existing and new history history to the given file.
fn write_to_file(
&self,
existing_entries: LruCache<WString, HistoryItem>,
dst: &mut File,
) -> bool {
let mut lru = existing_entries;
// Insert any unwritten new items // Insert any unwritten new items
for item in self for item in self
.new_items .new_items
@ -670,104 +670,115 @@ impl HistoryImpl {
break; break;
} }
let mut target_file = match wopen_cloexec( let target_file_before = wopen_cloexec(
&target_name, &target_name,
OFlag::O_RDWR | OFlag::O_CREAT, OFlag::O_RDONLY | OFlag::O_CREAT,
HISTORY_FILE_MODE, HISTORY_FILE_MODE,
) { );
Ok(file) => file, if let Err(err) = target_file_before {
Err(err) => { FLOG!(history_file, "Error opening history file:", err);
FLOG!(history_file, "Error opening history file:", err);
break;
}
};
// Note any lock is released when the fd is closed.
let locked = unsafe { Self::maybe_lock_file(&mut target_file, LOCK_EX) };
let mut orig_file_id = INVALID_FILE_ID;
if !locked {
orig_file_id = file_id_for_fd(target_file.as_fd());
} }
if !self.write_to_file( let orig_file_id = target_file_before
self.read_items(&mut target_file), .as_ref()
if locked { .map(|fd| file_id_for_fd(fd.as_fd()))
rewind_file(&mut target_file); .unwrap_or(INVALID_FILE_ID);
&mut target_file
} else { // Open any target file, but do not lock it right away
&mut tmp_file if !self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) {
},
) {
// Failed to write, no good // Failed to write, no good
break; break;
} }
if locked { // The crux! We rewrote the history file; see if the history file changed while we
done = true; // were rewriting it. Make an effort to take the lock before checking, to avoid racing.
break; // 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
}
}
} }
drop(target_file); let can_replace_file = new_file_id == orig_file_id || new_file_id == INVALID_FILE_ID;
// The crux! We rewrote the history file; see if the history file changed while we were
// rewriting it. It's critical to check the file at the path, NOT based on our fd.
let can_replace_file = match file_id_for_path_or_error(&target_name) {
Ok(new_file_id) => new_file_id == orig_file_id,
Err(err) => {
// If the open fails, this may be because there is no current history.
if err.kind() != std::io::ErrorKind::NotFound {
FLOG!(history_file, "Error re-opening history file:", err);
}
true
}
};
if !can_replace_file { if !can_replace_file {
rewind_file(&mut tmp_file); // The file has changed, so we're going to re-read it
continue; // 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
// The file is unchanged, or the new file doesn't exist or we can't read it // Ensure we maintain the ownership and permissions of the original (#2355). If the
// Ensure we maintain the ownership and permissions of the original (#2355). If the // stat fails, we assume (hope) our default permissions are correct. This
// 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
// 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
// did, it would be tricky to set the permissions correctly. (bash doesn't get this // case right either).
// case right either). if let Ok(target_file_after) = target_file_after.as_ref() {
if let Ok(target_file_after) = if let Ok(md) = fstat(target_file_after.as_raw_fd()) {
wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty()) if unsafe { fchown(tmp_file.as_raw_fd(), md.uid(), md.gid()) } == -1 {
{ FLOG!(
if let Ok(md) = fstat(target_file_after.as_raw_fd()) { history_file,
if unsafe { fchown(tmp_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { "Error when changing ownership of history file:",
FLOG!( errno::errno()
history_file, );
"Error when changing ownership of history file:", }
errno::errno() #[allow(clippy::useless_conversion)]
); if unsafe { fchmod(tmp_file.as_raw_fd(), md.mode().try_into().unwrap()) }
} == -1
#[allow(clippy::useless_conversion)] {
if unsafe { fchmod(tmp_file.as_raw_fd(), md.mode().try_into().unwrap()) } == -1 FLOG!(
{ history_file,
FLOG!( "Error when changing mode of history file:",
history_file, errno::errno(),
"Error when changing mode of history file:", );
errno::errno(), }
);
} }
} }
}
// Slide it into place // Slide it into place
if wrename(&tmp_name, &target_name) == -1 { if wrename(&tmp_name, &target_name) == -1 {
FLOG!( FLOG!(
error, error,
wgettext_fmt!( wgettext_fmt!(
"Error when renaming history file: %s", "Error when renaming history file: %s",
errno::errno().to_string() errno::errno().to_string()
) )
); );
} }
// We did it // We did it
done = true; done = true;
}
} }
// Ensure we never leave the old file around // Ensure we never leave the old file around
@ -953,15 +964,14 @@ impl HistoryImpl {
// countdown at a random number so that even if the user never runs more than 25 commands, we'll // 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 // eventually vacuum. If countdown_to_vacuum is None, it means we haven't yet picked a value for
// the counter. // the counter.
let vacuum_frequency = 25;
let countdown_to_vacuum = self let countdown_to_vacuum = self
.countdown_to_vacuum .countdown_to_vacuum
.get_or_insert_with(|| get_rng().gen_range(0..vacuum_frequency)); .get_or_insert_with(|| get_rng().gen_range(0..VACUUM_FREQUENCY));
// Determine if we're going to vacuum. // Determine if we're going to vacuum.
let mut vacuum = false; let mut vacuum = false;
if *countdown_to_vacuum == 0 { if *countdown_to_vacuum == 0 {
*countdown_to_vacuum = vacuum_frequency; *countdown_to_vacuum = VACUUM_FREQUENCY;
vacuum = true; vacuum = true;
} }
@ -1375,17 +1385,6 @@ fn create_temporary_file(name_template: &wstr) -> Option<(File, WString)> {
None None
} }
fn rewind_file(file: &mut 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) = file.set_len(0) {
FLOG!(history_file, "Error when truncating history file:", err);
}
if let Err(err) = file.seek(SeekFrom::Start(0)) {
FLOG!(history_file, "Error resetting cursor in history file:", err);
}
}
fn string_could_be_path(potential_path: &wstr) -> bool { fn string_could_be_path(potential_path: &wstr) -> bool {
// Assume that things with leading dashes aren't paths. // Assume that things with leading dashes aren't paths.
return !(potential_path.is_empty() || potential_path.starts_with('-')); return !(potential_path.is_empty() || potential_path.starts_with('-'));

5
src/history/file.rs
View file

@ -361,10 +361,7 @@ fn extract_prefix_and_unescape_yaml(line: &[u8]) -> Option<(Cow<[u8]>, Cow<[u8]>
fn decode_item_fish_2_0(mut data: &[u8]) -> Option<HistoryItem> { fn decode_item_fish_2_0(mut data: &[u8]) -> Option<HistoryItem> {
let (advance, line) = read_line(data); let (advance, line) = read_line(data);
let line = trim_start(line); let line = trim_start(line);
// Check this early *before* anything else. assert!(line.starts_with(b"- cmd"));
if !line.starts_with(b"- cmd") {
return None;
}
let (_key, value) = extract_prefix_and_unescape_yaml(line)?; let (_key, value) = extract_prefix_and_unescape_yaml(line)?;

38
src/tests/history.rs
View file

@ -1,6 +1,8 @@
use crate::common::{is_windows_subsystem_for_linux, str2wcstring, wcs2osstring, wcs2string, WSL}; use crate::common::{is_windows_subsystem_for_linux, str2wcstring, wcs2osstring, wcs2string, WSL};
use crate::env::{EnvMode, EnvStack}; use crate::env::{EnvMode, EnvStack};
use crate::history::{self, History, HistoryItem, HistorySearch, PathList, SearchDirection}; use crate::history::{
self, History, HistoryItem, HistorySearch, PathList, SearchDirection, VACUUM_FREQUENCY,
};
use crate::path::path_get_data; use crate::path::path_get_data;
use crate::tests::prelude::*; use crate::tests::prelude::*;
use crate::tests::string_escape::ESCAPE_TEST_CHAR; use crate::tests::string_escape::ESCAPE_TEST_CHAR;
@ -12,8 +14,9 @@ use rand::Rng;
use std::collections::VecDeque; use std::collections::VecDeque;
use std::ffi::CString; use std::ffi::CString;
use std::io::BufReader; use std::io::BufReader;
use std::time::SystemTime; use std::sync::Arc;
use std::time::UNIX_EPOCH; use std::time::UNIX_EPOCH;
use std::time::{Duration, SystemTime};
fn history_contains(history: &History, txt: &wstr) -> bool { fn history_contains(history: &History, txt: &wstr) -> bool {
for i in 1.. { for i in 1.. {
@ -229,7 +232,7 @@ fn generate_history_lines(item_count: usize, idx: usize) -> Vec<WString> {
result result
} }
fn pound_on_history(item_count: usize, idx: usize) { fn pound_on_history(item_count: usize, idx: usize) -> Arc<History> {
// Called in child thread to modify history. // Called in child thread to modify history.
let hist = History::new(L!("race_test")); let hist = History::new(L!("race_test"));
let hist_lines = generate_history_lines(item_count, idx); let hist_lines = generate_history_lines(item_count, idx);
@ -237,6 +240,7 @@ fn pound_on_history(item_count: usize, idx: usize) {
hist.add_commandline(line); hist.add_commandline(line);
hist.save(); hist.save();
} }
hist
} }
#[test] #[test]
@ -340,6 +344,34 @@ fn test_history_races() {
hist.clear(); hist.clear();
} }
#[test]
#[serial]
fn test_history_external_rewrites() {
let _cleanup = test_init();
// Write some history to disk.
{
let hist = pound_on_history(VACUUM_FREQUENCY / 2, 0);
hist.add_commandline("needle".into());
hist.save();
}
std::thread::sleep(Duration::from_secs(1));
// Read history from disk.
let hist = History::new(L!("race_test"));
assert_eq!(hist.item_at_index(1).unwrap().str(), "needle");
// Add items until we rewrite the file.
// In practice this might be done by another shell.
pound_on_history(VACUUM_FREQUENCY, 0);
for i in 1.. {
if hist.item_at_index(i).unwrap().str() == "needle" {
break;
}
}
}
#[test] #[test]
#[serial] #[serial]
fn test_history_merge() { fn test_history_merge() {