From 0ca199ef98747c374c7d95114f29765a89076f7f Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 23 Mar 2024 01:34:23 -0500 Subject: [PATCH] Change `wopen_cloexec()` to return `File` --- src/autoload.rs | 3 +-- src/builtins/source.rs | 10 +++---- src/env_universal_common.rs | 6 ++--- src/exec.rs | 5 ++-- src/fds.rs | 11 ++++---- src/history.rs | 52 ++++++++++++++----------------------- src/io.rs | 3 +-- src/reader.rs | 4 +-- src/tests/history.rs | 14 +++++----- 9 files changed, 44 insertions(+), 64 deletions(-) diff --git a/src/autoload.rs b/src/autoload.rs index ef952835d..51f65f353 100644 --- a/src/autoload.rs +++ b/src/autoload.rs @@ -338,13 +338,12 @@ fn test_autoload() { use std::fs::File; use std::io::Write; - let fd = wopen_cloexec( + let mut file = wopen_cloexec( path, OFlag::O_RDWR | OFlag::O_CREAT, Mode::from_bits_truncate(0o666), ) .unwrap(); - let mut file = File::from(fd); file.write_all(b"Hello").unwrap(); } diff --git a/src/builtins/source.rs b/src/builtins/source.rs index f521e21d8..5f3bb84b9 100644 --- a/src/builtins/source.rs +++ b/src/builtins/source.rs @@ -28,8 +28,8 @@ pub fn source(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O return STATUS_CMD_OK; } - // If we open a file, this ensures we close it. - let opened_fd; + // If we open a file, this ensures it stays open until the end of scope. + let opened_file; // The fd that we read from, either from opened_fd or stdin. let fd; @@ -52,8 +52,8 @@ pub fn source(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O fd = streams.stdin_fd; } else { match wopen_cloexec(args[optind], OFlag::O_RDONLY, Mode::empty()) { - Ok(fd) => { - opened_fd = fd; + Ok(file) => { + opened_file = file; } Err(_) => { let esc = escape(args[optind]); @@ -67,7 +67,7 @@ pub fn source(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O } }; - fd = opened_fd.as_raw_fd(); + fd = opened_file.as_raw_fd(); let mut buf: libc::stat = unsafe { std::mem::zeroed() }; if unsafe { libc::fstat(fd, &mut buf) } == -1 { let esc = escape(args[optind]); diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 2d2d01bd6..2f9f98423 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -387,10 +387,10 @@ impl EnvUniversal { return true; } - let Ok(fd) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { + let Ok(mut file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) + else { return false; }; - let mut file = File::from(fd); FLOG!(uvar_file, "universal log reading from file"); self.load_from_fd(&mut file, callbacks); @@ -443,7 +443,7 @@ impl EnvUniversal { loop { let mut file = match wopen_cloexec(&self.vars_path, flags, Mode::from_bits_truncate(0o644)) { - Ok(fd) => File::from(fd), + Ok(file) => file, Err(nix::Error::EINTR) => continue, Err(err) => { if !O_EXLOCK.is_empty() { diff --git a/src/exec.rs b/src/exec.rs index 706df8b9b..a7a45f3aa 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -367,9 +367,8 @@ pub fn is_thompson_shell_script(path: &CStr) -> bool { } let e = errno(); let mut res = false; - let fd = open_cloexec(path, OFlag::O_RDONLY | OFlag::O_NOCTTY, stat::Mode::empty()); - if let Ok(fd) = fd { - let mut file = std::fs::File::from(fd); + if let Ok(mut file) = open_cloexec(path, OFlag::O_RDONLY | OFlag::O_NOCTTY, stat::Mode::empty()) + { let mut buf = [b'\0'; 256]; if let Ok(got) = file.read(&mut buf) { if is_thompson_shell_payload(&buf[..got]) { diff --git a/src/fds.rs b/src/fds.rs index 0803b520e..da412585f 100644 --- a/src/fds.rs +++ b/src/fds.rs @@ -10,6 +10,7 @@ use libc::{c_int, EINTR, FD_CLOEXEC, F_GETFD, F_GETFL, F_SETFD, F_SETFL, O_NONBL use nix::fcntl::FcntlArg; use nix::{fcntl::OFlag, unistd}; use std::ffi::CStr; +use std::fs::File; use std::io::{self, Read, Write}; use std::os::unix::prelude::*; @@ -228,12 +229,12 @@ pub fn wopen_cloexec( pathname: &wstr, flags: OFlag, mode: nix::sys::stat::Mode, -) -> nix::Result { +) -> nix::Result { open_cloexec(wcs2zstring(pathname).as_c_str(), flags, mode) } /// Narrow versions of wopen_cloexec(). -pub fn open_cloexec(path: &CStr, flags: OFlag, mode: nix::sys::stat::Mode) -> nix::Result { +pub fn open_cloexec(path: &CStr, flags: OFlag, mode: nix::sys::stat::Mode) -> nix::Result { // Port note: the C++ version of this function had a fallback for platforms where // O_CLOEXEC is not supported, using fcntl. In 2023, this is no longer needed. let saved_errno = errno(); @@ -243,11 +244,11 @@ pub fn open_cloexec(path: &CStr, flags: OFlag, mode: nix::sys::stat::Mode) -> ni // If it is that's our cancel signal, so we abort. loop { let ret = nix::fcntl::open(path, flags | OFlag::O_CLOEXEC, mode); - let ret = ret.map(|raw_fd| unsafe { OwnedFd::from_raw_fd(raw_fd) }); + let ret = ret.map(|raw_fd| unsafe { File::from_raw_fd(raw_fd) }); match ret { - Ok(fd) => { + Ok(file) => { set_errno(saved_errno); - return Ok(fd); + return Ok(file); } Err(err) => { if err != nix::Error::EINTR || signal_check_cancel() != 0 { diff --git a/src/history.rs b/src/history.rs index 2b3024539..622b65100 100644 --- a/src/history.rs +++ b/src/history.rs @@ -476,12 +476,8 @@ impl HistoryImpl { let _profiler = TimeProfiler::new("load_old"); if let Some(filename) = history_filename(&self.name, L!("")) { - 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) + let Ok(mut file) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { + return; }; // Take a read lock to guard against someone else appending. This is released after @@ -492,7 +488,7 @@ 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(&file, LOCK_SH) }; + let locked = unsafe { Self::maybe_lock_file(&mut file, LOCK_SH) }; self.file_contents = HistoryFileContents::create(&mut file); self.history_file_id = if self.file_contents.is_some() { file_id_for_fd(file.as_fd()) @@ -556,18 +552,16 @@ 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<&mut File>, dst_fd: RawFd) -> bool { + 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 - // dst_fd must be valid - assert!(dst_fd >= 0); // 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_fd) = existing_fd { - if let Some(local_file) = HistoryFileContents::create(existing_fd) { + 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. @@ -618,7 +612,7 @@ impl HistoryImpl { let mut buffer = Vec::with_capacity(HISTORY_OUTPUT_BUFFER_SIZE + 128); for item in items { append_history_item_to_buffer(&item, &mut buffer); - if let Err(e) = flush_to_fd(&mut buffer, dst_fd, HISTORY_OUTPUT_BUFFER_SIZE) { + if let Err(e) = flush_to_fd(&mut buffer, dst.as_raw_fd(), HISTORY_OUTPUT_BUFFER_SIZE) { err = Some(e); break; } @@ -632,7 +626,7 @@ impl HistoryImpl { false } else { - flush_to_fd(&mut buffer, dst_fd, 0).is_ok() + flush_to_fd(&mut buffer, dst.as_raw_fd(), 0).is_ok() } } @@ -672,8 +666,7 @@ impl HistoryImpl { &target_name, OFlag::O_RDONLY | OFlag::O_CREAT, HISTORY_FILE_MODE, - ) - .map(File::from); + ); let orig_file_id = target_file_before .as_ref() @@ -681,9 +674,7 @@ impl HistoryImpl { .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(), tmp_file.as_raw_fd()) - { + if !self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) { // Failed to write, no good break; } @@ -693,8 +684,8 @@ impl HistoryImpl { // If the open fails, then proceed; this may be because there is no current history 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() { + let mut target_fd_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty()); + if let Ok(target_fd_after) = target_fd_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. @@ -806,7 +797,7 @@ impl HistoryImpl { // Limit our max tries so we don't do this forever. let mut history_file = None; for _i in 0..MAX_SAVE_TRIES { - let Ok(fd) = wopen_cloexec( + let Ok(mut file) = wopen_cloexec( &history_path, OFlag::O_WRONLY | OFlag::O_APPEND, Mode::empty(), @@ -814,7 +805,6 @@ impl HistoryImpl { // can't open, we're hosed break; }; - let mut file = File::from(fd); // 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 @@ -1100,17 +1090,15 @@ impl HistoryImpl { old_file.push_utfstr(&self.name); old_file.push_str("_history"); - let Ok(src_fd) = wopen_cloexec(&old_file, OFlag::O_RDONLY, Mode::empty()) else { + let Ok(mut src_file) = wopen_cloexec(&old_file, OFlag::O_RDONLY, Mode::empty()) else { return; }; - let mut src_fd = std::fs::File::from(src_fd); - // Clear must come after we've retrieved the new_file name, and before we open // destination file descriptor, since it destroys the name and the file. self.clear(); - let Ok(dst_fd) = wopen_cloexec( + let Ok(mut dst_file) = wopen_cloexec( &new_file, OFlag::O_WRONLY | OFlag::O_CREAT, HISTORY_FILE_MODE, @@ -1119,15 +1107,13 @@ impl HistoryImpl { return; }; - let mut dst_fd = std::fs::File::from(dst_fd); - let mut buf = [0; libc::BUFSIZ as usize]; - while let Ok(n) = src_fd.read(&mut buf) { + while let Ok(n) = src_file.read(&mut buf) { if n == 0 { break; } - if dst_fd.write(&buf[..n]).is_err() { + if dst_file.write(&buf[..n]).is_err() { // This message does not have high enough priority to be shown by default. FLOG!(history_file, "Error when writing history file"); break; @@ -1317,10 +1303,10 @@ impl HistoryImpl { /// # Safety /// /// `fd` and `lock_type` must be valid arguments to `flock(2)`. - unsafe fn maybe_lock_file(fd: impl AsFd, lock_type: libc::c_int) -> bool { + 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"); - let raw_fd = fd.as_fd().as_raw_fd(); + let raw_fd = file.as_raw_fd(); // 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. diff --git a/src/io.rs b/src/io.rs index 1c9284d6f..bb68c007b 100644 --- a/src/io.rs +++ b/src/io.rs @@ -666,8 +666,7 @@ impl IoChain { let oflags = spec.oflags(); match wopen_cloexec(&path, oflags, OPEN_MASK) { - Ok(fd) => { - let file = File::from(fd); + Ok(file) => { self.push(Arc::new(IoFile::new(spec.fd, file))); } Err(err) => { diff --git a/src/reader.rs b/src/reader.rs index 721a344fe..c22ed8977 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -4665,11 +4665,9 @@ impl ReaderData { var.map_or_else(|| L!("~/.bash_history").to_owned(), |var| var.as_string()); expand_tilde(&mut path, self.vars()); - let Ok(fd) = wopen_cloexec(&path, OFlag::O_RDONLY, Mode::empty()) else { + let Ok(file) = wopen_cloexec(&path, OFlag::O_RDONLY, Mode::empty()) else { return; }; - - let file = std::fs::File::from(fd); self.history.populate_from_bash(BufReader::new(file)); } } diff --git a/src/tests/history.rs b/src/tests/history.rs index cce7ed4e9..94ff467e5 100644 --- a/src/tests/history.rs +++ b/src/tests/history.rs @@ -594,14 +594,12 @@ fn test_history_formats() { "echo foo".into(), ]; let test_history_imported_from_bash = History::with_name(L!("bash_import")); - let file = std::fs::File::from( - wopen_cloexec( - L!("tests/history_sample_bash"), - OFlag::O_RDONLY, - Mode::empty(), - ) - .unwrap(), - ); + let file = wopen_cloexec( + L!("tests/history_sample_bash"), + OFlag::O_RDONLY, + Mode::empty(), + ) + .unwrap(); test_history_imported_from_bash.populate_from_bash(BufReader::new(file)); assert_eq!(test_history_imported_from_bash.get_history(), expected); test_history_imported_from_bash.clear();