Convert fish_mkstemp_cloexec() to return an OwnedFd

Author: mqudsi

src/env_universal_common.rs

@@ -6,7 +6,6 @@ use crate::common::{
 };
 use crate::env::{EnvVar, EnvVarFlags, VarTable};
 use crate::fallback::fish_mkstemp_cloexec;
-use crate::fds::AutoCloseFd;
 use crate::fds::{open_cloexec, wopen_cloexec};
 use crate::flog::{FLOG, FLOGF};
 use crate::path::path_get_config;
@@ -493,39 +492,41 @@ impl EnvUniversal {
         res_fd
     }
 
-    fn open_temporary_file(&mut self, directory: &wstr, out_path: &mut WString) -> AutoCloseFd {
+    fn open_temporary_file(&mut self, directory: &wstr, out_path: &mut WString) -> OwnedFd {
         // 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.
         assert!(!string_suffixes_string(L!("/"), directory));
 
-        let mut saved_errno = Errno(0);
+        let mut attempt = 0;
         let tmp_name_template = directory.to_owned() + L!("/fishd.tmp.XXXXXX");
-        let mut result = AutoCloseFd::empty();
-        let mut narrow_str = CString::default();
-        for _attempt in 0..10 {
-            if result.is_valid() {
-                break;
+        let result = loop {
+            attempt += 1;
+            let result = fish_mkstemp_cloexec(wcs2zstring(&tmp_name_template));
+            match (result, attempt) {
+                (Ok(r), _) => break r,
+                (Err(e), 10) => {
+                    FLOG!(
+                        error,
+                        // We previously used to log a copy of the buffer we expected mk(o)stemp to
+                        // update with the new path, but mkstemp(3) says the contents of the buffer
+                        // are undefined in case of EEXIST, but left unchanged in case of EINVAL. So
+                        // just log the original template we pass in to the function instead.
+                        wgettext_fmt!(
+                            "Unable to create temporary file '%ls': %s",
+                            &tmp_name_template,
+                            e.to_string()
+                        )
+                    );
+                }
+                _ => continue,
             }
-            let (fd, tmp_name) = fish_mkstemp_cloexec(wcs2zstring(&tmp_name_template));
-            result.reset(fd);
-            narrow_str = tmp_name;
-            saved_errno = errno();
-        }
-        *out_path = str2wcstring(narrow_str.as_bytes());
+        };
 
-        if !result.is_valid() {
-            FLOG!(
-                error,
-                wgettext_fmt!(
-                    "Unable to open temporary file '%ls': %s",
-                    out_path,
-                    saved_errno.to_string()
-                )
-            );
-        }
-        result
+        *out_path = str2wcstring(result.1.as_bytes());
+        result.0
     }
+
     /// Writes our state to the fd. path is provided only for error reporting.
     fn write_to_fd(&mut self, fd: RawFd, path: &wstr) -> bool {
         assert!(fd >= 0);
@@ -753,19 +754,11 @@ impl EnvUniversal {
 
         // Open adjacent temporary file.
         let private_fd = self.open_temporary_file(directory, &mut private_file_path);
-        let mut success = private_fd.is_valid();
-
-        if !success {
-            FLOG!(uvar_file, "universal log open_temporary_file() failed");
-        }
 
         // Write to it.
-        if success {
-            assert!(private_fd.is_valid());
-            success = self.write_to_fd(private_fd.fd(), &private_file_path);
-            if !success {
-                FLOG!(uvar_file, "universal log write_to_fd() failed");
-            }
+        let mut success = self.write_to_fd(private_fd.as_raw_fd(), &private_file_path);
+        if !success {
+            FLOG!(uvar_file, "universal log write_to_fd() failed");
         }
 
         if success {
@@ -774,12 +767,12 @@ impl EnvUniversal {
             // Ensure we maintain ownership and permissions (#2176).
             // let mut sbuf : libc::stat = MaybeUninit::uninit();
             if let Ok(md) = wstat(&real_path) {
-                if unsafe { libc::fchown(private_fd.fd(), md.uid(), md.gid()) } == -1 {
+                if unsafe { libc::fchown(private_fd.as_raw_fd(), md.uid(), md.gid()) } == -1 {
                     FLOG!(uvar_file, "universal log fchown() failed");
                 }
                 #[allow(clippy::useless_conversion)]
                 let mode: libc::mode_t = md.mode().try_into().unwrap();
-                if unsafe { libc::fchmod(private_fd.fd(), mode) } == -1 {
+                if unsafe { libc::fchmod(private_fd.as_raw_fd(), mode) } == -1 {
                     FLOG!(uvar_file, "universal log fchmod() failed");
                 }
             }
@@ -802,7 +795,7 @@ impl EnvUniversal {
                 times[0].tv_nsec = libc::UTIME_OMIT; // don't change ctime
                 if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } != 0 {
                     unsafe {
-                        libc::futimens(private_fd.fd(), &times[0]);
+                        libc::futimens(private_fd.as_raw_fd(), &times[0]);
                     }
                 }
             }

src/fallback.rs

@@ -5,10 +5,12 @@
 
 use crate::widecharwidth::{WcLookupTable, WcWidth};
 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::sync::atomic::{AtomicIsize, Ordering};
-use std::{ffi::CString, mem, os::fd::RawFd};
+use std::{ffi::CString, mem};
 
 /// Width of ambiguous East Asian characters and, as of TR11, all private-use characters.
 /// 1 is the typical default, but we accept any non-negative override via `$fish_ambiguous_width`.
@@ -100,7 +102,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) -> (RawFd, CString) {
+pub fn fish_mkstemp_cloexec(name_template: CString) -> Result<(OwnedFd, CString), Errno> {
     let name = name_template.into_raw();
     #[cfg(not(target_os = "macos"))]
     let fd = {
@@ -116,7 +118,11 @@ pub fn fish_mkstemp_cloexec(name_template: CString) -> (RawFd, CString) {
         }
         fd
     };
-    (fd, unsafe { CString::from_raw(name) })
+    if fd == -1 {
+        Err(errno())
+    } else {
+        unsafe { Ok((OwnedFd::from_raw_fd(fd), CString::from_raw(name))) }
+    }
 }
 
 pub fn wcscasecmp(lhs: &wstr, rhs: &wstr) -> cmp::Ordering {

src/history.rs

@@ -25,7 +25,7 @@ use std::{
     mem,
     num::NonZeroUsize,
     ops::ControlFlow,
-    os::fd::{AsFd, AsRawFd, RawFd},
+    os::fd::{AsFd, AsRawFd, OwnedFd, RawFd},
     sync::{Arc, Mutex, MutexGuard},
     time::{Duration, SystemTime, UNIX_EPOCH},
 };
@@ -45,7 +45,7 @@ use crate::{
     env::{EnvMode, EnvStack, Environment},
     expand::{expand_one, ExpandFlags},
     fallback::fish_mkstemp_cloexec,
-    fds::{wopen_cloexec, AutoCloseFd},
+    fds::wopen_cloexec,
     flog::{FLOG, FLOGF},
     global_safety::RelaxedAtomicBool,
     history::file::{append_history_item_to_buffer, HistoryFileContents},
@@ -658,8 +658,7 @@ impl HistoryImpl {
         let Some((tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else {
             return;
         };
-        let tmp_fd = tmp_file.fd();
-        assert!(tmp_fd >= 0);
+        let tmp_fd = tmp_file.as_raw_fd();
         let mut done = false;
         for _i in 0..MAX_SAVE_TRIES {
             if done {
@@ -1363,13 +1362,11 @@ impl HistoryImpl {
 }
 
 // Returns the fd of an opened temporary file, or None on failure.
-fn create_temporary_file(name_template: &wstr) -> Option<(AutoCloseFd, WString)> {
+fn create_temporary_file(name_template: &wstr) -> Option<(OwnedFd, WString)> {
     for _attempt in 0..10 {
         let narrow_str = wcs2zstring(name_template);
-        let (fd, narrow_str) = fish_mkstemp_cloexec(narrow_str);
-        let out_fd = AutoCloseFd::new(fd);
-        if out_fd.is_valid() {
-            return Some((out_fd, str2wcstring(narrow_str.to_bytes())));
+        if let Ok((fd, narrow_str)) = fish_mkstemp_cloexec(narrow_str) {
+            return Some((fd, str2wcstring(narrow_str.to_bytes())));
         }
     }
     None

src/wutil/tests.rs

@@ -60,13 +60,8 @@ fn test_wdirname_wbasename() {
 #[serial]
 fn test_wwrite_to_fd() {
     test_init();
-    let (fd, filename) =
-        fish_mkstemp_cloexec(CString::new("/tmp/fish_test_wwrite.XXXXXX").unwrap());
-    {
-        let mut tmpfd = AutoCloseFd::new(fd);
-        assert!(tmpfd.is_valid());
-        tmpfd.close();
-    }
+    let (_fd, filename) =
+        fish_mkstemp_cloexec(CString::new("/tmp/fish_test_wwrite.XXXXXX").unwrap()).unwrap();
     let sizes = [1, 2, 3, 5, 13, 23, 64, 128, 255, 4096, 4096 * 2];
     for &size in &sizes {
         let fd = AutoCloseFd::new(unsafe {