mirror of
https://github.com/fish-shell/fish-shell
synced 2024-12-26 21:03:12 +00:00
Lock history file before reading it
We use optimistic concurrency when rewriting the history file to minimize the lock scope. Unfortunately, old.mtime == new.mtime does not imply that file is unchanged; we don't have guarantees on the granularity of the modification time timestamp, see https://stackoverflow.com/questions/14392975/timestamp-accuracy-on-ext4-sub-millsecond So let's lock before reading any old contents and use the other "write-to-tempfile-and-rename" code path only when locking fails. Potentially fixes #10300 (untested) which probably happens because read_zero_padded() attempts to read bytes that have not been flushed yet.
This commit is contained in:
Johannes Altmanninger
parent
35ee5e661f
commit
5db0bd5874
1 changed files with 131 additions and 129 deletions
260
src/history.rs
260
src/history.rs
|
|
@ -556,46 +556,48 @@ impl HistoryImpl {
|
|||
usize::min(self.first_unwritten_new_item_index, self.new_items.len());
|
||||
}
|
||||
|
||||
/// 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
|
||||
|
||||
fn read_items(&self, existing_file: &mut File) -> LruCache<WString, HistoryItem> {
|
||||
// Make an LRU cache to save only the last N elements.
|
||||
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
|
||||
// old file contents).
|
||||
if let Some(existing_file) = existing_file {
|
||||
if let Some(local_file) = HistoryFileContents::create(existing_file) {
|
||||
let mut cursor = 0;
|
||||
while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) {
|
||||
// Try decoding an old item.
|
||||
let Some(old_item) = local_file.decode_item(offset) else {
|
||||
continue;
|
||||
};
|
||||
let Some(local_file) = HistoryFileContents::create(existing_file) else {
|
||||
return lru;
|
||||
};
|
||||
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);
|
||||
}
|
||||
}
|
||||
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
|
||||
for item in self
|
||||
.new_items
|
||||
|
|
@ -668,115 +670,104 @@ impl HistoryImpl {
|
|||
break;
|
||||
}
|
||||
|
||||
let target_file_before = wopen_cloexec(
|
||||
let mut target_file = match wopen_cloexec(
|
||||
&target_name,
|
||||
OFlag::O_RDONLY | OFlag::O_CREAT,
|
||||
OFlag::O_RDWR | OFlag::O_CREAT,
|
||||
HISTORY_FILE_MODE,
|
||||
);
|
||||
if let Err(err) = target_file_before {
|
||||
FLOG!(history_file, "Error opening history file:", err);
|
||||
) {
|
||||
Ok(file) => file,
|
||||
Err(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());
|
||||
}
|
||||
|
||||
let orig_file_id = target_file_before
|
||||
.as_ref()
|
||||
.map(|fd| file_id_for_fd(fd.as_fd()))
|
||||
.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 !self.write_to_file(
|
||||
self.read_items(&mut target_file),
|
||||
if locked {
|
||||
rewind_file(&mut target_file);
|
||||
&mut target_file
|
||||
} else {
|
||||
&mut tmp_file
|
||||
},
|
||||
) {
|
||||
// Failed to write, no good
|
||||
break;
|
||||
}
|
||||
|
||||
// The crux! We rewrote the history file; see if the history file changed while we
|
||||
// were rewriting it. Make an effort to take the lock before checking, to avoid racing.
|
||||
// If the open fails, then proceed; this may be because there is no current history
|
||||
let mut new_file_id = INVALID_FILE_ID;
|
||||
|
||||
let mut target_file_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty());
|
||||
if let Ok(target_file_after) = target_file_after.as_mut() {
|
||||
// critical to take the lock before checking file IDs,
|
||||
// and hold it until after we are done replacing.
|
||||
// Also critical to check the file at the path, NOT based on our fd.
|
||||
// It's only OK to replace the file while holding the lock.
|
||||
// Note any lock is released when target_file_after is closed.
|
||||
unsafe {
|
||||
Self::maybe_lock_file(target_file_after, LOCK_EX);
|
||||
}
|
||||
new_file_id = match file_id_for_path_or_error(&target_name) {
|
||||
Ok(file_id) => file_id,
|
||||
Err(err) => {
|
||||
if err.kind() != std::io::ErrorKind::NotFound {
|
||||
FLOG!(history_file, "Error re-opening history file:", err);
|
||||
}
|
||||
INVALID_FILE_ID
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let can_replace_file = new_file_id == orig_file_id || new_file_id == INVALID_FILE_ID;
|
||||
if !can_replace_file {
|
||||
// The file has changed, so we're going to re-read it
|
||||
// Truncate our tmp_file so we can reuse it
|
||||
if let Err(err) = tmp_file.set_len(0) {
|
||||
FLOG!(
|
||||
history_file,
|
||||
"Error when truncating temporary history file:",
|
||||
err
|
||||
);
|
||||
}
|
||||
if let Err(err) = tmp_file.seek(SeekFrom::Start(0)) {
|
||||
FLOG!(
|
||||
history_file,
|
||||
"Error resetting cursor in temporary history file:",
|
||||
err
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// The file is unchanged, or the new file doesn't exist or we can't read it
|
||||
// We also attempted to take the lock, so we feel confident in replacing it
|
||||
|
||||
// Ensure we maintain the ownership and permissions of the original (#2355). If the
|
||||
// stat fails, we assume (hope) our default permissions are correct. This
|
||||
// corresponds to e.g. someone running sudo -E as the very first command. If they
|
||||
// did, it would be tricky to set the permissions correctly. (bash doesn't get this
|
||||
// case right either).
|
||||
if let Ok(target_file_after) = target_file_after.as_ref() {
|
||||
if let Ok(md) = fstat(target_file_after.as_raw_fd()) {
|
||||
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()
|
||||
);
|
||||
}
|
||||
#[allow(clippy::useless_conversion)]
|
||||
if unsafe { fchmod(tmp_file.as_raw_fd(), md.mode().try_into().unwrap()) }
|
||||
== -1
|
||||
{
|
||||
FLOG!(
|
||||
history_file,
|
||||
"Error when changing mode of history file:",
|
||||
errno::errno(),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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
|
||||
if locked {
|
||||
done = true;
|
||||
break;
|
||||
}
|
||||
|
||||
drop(target_file);
|
||||
|
||||
// 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 {
|
||||
rewind_file(&mut tmp_file);
|
||||
continue;
|
||||
}
|
||||
|
||||
// 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
|
||||
// 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) =
|
||||
wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty())
|
||||
{
|
||||
if let Ok(md) = fstat(target_file_after.as_raw_fd()) {
|
||||
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()
|
||||
);
|
||||
}
|
||||
#[allow(clippy::useless_conversion)]
|
||||
if unsafe { fchmod(tmp_file.as_raw_fd(), md.mode().try_into().unwrap()) } == -1
|
||||
{
|
||||
FLOG!(
|
||||
history_file,
|
||||
"Error when changing mode of history file:",
|
||||
errno::errno(),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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
|
||||
|
|
@ -1384,6 +1375,17 @@ fn create_temporary_file(name_template: &wstr) -> Option<(File, WString)> {
|
|||
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 {
|
||||
// Assume that things with leading dashes aren't paths.
|
||||
return !(potential_path.is_empty() || potential_path.starts_with('-'));
|
||||
|
|
|
|||
Loading…
Reference in a new issue