From 58a6eb6e45cbbf229d74196ecbe8bb83031c5a4d Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Tue, 5 Mar 2024 14:29:31 -0600 Subject: [PATCH] Convert fish_mkstemp_cloexec() to return an OwnedFd --- src/env_universal_common.rs | 71 +++++++++++++++++-------------------- src/fallback.rs | 12 +++++-- src/history.rs | 15 ++++---- src/wutil/tests.rs | 9 ++--- 4 files changed, 49 insertions(+), 58 deletions(-) diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 6777bd64b..ea036989d 100644 --- a/src/env_universal_common.rs +++ b/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(), ×[0]); + libc::futimens(private_fd.as_raw_fd(), ×[0]); } } } diff --git a/src/fallback.rs b/src/fallback.rs index ff898c8ce..f79387b58 100644 --- a/src/fallback.rs +++ b/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 { diff --git a/src/history.rs b/src/history.rs index f45837ab2..17557f428 100644 --- a/src/history.rs +++ b/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 diff --git a/src/wutil/tests.rs b/src/wutil/tests.rs index 223c6731f..d5b392e23 100644 --- a/src/wutil/tests.rs +++ b/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 {