From decf99f71b2fb781013750898a33cd43847317f2 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 17 Mar 2024 11:20:44 -0500 Subject: [PATCH] Use `File` instead of `OwnedFd` in a few places (#10355) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a step towards converting `wopen_cloexec()` to return `File` instead of `OwnedFd`/`AutocloseFd`.¹ In addition to letting us use native standard library functions instead of unsafe libc calls, we gain additional semantic safety because `File` operations that manipulate the state of the fd (e.g. `File::seek()`) require a `&mut` reference to the `File`, whereas using `RawFd` or `OwnedFd` everywhere leaves us in a position where it's not clear whether or not other references to the same fd will manipulate its underlying state. ¹ We actually wouldn't even need `wopen_cloexec()` at all (just a widechar wrapper) as Rust's native `File::open()`/`File::create()` functionality uses `FD_CLOEXEC` internally. --- src/env_universal_common.rs | 3 +- src/fallback.rs | 7 +++-- src/history.rs | 57 ++++++++++++++++++------------------- src/history/file.rs | 39 ++++++++++--------------- 4 files changed, 49 insertions(+), 57 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 75003446e..e6113621a 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -22,6 +22,7 @@ use nix::{fcntl::OFlag, sys::stat::Mode}; use std::collections::hash_map::Entry; use std::collections::HashSet; use std::ffi::CString; +use std::fs::File; use std::mem::MaybeUninit; use std::os::fd::{AsFd, AsRawFd, OwnedFd, RawFd}; use std::os::unix::prelude::MetadataExt; @@ -496,7 +497,7 @@ impl EnvUniversal { &mut self, directory: &wstr, out_path: &mut WString, - ) -> Result { + ) -> 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. diff --git a/src/fallback.rs b/src/fallback.rs index f79387b58..1c560a3a3 100644 --- a/src/fallback.rs +++ b/src/fallback.rs @@ -8,7 +8,8 @@ use crate::{common::is_console_session, wchar::prelude::*}; use errno::{errno, Errno}; use once_cell::sync::Lazy; use std::cmp; -use std::os::fd::{FromRawFd, OwnedFd}; +use std::fs::File; +use std::os::fd::FromRawFd; use std::sync::atomic::{AtomicIsize, Ordering}; use std::{ffi::CString, mem}; @@ -102,7 +103,7 @@ pub fn fish_wcswidth(s: &wstr) -> isize { // 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<(OwnedFd, CString), Errno> { +pub fn fish_mkstemp_cloexec(name_template: CString) -> Result<(File, CString), Errno> { let name = name_template.into_raw(); #[cfg(not(target_os = "macos"))] let fd = { @@ -121,7 +122,7 @@ pub fn fish_mkstemp_cloexec(name_template: CString) -> Result<(OwnedFd, CString) if fd == -1 { Err(errno()) } else { - unsafe { Ok((OwnedFd::from_raw_fd(fd), CString::from_raw(name))) } + unsafe { Ok((File::from_raw_fd(fd), CString::from_raw(name))) } } } diff --git a/src/history.rs b/src/history.rs index cd547ce16..2ed1fd9fc 100644 --- a/src/history.rs +++ b/src/history.rs @@ -21,17 +21,18 @@ use std::{ borrow::Cow, collections::{BTreeMap, HashMap, HashSet, VecDeque}, ffi::CString, - io::{BufRead, Read, Write}, + fs::File, + io::{BufRead, Read, Seek, SeekFrom, Write}, mem, num::NonZeroUsize, ops::ControlFlow, - os::fd::{AsFd, AsRawFd, OwnedFd, RawFd}, + os::fd::{AsFd, AsRawFd, RawFd}, sync::{Arc, Mutex, MutexGuard}, time::{Duration, SystemTime, UNIX_EPOCH}, }; use bitflags::bitflags; -use libc::{fchmod, fchown, flock, fstat, ftruncate, lseek, LOCK_EX, LOCK_SH, LOCK_UN, SEEK_SET}; +use libc::{fchmod, fchown, flock, fstat, LOCK_EX, LOCK_SH, LOCK_UN}; use lru::LruCache; use nix::{fcntl::OFlag, sys::stat::Mode}; use rand::Rng; @@ -475,8 +476,12 @@ impl HistoryImpl { let _profiler = TimeProfiler::new("load_old"); if let Some(filename) = history_filename(&self.name, L!("")) { - let Ok(fd) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { - return; + let mut file = { + let Ok(fd) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { + return; + }; + // TODO: Return `File` directly from wopen_cloexec + File::from(fd) }; // Take a read lock to guard against someone else appending. This is released after @@ -487,16 +492,16 @@ impl HistoryImpl { // 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(&fd, LOCK_SH) }; - self.file_contents = HistoryFileContents::create(fd.as_raw_fd()); + let locked = unsafe { Self::maybe_lock_file(&file, LOCK_SH) }; + self.file_contents = HistoryFileContents::create(&mut file); self.history_file_id = if self.file_contents.is_some() { - file_id_for_fd(fd.as_raw_fd()) + file_id_for_fd(file.as_raw_fd()) } else { INVALID_FILE_ID }; if locked { unsafe { - Self::unlock_file(fd.as_raw_fd()); + Self::unlock_file(file.as_raw_fd()); } } @@ -551,7 +556,7 @@ impl HistoryImpl { /// 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_fd: Option, dst_fd: RawFd) -> bool { + fn rewrite_to_temporary_file(&self, existing_fd: Option<&mut File>, dst_fd: RawFd) -> bool { // We are reading FROM existing_fd and writing TO dst_fd // dst_fd must be valid assert!(dst_fd >= 0); @@ -654,37 +659,34 @@ impl HistoryImpl { wrealpath(&possibly_indirect_target_name).unwrap_or(possibly_indirect_target_name); // Make our temporary file - // Remember that we have to close this fd! - let Some((tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { + let Some((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { return; }; - let tmp_fd = tmp_file.as_raw_fd(); let mut done = false; for _i in 0..MAX_SAVE_TRIES { if done { break; } - let target_fd_before = wopen_cloexec( + let target_file_before = wopen_cloexec( &target_name, OFlag::O_RDONLY | OFlag::O_CREAT, HISTORY_FILE_MODE, - ); + ) + .map(File::from); - let orig_file_id = target_fd_before + let orig_file_id = target_file_before .as_ref() .map(|fd| file_id_for_fd(fd.as_raw_fd())) .unwrap_or(INVALID_FILE_ID); // Open any target file, but do not lock it right away - if !self.rewrite_to_temporary_file( - target_fd_before.as_ref().map(AsRawFd::as_raw_fd).ok(), - tmp_fd, - ) { + if !self + .rewrite_to_temporary_file(target_file_before.ok().as_mut(), tmp_file.as_raw_fd()) + { // Failed to write, no good break; } - drop(target_fd_before); // 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. @@ -692,7 +694,6 @@ impl HistoryImpl { let mut new_file_id = INVALID_FILE_ID; let target_fd_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty()); - if let Ok(target_fd_after) = target_fd_after.as_ref() { // critical to take the lock before checking file IDs, // and hold it until after we are done replacing. @@ -708,10 +709,8 @@ impl HistoryImpl { 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_fd so we can reuse it - if unsafe { ftruncate(tmp_fd, 0) } == -1 - || unsafe { lseek(tmp_fd, 0, SEEK_SET) } == -1 - { + // Truncate our tmp_file so we can reuse it + if tmp_file.set_len(0).is_err() || tmp_file.seek(SeekFrom::Start(0)).is_err() { FLOGF!( history_file, "Error %d when truncating temporary history file", @@ -730,14 +729,14 @@ impl HistoryImpl { if let Ok(target_fd_after) = target_fd_after.as_ref() { let mut sbuf: libc::stat = unsafe { mem::zeroed() }; if unsafe { fstat(target_fd_after.as_raw_fd(), &mut sbuf) } >= 0 { - if unsafe { fchown(tmp_fd, sbuf.st_uid, sbuf.st_gid) } == -1 { + if unsafe { fchown(tmp_file.as_raw_fd(), sbuf.st_uid, sbuf.st_gid) } == -1 { FLOGF!( history_file, "Error %d when changing ownership of history file", errno::errno().0 ); } - if unsafe { fchmod(tmp_fd, sbuf.st_mode) } == -1 { + if unsafe { fchmod(tmp_file.as_raw_fd(), sbuf.st_mode) } == -1 { FLOGF!( history_file, "Error %d when changing mode of history file", @@ -1364,7 +1363,7 @@ impl HistoryImpl { } // Returns the fd of an opened temporary file, or None on failure. -fn create_temporary_file(name_template: &wstr) -> Option<(OwnedFd, WString)> { +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) { diff --git a/src/history/file.rs b/src/history/file.rs index 46062fefd..489bc83cc 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -1,16 +1,14 @@ //! Implemention of history files. use std::{ - io::Write, + fs::File, + io::{Read, Seek, SeekFrom, Write}, ops::{Deref, DerefMut}, - os::fd::RawFd, + os::fd::{AsRawFd, RawFd}, time::{Duration, SystemTime, UNIX_EPOCH}, }; -use libc::{ - lseek, mmap, munmap, MAP_ANONYMOUS, MAP_FAILED, MAP_PRIVATE, PROT_READ, PROT_WRITE, SEEK_END, - SEEK_SET, -}; +use libc::{mmap, munmap, MAP_ANONYMOUS, MAP_FAILED, MAP_PRIVATE, PROT_READ, PROT_WRITE}; use super::{HistoryItem, PersistenceMode}; use crate::{ @@ -117,28 +115,21 @@ pub struct HistoryFileContents { } impl HistoryFileContents { - /// Construct a history file contents from a file descriptor. The file descriptor is not closed. - pub fn create(fd: RawFd) -> Option { + /// Construct a history file contents from a File reference. + pub fn create(file: &mut File) -> Option { // Check that the file is seekable, and its size. - let len = unsafe { lseek(fd, 0, SEEK_END) }; - let Ok(len) = usize::try_from(len) else { - return None; - }; + let len: usize = file.seek(SeekFrom::End(0)).ok()?.try_into().ok()?; let mmap_file_directly = should_mmap(); let mut region = if mmap_file_directly { - MmapRegion::map_file(fd, len)? + MmapRegion::map_file(file.as_raw_fd(), len) } else { - MmapRegion::map_anon(len)? - }; + MmapRegion::map_anon(len) + }?; // If we mapped anonymous memory, we have to read from the file. if !mmap_file_directly { - if unsafe { lseek(fd, 0, SEEK_SET) } != 0 { - return None; - } - if read_from_fd(fd, region.as_mut()).is_err() { - return None; - } + file.seek(SeekFrom::Start(0)).ok()?; + read_zero_padded(&mut *file, region.as_mut()).ok()?; } region.try_into().ok() @@ -228,14 +219,14 @@ fn should_mmap() -> bool { } /// Read from `fd` to fill `dest`, zeroing any unused space. -fn read_from_fd(fd: RawFd, mut dest: &mut [u8]) -> nix::Result<()> { +fn read_zero_padded(file: &mut File, mut dest: &mut [u8]) -> std::io::Result<()> { while !dest.is_empty() { - match nix::unistd::read(fd, dest) { + match file.read(dest) { Ok(0) => break, Ok(amt) => { dest = &mut dest[amt..]; } - Err(nix::Error::EINTR) => continue, + Err(e) if e.kind() == std::io::ErrorKind::Interrupted => continue, Err(err) => return Err(err), } }