Return OwnedFd from open_cloexec

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();

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())
+        let res =
-            .map(AutoCloseFd::new)
+            wopen_cloexec(&norm_dir, OFlag::O_RDONLY, Mode::empty()).map_err(|err| err as i32);
-            .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

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 {
+        match wopen_cloexec(args[optind], OFlag::O_RDONLY, Mode::empty()) {
-            let esc = escape(args[optind]);
+            Ok(fd) => {
-            streams.err.append(wgettext_fmt!(
+                opened_fd = fd;
-                "%ls: Error encountered while sourcing file '%ls':\n",
+            }
-                cmd,
+            Err(_) => {
-                &esc
+                let esc = escape(args[optind]);
-            ));
+                streams.err.append(wgettext_fmt!(
-            builtin_wperror(cmd, streams);
+                    "%ls: Error encountered while sourcing file '%ls':\n",
-            return STATUS_CMD_ERROR;
+                    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() };

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())
+        let Ok(fd) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else {
-        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<OwnedFd> {
 
         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<OwnedFd> {
                 }
             };
 
-            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 {

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]) {

src/fds.rs

@@ -226,12 +226,12 @@ pub fn wopen_cloexec(
     pathname: &wstr,
     flags: OFlag,
     mode: nix::sys::stat::Mode,
-) -> nix::Result<RawFd> {
+) -> nix::Result<OwnedFd> {
     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<RawFd> {
+pub fn open_cloexec(path: &CStr, flags: OFlag, mode: nix::sys::stat::Mode) -> nix::Result<OwnedFd> {
     // 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
                 }
             }
         }
-
-        
     }
 }
 

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())
+            let target_fd_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty());
-                .map(|raw_fd| unsafe { OwnedFd::from_raw_fd(raw_fd) });
 
             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) {

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) => {
+                        Ok(fd) => {
-                            let file = AutoCloseFd::new(raw_fd);
+                            self.push(Arc::new(IoFile::new(spec.fd, fd)));
-                            self.push(Arc::new(IoFile::new(spec.fd, file)));
                         }
                         Err(err) => {
                             if oflags.intersects(OFlag::O_EXCL) && err == nix::Error::EEXIST {

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<Arc<AutoCloseFd>>,
+    pub cwd_fd: Option<Arc<OwnedFd>>,
 
     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) => {
+            Ok(fd) => {
-                result.libdata_mut().cwd_fd = Some(Arc::new(AutoCloseFd::new(raw_fd)));
+                result.libdata_mut().cwd_fd = Some(Arc::new(fd));
             }
             Err(_) => {
                 perror("Unable to open the current working directory");

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));
         }
     }

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,