From 6ef8125c9617b21917c379fecff063544cef22de Mon Sep 17 00:00:00 2001 From: PolyMeilex Date: Sun, 21 Jan 2024 01:31:32 +0100 Subject: [PATCH] Return `OwnedFd` from `open_cloexec` --- src/autoload.rs | 1 - src/builtins/cd.rs | 9 ++++----- src/builtins/source.rs | 26 +++++++++++++++----------- src/env_universal_common.rs | 13 ++++--------- src/exec.rs | 4 ++-- src/fds.rs | 13 ++++++------- src/history.rs | 18 ++++++------------ src/io.rs | 15 +++++++-------- src/parser.rs | 10 +++++----- src/reader.rs | 6 +++--- src/tests/history.rs | 4 ++-- 11 files changed, 54 insertions(+), 65 deletions(-) diff --git a/src/autoload.rs b/src/autoload.rs index 74f3c68f0..4b3b74d3d 100644 --- a/src/autoload.rs +++ b/src/autoload.rs @@ -350,7 +350,6 @@ fn touch_file(path: &wstr) { ) .unwrap(); write_loop(&fd, "Hello".as_bytes()).unwrap(); - unsafe { libc::close(fd) }; } let mut t1 = "/tmp/fish_test_autoload.XXXXXX\0".as_bytes().to_vec(); diff --git a/src/builtins/cd.rs b/src/builtins/cd.rs index fbba18eb9..059320816 100644 --- a/src/builtins/cd.rs +++ b/src/builtins/cd.rs @@ -3,7 +3,7 @@ use super::prelude::*; use crate::{ env::{EnvMode, Environment}, - fds::{wopen_cloexec, AutoCloseFd}, + fds::wopen_cloexec, path::path_apply_cdpath, wutil::{normalize_path, wperror, wreadlink}, }; @@ -87,9 +87,8 @@ pub fn cd(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> Optio errno::set_errno(Errno(0)); - let res = wopen_cloexec(&norm_dir, OFlag::O_RDONLY, Mode::empty()) - .map(AutoCloseFd::new) - .map_err(|err| err as i32); + let res = + wopen_cloexec(&norm_dir, OFlag::O_RDONLY, Mode::empty()).map_err(|err| err as i32); let res = res.and_then(|fd| { if unsafe { fchdir(fd.as_raw_fd()) } == 0 { @@ -100,7 +99,7 @@ pub fn cd(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> Optio }); let fd = match res { - Ok(raw_fd) => raw_fd, + Ok(fd) => fd, Err(err) => { // Some errors we skip and only report if nothing worked. // ENOENT in particular is very low priority diff --git a/src/builtins/source.rs b/src/builtins/source.rs index d39dbdd52..8f37674ec 100644 --- a/src/builtins/source.rs +++ b/src/builtins/source.rs @@ -1,4 +1,4 @@ -use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; +use std::os::fd::AsRawFd; use crate::{ common::{escape, scoped_push_replacer, FilenameRef}, @@ -51,17 +51,21 @@ pub fn source(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O func_filename = FilenameRef::new(L!("-").to_owned()); fd = streams.stdin_fd; } else { - let Ok(raw_fd) = wopen_cloexec(args[optind], OFlag::O_RDONLY, Mode::empty()) else { - let esc = escape(args[optind]); - streams.err.append(wgettext_fmt!( - "%ls: Error encountered while sourcing file '%ls':\n", - cmd, - &esc - )); - builtin_wperror(cmd, streams); - return STATUS_CMD_ERROR; + match wopen_cloexec(args[optind], OFlag::O_RDONLY, Mode::empty()) { + Ok(fd) => { + opened_fd = fd; + } + Err(_) => { + let esc = escape(args[optind]); + streams.err.append(wgettext_fmt!( + "%ls: Error encountered while sourcing file '%ls':\n", + cmd, + &esc + )); + builtin_wperror(cmd, streams); + return STATUS_CMD_ERROR; + } }; - opened_fd = unsafe { OwnedFd::from_raw_fd(raw_fd) }; fd = opened_fd.as_raw_fd(); let mut buf: libc::stat = unsafe { std::mem::zeroed() }; diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index b8673be91..8e809625b 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -25,7 +25,7 @@ use std::collections::HashSet; use std::ffi::CString; use std::mem::MaybeUninit; -use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}; +use std::os::fd::{AsRawFd, OwnedFd, RawFd}; use std::os::unix::prelude::MetadataExt; // Pull in the O_EXLOCK constant if it is defined, otherwise set it to 0. @@ -389,12 +389,10 @@ fn load_from_path_narrow(&mut self, callbacks: &mut CallbackDataList) -> bool { return true; } - let Ok(raw_fd) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) - else { + let Ok(fd) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { return false; }; - let fd = unsafe { std::os::fd::OwnedFd::from_raw_fd(raw_fd) }; FLOG!(uvar_file, "universal log reading from file"); self.load_from_fd(fd.as_raw_fd(), callbacks); true @@ -443,12 +441,12 @@ fn open_and_acquire_lock(&mut self) -> Option { let mut res_fd = None; while res_fd.is_none() { - let raw = match wopen_cloexec( + let fd = match wopen_cloexec( &self.vars_path, flags, Mode::S_IRUSR | Mode::S_IWUSR | Mode::S_IRGRP | Mode::S_IROTH, ) { - Ok(raw) => raw, + Ok(fd) => fd, Err(err) => { if err == nix::Error::EINTR { continue; // signaled; try again @@ -479,9 +477,6 @@ fn open_and_acquire_lock(&mut self) -> Option { } }; - assert!(raw >= 0, "Should have a valid fd here"); - let fd = unsafe { OwnedFd::from_raw_fd(raw) }; - // 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 { diff --git a/src/exec.rs b/src/exec.rs index 149e884f9..dc1bd092a 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -57,7 +57,7 @@ use nix::sys::stat; use std::ffi::CStr; use std::io::{Read, Write}; -use std::os::fd::{FromRawFd, RawFd}; +use std::os::fd::RawFd; use std::slice; use std::sync::atomic::Ordering; use std::sync::{atomic::AtomicUsize, Arc}; @@ -370,7 +370,7 @@ pub fn is_thompson_shell_script(path: &CStr) -> bool { 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 = unsafe { std::fs::File::from_raw_fd(fd) }; + let mut file = std::fs::File::from(fd); 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 f976d97ea..9dd48d046 100644 --- a/src/fds.rs +++ b/src/fds.rs @@ -226,12 +226,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(); @@ -241,11 +241,12 @@ 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) }); + match ret { - Ok(ret) => { + Ok(fd) => { set_errno(saved_errno); - return Ok(ret); + return Ok(fd); } Err(err) => { if err != nix::Error::EINTR || signal_check_cancel() != 0 { @@ -253,8 +254,6 @@ pub fn open_cloexec(path: &CStr, flags: OFlag, mode: nix::sys::stat::Mode) -> ni } } } - - } } diff --git a/src/history.rs b/src/history.rs index c168a2fff..f585f98e1 100644 --- a/src/history.rs +++ b/src/history.rs @@ -25,7 +25,7 @@ mem, num::NonZeroUsize, ops::ControlFlow, - os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}, + os::fd::{AsRawFd, RawFd}, sync::{Arc, Mutex, MutexGuard}, time::{Duration, SystemTime, UNIX_EPOCH}, }; @@ -478,12 +478,10 @@ fn load_old_if_needed(&mut self) { let _profiler = TimeProfiler::new("load_old"); if let Some(filename) = history_filename(&self.name, L!("")) { - let Ok(raw_fd) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { + let Ok(fd) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { return; }; - let fd = unsafe { OwnedFd::from_raw_fd(raw_fd) }; - // 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 @@ -675,8 +673,7 @@ fn save_internal_via_rewrite(&mut self) { &target_name, OFlag::O_RDONLY | OFlag::O_CREAT, HISTORY_FILE_MODE, - ) - .map(|raw_fd| unsafe { OwnedFd::from_raw_fd(raw_fd) }); + ); let orig_file_id = target_fd_before .as_ref() @@ -698,8 +695,7 @@ fn save_internal_via_rewrite(&mut self) { // 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()) - .map(|raw_fd| unsafe { OwnedFd::from_raw_fd(raw_fd) }); + 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, @@ -824,8 +820,6 @@ fn save_internal_via_appending(&mut self) -> bool { break; }; - let fd = unsafe { OwnedFd::from_raw_fd(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 // we may get interleaved history items, which is considered better than no history, or @@ -1114,7 +1108,7 @@ fn populate_from_config_path(&mut self) { return; }; - let mut src_fd = unsafe { std::fs::File::from_raw_fd(src_fd) }; + 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. @@ -1129,7 +1123,7 @@ fn populate_from_config_path(&mut self) { return; }; - let mut dst_fd = unsafe { std::fs::File::from_raw_fd(dst_fd) }; + 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) { diff --git a/src/io.rs b/src/io.rs index c140904c6..c1841b3f1 100644 --- a/src/io.rs +++ b/src/io.rs @@ -21,7 +21,7 @@ use nix::fcntl::OFlag; use nix::sys::stat::Mode; use std::cell::{RefCell, UnsafeCell}; -use std::os::fd::RawFd; +use std::os::fd::{AsRawFd, OwnedFd, RawFd}; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Condvar, Mutex, MutexGuard}; @@ -264,10 +264,10 @@ fn as_ptr(&self) -> *const () { pub struct IoFile { fd: RawFd, // The fd for the file which we are writing to or reading from. - file_fd: AutoCloseFd, + file_fd: OwnedFd, } impl IoFile { - pub fn new(fd: RawFd, file_fd: AutoCloseFd) -> Self { + pub fn new(fd: RawFd, file_fd: OwnedFd) -> Self { IoFile { fd, file_fd } // Invalid file redirections are replaced with a closed fd, so the following // assertion isn't guaranteed to pass: @@ -282,10 +282,10 @@ fn fd(&self) -> RawFd { self.fd } fn source_fd(&self) -> RawFd { - self.file_fd.fd() + self.file_fd.as_raw_fd() } fn print(&self) { - eprintf!("file %d -> %d\n", self.file_fd.fd(), self.fd) + eprintf!("file %d -> %d\n", self.file_fd.as_raw_fd(), self.fd) } fn as_ptr(&self) -> *const () { (self as *const Self).cast() @@ -661,9 +661,8 @@ pub fn append_from_specs(&mut self, specs: &RedirectionSpecList, pwd: &wstr) -> let oflags = spec.oflags(); match wopen_cloexec(&path, oflags, OPEN_MASK) { - Ok(raw_fd) => { - let file = AutoCloseFd::new(raw_fd); - self.push(Arc::new(IoFile::new(spec.fd, file))); + Ok(fd) => { + self.push(Arc::new(IoFile::new(spec.fd, fd))); } Err(err) => { if oflags.intersects(OFlag::O_EXCL) && err == nix::Error::EEXIST { diff --git a/src/parser.rs b/src/parser.rs index 8148e7a4d..475d0e37c 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -12,7 +12,7 @@ use crate::expand::{ expand_string, replace_home_directory_with_tilde, ExpandFlags, ExpandResultCode, }; -use crate::fds::{open_cloexec, AutoCloseFd}; +use crate::fds::open_cloexec; use crate::flog::FLOGF; use crate::function; use crate::global_safety::{RelaxedAtomicBool, SharedFromThis, SharedFromThisBase}; @@ -39,7 +39,7 @@ use printf_compat::sprintf; use std::cell::{Ref, RefCell, RefMut}; use std::ffi::{CStr, OsStr}; -use std::os::fd::{AsRawFd, RawFd}; +use std::os::fd::{AsRawFd, OwnedFd, RawFd}; use std::os::unix::prelude::OsStrExt; use std::pin::Pin; use std::rc::Rc; @@ -224,7 +224,7 @@ pub struct LibraryData { /// A file descriptor holding the current working directory, for use in openat(). /// This is never null and never invalid. - pub cwd_fd: Option>, + pub cwd_fd: Option>, pub status_vars: StatusVars, } @@ -364,8 +364,8 @@ pub fn new(variables: EnvStackRef, is_principal: bool) -> ParserRef { OFlag::O_RDONLY, Mode::empty(), ) { - Ok(raw_fd) => { - result.libdata_mut().cwd_fd = Some(Arc::new(AutoCloseFd::new(raw_fd))); + Ok(fd) => { + result.libdata_mut().cwd_fd = Some(Arc::new(fd)); } Err(_) => { perror("Unable to open the current working directory"); diff --git a/src/reader.rs b/src/reader.rs index febf586ee..20f7acd74 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -24,7 +24,7 @@ use std::io::BufReader; use std::num::NonZeroUsize; use std::ops::Range; -use std::os::fd::{FromRawFd, RawFd}; +use std::os::fd::RawFd; use std::pin::Pin; use std::rc::Rc; use std::sync::atomic::Ordering; @@ -4661,11 +4661,11 @@ fn import_history_if_necessary(&mut self) { var.map_or_else(|| L!("~/.bash_history").to_owned(), |var| var.as_string()); expand_tilde(&mut path, self.vars()); - let Ok(raw_fd) = wopen_cloexec(&path, OFlag::O_RDONLY, Mode::empty()) else { + let Ok(fd) = wopen_cloexec(&path, OFlag::O_RDONLY, Mode::empty()) else { return; }; - let file = unsafe { std::fs::File::from_raw_fd(raw_fd) }; + 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 ea6f29f29..cce7ed4e9 100644 --- a/src/tests/history.rs +++ b/src/tests/history.rs @@ -1,6 +1,6 @@ use crate::common::{is_windows_subsystem_for_linux, str2wcstring, wcs2osstring}; use crate::env::{EnvMode, EnvStack}; -use crate::fds::{wopen_cloexec, AutoCloseFd}; +use crate::fds::wopen_cloexec; use crate::history::{self, History, HistoryItem, HistorySearch, PathList, SearchDirection}; use crate::path::path_get_data; use crate::tests::prelude::*; @@ -594,7 +594,7 @@ fn test_history_formats() { "echo foo".into(), ]; let test_history_imported_from_bash = History::with_name(L!("bash_import")); - let file = AutoCloseFd::new( + let file = std::fs::File::from( wopen_cloexec( L!("tests/history_sample_bash"), OFlag::O_RDONLY,