From 10eedfacc1fba607fa153d3686a2d0335d3b2146 Mon Sep 17 00:00:00 2001 From: Mason Mac <67773714+kirawi@users.noreply.github.com> Date: Tue, 30 Jul 2024 12:21:17 -0400 Subject: [PATCH 1/9] Ensure that temporary files written have the same metadata as the original --- Cargo.lock | 1 + helix-loader/src/lib.rs | 15 ++ helix-stdx/src/faccess.rs | 196 +++++++++++++++++++++----- helix-stdx/src/path.rs | 41 ++++++ helix-view/Cargo.toml | 1 + helix-view/src/document.rs | 279 ++++++++++++++++++++++++++++--------- helix-view/src/editor.rs | 30 ++++ 7 files changed, 466 insertions(+), 97 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7e80230d..7e445ba6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1457,6 +1457,7 @@ dependencies = [ "once_cell", "parking_lot", "rustix", + "same-file", "serde", "serde_json", "slotmap", diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index 0e7c134d..3d754cb8 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -132,6 +132,21 @@ pub fn cache_dir() -> PathBuf { path } +pub fn state_dir() -> PathBuf { + #[cfg(unix)] + { + let strategy = choose_base_strategy().expect("Unable to find the state directory!"); + let mut path = strategy.state_dir(); + path.push("helix"); + path + } + + #[cfg(windows)] + { + cache_dir() + } +} + pub fn config_file() -> PathBuf { CONFIG_FILE.get().map(|path| path.to_path_buf()).unwrap() } diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index e4c3daf2..1b305767 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -1,5 +1,6 @@ //! From +use std::fs::File; use std::io; use std::path::Path; @@ -24,7 +25,10 @@ bitflags! { mod imp { use super::*; - use rustix::fs::Access; + use rustix::{ + fd::AsFd, + fs::{Access, OpenOptionsExt}, + }; use std::os::unix::fs::{MetadataExt, PermissionsExt}; pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { @@ -50,13 +54,20 @@ mod imp { Ok(()) } - fn chown(p: &Path, uid: Option, gid: Option) -> io::Result<()> { + pub fn chown(p: &Path, uid: Option, gid: Option) -> io::Result<()> { let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) }); let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) }); rustix::fs::chown(p, uid, gid)?; Ok(()) } + pub fn fchown(fd: impl AsFd, uid: Option, gid: Option) -> io::Result<()> { + let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) }); + let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) }); + rustix::fs::fchown(fd, uid, gid)?; + Ok(()) + } + pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { let from_meta = std::fs::metadata(from)?; let to_meta = std::fs::metadata(to)?; @@ -79,15 +90,34 @@ mod imp { let metadata = p.metadata()?; Ok(metadata.nlink()) } + + pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result { + let from_meta = std::fs::metadata(from)?; + let mode = from_meta.permissions().mode(); + let file = std::fs::OpenOptions::new() + .mode(mode) + .read(true) + .write(true) + .create_new(true) + .open(to)?; + + // Change ownership + let from_meta = std::fs::metadata(from)?; + let uid = from_meta.uid(); + let gid = from_meta.gid(); + fchown(file.as_fd(), Some(uid), Some(gid))?; + Ok(file) + } } -// Licensed under MIT from faccess except for `chown`, `copy_metadata` and `is_acl_inherited` #[cfg(windows)] mod imp { - use windows_sys::Win32::Foundation::{CloseHandle, LocalFree, ERROR_SUCCESS, HANDLE}; + use windows_sys::Win32::Foundation::{ + CloseHandle, LocalFree, ERROR_SUCCESS, GENERIC_READ, GENERIC_WRITE, HANDLE, + }; use windows_sys::Win32::Security::Authorization::{ - GetNamedSecurityInfoW, SetNamedSecurityInfoW, SE_FILE_OBJECT, + GetNamedSecurityInfoW, SetSecurityInfo, SE_FILE_OBJECT, }; use windows_sys::Win32::Security::{ AccessCheck, AclSizeInformation, GetAce, GetAclInformation, GetSidIdentifierAuthority, @@ -100,7 +130,8 @@ mod imp { }; use windows_sys::Win32::Storage::FileSystem::{ GetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION, FILE_ACCESS_RIGHTS, - FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE, + FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE, WRITE_DAC, + WRITE_OWNER, }; use windows_sys::Win32::System::Threading::{GetCurrentThread, OpenThreadToken}; @@ -110,6 +141,7 @@ mod imp { use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt, io::AsRawHandle}; + // Licensed under MIT from faccess struct SecurityDescriptor { sd: PSECURITY_DESCRIPTOR, owner: PSID, @@ -128,6 +160,7 @@ mod imp { } impl SecurityDescriptor { + // Licensed under MIT from faccess fn for_path(p: &Path) -> io::Result { let path = std::fs::canonicalize(p)?; let pathos = path.into_os_string(); @@ -202,6 +235,7 @@ mod imp { } } + // Licensed under MIT from faccess struct ThreadToken(HANDLE); impl Drop for ThreadToken { fn drop(&mut self) { @@ -211,6 +245,7 @@ mod imp { } } + // Licensed under MIT from faccess impl ThreadToken { fn new() -> io::Result { unsafe { @@ -237,6 +272,7 @@ mod imp { } } + // Licensed under MIT from faccess // Based roughly on Tcl's NativeAccess() // https://github.com/tcltk/tcl/blob/2ee77587e4dc2150deb06b48f69db948b4ab0584/win/tclWinFile.c fn eaccess(p: &Path, mut mode: FILE_ACCESS_RIGHTS) -> io::Result<()> { @@ -330,6 +366,7 @@ mod imp { } } + // Licensed under MIT from faccess pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { let mut imode = 0; @@ -356,13 +393,8 @@ mod imp { } } - fn chown(p: &Path, sd: SecurityDescriptor) -> io::Result<()> { - let path = std::fs::canonicalize(p)?; - let pathos = path.as_os_str(); - let mut pathw = Vec::with_capacity(pathos.len() + 1); - pathw.extend(pathos.encode_wide()); - pathw.push(0); - + // SAFETY: It is the caller's responsibility to close the handle + fn chown(handle: HANDLE, sd: SecurityDescriptor) -> io::Result<()> { let mut owner = std::ptr::null_mut(); let mut group = std::ptr::null_mut(); let mut dacl = std::ptr::null(); @@ -387,8 +419,8 @@ mod imp { } let err = unsafe { - SetNamedSecurityInfoW( - pathw.as_ptr(), + SetSecurityInfo( + handle, SE_FILE_OBJECT, si, owner, @@ -405,9 +437,18 @@ mod imp { } } - pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { + pub fn copy_ownership(from: &Path, to: &Path) -> io::Result<()> { let sd = SecurityDescriptor::for_path(from)?; - chown(to, sd)?; + let to_file = std::fs::OpenOptions::new() + .read(true) + .access_mode(GENERIC_READ | GENERIC_WRITE | WRITE_OWNER | WRITE_DAC) + .open(to)?; + chown(to_file.as_raw_handle(), sd)?; + Ok(()) + } + + pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { + copy_ownership(from, to)?; let meta = std::fs::metadata(from)?; let perms = meta.permissions(); @@ -417,17 +458,42 @@ mod imp { Ok(()) } - pub fn hardlink_count(p: &Path) -> std::io::Result { - let file = std::fs::File::open(p)?; + fn file_info(p: &Path) -> io::Result { + let file = File::open(p)?; let handle = file.as_raw_handle(); let mut info: BY_HANDLE_FILE_INFORMATION = unsafe { std::mem::zeroed() }; if unsafe { GetFileInformationByHandle(handle, &mut info) } == 0 { - Err(std::io::Error::last_os_error()) + Err(io::Error::last_os_error()) } else { - Ok(info.nNumberOfLinks as u64) + Ok(info) } } + + pub fn hardlink_count(p: &Path) -> io::Result { + let n = file_info(p)?.nNumberOfLinks as u64; + Ok(n) + } + + pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result { + let sd = SecurityDescriptor::for_path(from)?; + + // read/write still need to be set to true or `create_new` returns an error + let to_file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .access_mode(GENERIC_READ | GENERIC_WRITE | WRITE_OWNER | WRITE_DAC) + .create_new(true) + .open(to)?; + + // Necessary because `security_attributes` is not exposed: https://github.com/rust-lang/libs-team/issues/314 + chown(to_file.as_raw_handle(), sd)?; + + let meta = std::fs::metadata(from)?; + let perms = meta.permissions(); + std::fs::set_permissions(to, perms)?; + Ok(to_file) + } } // Licensed under MIT from faccess except for `copy_metadata` @@ -453,14 +519,6 @@ mod imp { Err(io::Error::new(io::ErrorKind::NotFound, "Path not found")) } } - - pub fn copy_metadata(from: &path, to: &Path) -> io::Result<()> { - let meta = std::fs::metadata(from)?; - let perms = meta.permissions(); - std::fs::set_permissions(to, perms)?; - - Ok(()) - } } pub fn readonly(p: &Path) -> bool { @@ -471,10 +529,86 @@ pub fn readonly(p: &Path) -> bool { } } +pub fn hardlink_count(p: &Path) -> io::Result { + imp::hardlink_count(p) +} + pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { imp::copy_metadata(from, to) } -pub fn hardlink_count(p: &Path) -> io::Result { - imp::hardlink_count(p) +// /// Create a file copying permissions, uid, and gid of `from` at the the target destination `to` +// pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result { +// imp::create_copy_mode(from, to) +// } + +#[cfg(windows)] +pub fn copy_ownership(from: &Path, to: &Path) -> io::Result<()> { + imp::copy_ownership(from, to) } + +/* +Neovim backup path function: +- If a backup is desired (would be disabled by a user if using a file watcher): + - Checks if user explicitly requested a copy + - Or automatically choose whether to copy or rename + - Offers options for: + - Breaking symlinks or hardlinks (not offered in Helix) + - Offers the ability to have a list of directories where the backup file is written: + - Default is: ".,$XDG_STATE_HOME/nvim/backup//" + - Offers ability to set backup extension +- For copy backup: + - If the file is a link, then the backup will have the name of the link +- Auto backup: + - Immediately set copy if: + - Is hardlink or symlink + - Then, tries to: + - Create a temporary file with the same permissions as the file to test if its ok to rename later + - If it fails, then set copy + - fchown created file + - If it fails or perms weren't copied, then set copy + - Delete test file + - Otherwise, will rename +- Break symlink/hardlink if requested +- Copy backup: + - If there is an error while creating the file, it will be propogated unless force write is true + - Try to create backup path in bdir: + - Tries first directory where this is possible + - If no directory exists, the last directory is created + - Filename is escaped and extension applied + - Check if backup already exists: + - Check if pre-existing file is a symlink to the original file (and don't attempt to create one) + - Dunno what p_bk is, but if false, it tries to create a different backup file path where each character before the extension is changed (if all attempts fail, then error) + - Copies file with UV_FS_COPYFILE_FICLONE + - Sets perm as os_setperm(*backupp, perm & 0777); + - On Unix: + - Attempts to set gid via chown: + - os_setperm(*backupp, (perm & 0707) | ((perm & 07) << 3) if fails + - Sets file time: + os_file_settime(*backupp, + (double)file_info_old->stat.st_atim.tv_sec, + (double)file_info_old->stat.st_mtim.tv_sec); + - On Windows, sets ACL + - Attempts to copy xattr if exists +- Rename backup: + - Backup is created by renaming original file: + - Don't if file is read-only and cpoptions has "W" flag + - Tries to find backup file name w/ bdir (similar to copy) + - Checks if a file with that name already exists: + - Attempts same method as copy backup to create a different filename + +Neovim write: +- On Unix: + - If using :w! and file was read-only, make it writable (if process uid is same as file): +- Reset read-only flag if overwriting +- Executes fsync (will not propogate error if storage does not support op) +- Copies xattr for non-copy backups +- If a rename backup is being performed: + - Check if uid and gid are same as original file, and set if they aren't + - Set perms +- Either way, copy perms from old file to new file +- Either way, if not a backup copy, also set ACL (method seems to not do anything?) +- On failure: + - If a copy, copy contents from copy to original file + - Otherwise, rename backup back to original path +*/ diff --git a/helix-stdx/src/path.rs b/helix-stdx/src/path.rs index 72b233cc..74390212 100644 --- a/helix-stdx/src/path.rs +++ b/helix-stdx/src/path.rs @@ -8,6 +8,7 @@ use std::{ ffi::OsString, ops::Range, path::{Component, Path, PathBuf, MAIN_SEPARATOR_STR}, + str::Utf8Error, }; use crate::env::current_working_dir; @@ -295,6 +296,46 @@ pub fn expand + ?Sized>(path: &T) -> Cow<'_, Path> { } } +fn os_str_as_bytes>(path: P) -> Vec { + let path = path.as_ref(); + + #[cfg(windows)] + return path.to_str().unwrap().into(); + + #[cfg(unix)] + return std::os::unix::ffi::OsStrExt::as_bytes(path).to_vec(); +} + +fn path_from_bytes(slice: &[u8]) -> Result { + #[cfg(windows)] + return Ok(PathBuf::from(std::str::from_utf8(slice)?)); + + #[cfg(unix)] + return Ok(PathBuf::from( + ::from_bytes(slice), + )); +} + +fn is_sep_byte(b: u8) -> bool { + if cfg!(windows) { + b == b'/' || b == b'\\' + } else { + b == b'/' + } +} + +/// Replaces all path separators in a path with % +pub fn escape_path(path: &Path) -> PathBuf { + let s = path.as_os_str().to_os_string(); + let mut bytes = os_str_as_bytes(&s); + for b in bytes.iter_mut() { + if is_sep_byte(*b) { + *b = b'%'; + } + } + path_from_bytes(&bytes).unwrap() +} + #[cfg(test)] mod tests { use std::{ diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 6f71fa05..58ab50f6 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -29,6 +29,7 @@ anyhow = "1" crossterm = { version = "0.28", optional = true } tempfile = "3.14" +same-file = "1.0.1" # Conversion traits once_cell = "1.20" diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index fa089cda..d398b930 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1,5 +1,5 @@ use anyhow::{anyhow, bail, Error}; -use arc_swap::access::DynAccess; +use arc_swap::access::{Access, DynAccess}; use arc_swap::ArcSwap; use futures_util::future::BoxFuture; use futures_util::FutureExt; @@ -10,7 +10,7 @@ use helix_core::encoding::Encoding; use helix_core::syntax::{Highlight, LanguageServerFeature}; use helix_core::text_annotations::{InlineAnnotation, Overlay}; use helix_lsp::util::lsp_pos_to_pos; -use helix_stdx::faccess::{copy_metadata, readonly}; +use helix_stdx::faccess::readonly; use helix_vcs::{DiffHandle, DiffProviderRegistry}; use thiserror; @@ -37,6 +37,7 @@ use helix_core::{ ChangeSet, Diagnostic, LineEnding, Range, Rope, RopeBuilder, Selection, Syntax, Transaction, }; +use crate::editor::BackupConfig; use crate::{ editor::Config, events::{DocumentDidChange, SelectionDidChange}, @@ -130,6 +131,132 @@ pub enum DocumentOpenError { IoError(#[from] io::Error), } +struct Backup { + copy: bool, + path: PathBuf, +} + +impl Backup { + async fn from(p: PathBuf, config: &BackupConfig) -> Result { + use crate::editor::BackupKind; + use helix_stdx::faccess::*; + + // This fn won't be called if it's none or it doesn't exist + if config.kind == BackupKind::None || !p.exists() { + unreachable!(); + } + + let mut copy = config.kind == BackupKind::Copy; + + // Do not rename iff: + // - it is a hardlink + // - it is a symlink + // - we don't have file create perms for the dir + // TODO: also set copy when perms can't be set or metadata can't be read + if !copy { + // Conservatively assume it is a hardlink if we can't read metadata + let is_hardlink = { + let p_ = p.clone(); + tokio::task::spawn_blocking(move || hardlink_count(&p_).unwrap_or(2)).await? > 1 + }; + let is_symlink = tokio::fs::metadata(&p).await?.is_symlink(); + if is_hardlink || is_symlink { + copy = true; + } else { + // Check if we have write permissions by creating a temporary file + let mut builder = tempfile::Builder::new(); + // TODO: Need a way to create cross-platform perms with more granularity + // builder.permissions() + if let Ok(f) = builder.tempfile() { + // Check if we have perms to set perms + if copy_metadata(&p, f.path()).is_err() { + copy = true; + } + } else { + copy = true; + } + } + } + + // Look for valid backup directory + // Check if: + // - directory is not writable + // - path is a directory + // - path exists + let mut dir_exists = false; + let escaped_p = helix_stdx::path::escape_path(&p); + 'outer: for dir in config.directories.iter().filter(|p| p.is_dir()) { + let ext = config.extension.as_str(); + let bck_base_path = &dir.join(&escaped_p); + let mut backup = bck_base_path.join(&ext); + + // NOTE: Should we just overwrite regardless? + // If the backup file already exists, we'll try to add a number before the extension + // until we're done + // NOTE: u8 since if we need more than 256, there might be an issue + let mut n: u8 = 1; + while backup.exists() { + backup = bck_base_path.join(n.to_string()).join(&ext); + let Some(n) = n.checked_add(1) else { + continue 'outer; + }; + } + + if copy { + // Create the copy backup + // TODO: How handle error? + tokio::fs::copy(&p, &backup).await?; + + #[cfg(unix)] + { + let mut meta = tokio::fs::metadata(&p).await?; + let mut perms = meta.permissions(); + + // Strip s-bit + perms.set_mode(perms.mode() & 0o0777); + + let from_gid = from_meta.gid(); + let to_gid = to_meta.gid(); + + // If chown fails, se the protection bits for the roup the same as the perm bits for others + if from_gid != to_gid && chown(to, None, Some(from_gid)).is_err() { + let new_perms = (perms.mode() & 0o0707) | ((perms.mode() & 0o07) << 3); + perms.set_mode(new_perms); + } + std::fs::set_permissions(&backup, perms)?; + // TODO: Set time + // TODO: set xattr via rustix + } + + #[cfg(windows)] + { + let backup_ = backup.clone(); + tokio::task::spawn_blocking(move || -> std::io::Result<()> { + copy_ownership(&p, &backup_)?; + Ok(()) + }) + .await?; + } + + return Ok(Self { + copy: true, + path: backup, + }); + } else { + tokio::fs::rename(p, &backup).await?; + return Ok(Self { + copy: false, + path: backup, + }); + } + } + // TODO: Try to initialize last dir if none of the dirs exist + + // TODO + bail!("err"); + } +} + pub struct Document { pub(crate) id: DocumentId, text: Rope, @@ -892,6 +1019,8 @@ impl Document { let encoding_with_bom_info = (self.encoding, self.has_bom); let last_saved_time = self.last_saved_time; + let bck_config = self.config.clone().load().backup.clone(); + // We encode the file according to the `Document`'s encoding. let future = async move { use tokio::fs; @@ -935,84 +1064,102 @@ impl Document { )); } - // Assume it is a hardlink to prevent data loss if the metadata cant be read (e.g. on certain Windows configurations) - let is_hardlink = helix_stdx::faccess::hardlink_count(&write_path).unwrap_or(2) > 1; - let backup = if path.exists() { - let path_ = write_path.clone(); - // hacks: we use tempfile to handle the complex task of creating - // non clobbered temporary path for us we don't want - // the whole automatically delete path on drop thing - // since the path doesn't exist yet, we just want - // the path - tokio::task::spawn_blocking(move || -> Option { - let mut builder = tempfile::Builder::new(); - builder.prefix(path_.file_name()?).suffix(".bck"); + // Use a backup file + let mut bck = None; + let write_result = if bck_config.kind != crate::editor::BackupKind::None { + bck = Some(Backup::from(write_path.clone(), &bck_config).await?); + let bck = bck.unwrap(); - let backup_path = if is_hardlink { - builder - .make_in(path_.parent()?, |backup| std::fs::copy(&path_, backup)) - .ok()? - .into_temp_path() - } else { - builder - .make_in(path_.parent()?, |backup| std::fs::rename(&path_, backup)) - .ok()? - .into_temp_path() - }; + // SECURITY: Ensure that the created file has the same perms as the original file + let dst = if !bck.copy { + let from_meta = tokio::fs::metadata(&write_path).await?; + let mut open_opt = tokio::fs::OpenOptions::new() + .read(true) + .write(true) + .create_new(true); - backup_path.keep().ok() - }) - .await - .ok() - .flatten() - } else { - None - }; + #[cfg(unix)] + { + let mode = from_meta.permissions().mode(); + open_opt.mode(mode); + } + + let file = open_opt.open(&bck.path).await?; + let to_meta = file.metadata().await?; + + #[cfg(unix)] + { + // TODO: set gid/uid via fchown + } + + #[cfg(windows)] + { + let from = write_path.clone(); + let to = bck.path.clone(); + tokio::task::spawn_blocking(move || { + helix_stdx::faccess::copy_ownership(&from, &to)?; + Ok(()) + }) + .await?; + } + file + } else { + // SECURITY: Backup copy already exists + tokio::fs::File::create(&write_path).await? + }; - let write_result: anyhow::Result<_> = async { - let mut dst = tokio::fs::File::create(&write_path).await?; to_writer(&mut dst, encoding_with_bom_info, &text).await?; dst.sync_all().await?; Ok(()) - } - .await; + } else { + let dst = tokio::fs::File::create(&write_path).await?; + to_writer(&mut dst, encoding_with_bom_info, &text).await?; + dst.sync_all().await?; + Ok(()) + }; let save_time = match fs::metadata(&write_path).await { Ok(metadata) => metadata.modified().map_or(SystemTime::now(), |mtime| mtime), Err(_) => SystemTime::now(), }; - if let Some(backup) = backup { - if is_hardlink { - let mut delete = true; - if write_result.is_err() { - // Restore backup - let _ = tokio::fs::copy(&backup, &write_path).await.map_err(|e| { - delete = false; - log::error!("Failed to restore backup on write failure: {e}") - }); - } - - if delete { - // Delete backup - let _ = tokio::fs::remove_file(backup) + if let Some(bck) = bck { + /* + - If original file no longer exists, then backup is renamed to original file + - And the timestamp is preserved by setting timestmaps to prior to write + - Then backup is deleted + */ + let mut delete_bck = true; + if write_result.is_err() { + // If original file no longer exists, then backup is renamed to original file + if !write_path.exists() { + if !tokio::fs::rename(&bck.path, &write_path) .await - .map_err(|e| log::error!("Failed to remove backup file on write: {e}")); + .map_err(|e| { + delete_bck = false; + log::error!("Failed to restore backup on write failure: {e}") + }) + .is_err() + { + // TODO: Set timestamps to prior to write + } + } else { + if bck.copy { + // Restore backup from copy + let _ = tokio::fs::copy(&bck.path, &write_path).await.map_err(|e| { + delete_bck = false; + log::error!("Failed to restore backup on write failure: {e}") + }); + } else { + // restore backup + let _ = tokio::fs::rename(&bck.path, &write_path) + .await + .map_err(|e| { + delete_bck = false; + log::error!("Failed to restore backup on write failure: {e}") + }); + } } - } else if write_result.is_err() { - // restore backup - let _ = tokio::fs::rename(&backup, &write_path) - .await - .map_err(|e| log::error!("Failed to restore backup on write failure: {e}")); - } else { - // copy metadata and delete backup - let _ = tokio::task::spawn_blocking(move || { - let _ = copy_metadata(&backup, &write_path) - .map_err(|e| log::error!("Failed to copy metadata on write: {e}")); - let _ = std::fs::remove_file(backup) - .map_err(|e| log::error!("Failed to remove backup file on write: {e}")); - }) - .await; } } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 174190e5..25c06b21 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -355,6 +355,35 @@ pub struct Config { pub end_of_line_diagnostics: DiagnosticFilter, // Set to override the default clipboard provider pub clipboard_provider: ClipboardProvider, + pub backup: BackupConfig, +} + +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, PartialOrd, Ord)] +#[serde(rename_all = "kebab-case", default)] +pub struct BackupConfig { + pub kind: BackupKind, + pub directories: Vec, + pub extension: String, +} + +impl Default for BackupConfig { + fn default() -> Self { + Self { + kind: BackupKind::Auto, + // TODO: Prevent empty vector + directories: vec![helix_loader::state_dir().join("backup")], + // TODO: Prevent empty strings + extension: String::from("bck"), + } + } +} + +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, PartialOrd, Ord)] +#[serde(rename_all = "kebab-case")] +pub enum BackupKind { + None, + Copy, + Auto, } #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, PartialOrd, Ord)] @@ -995,6 +1024,7 @@ impl Default for Config { inline_diagnostics: InlineDiagnosticsConfig::default(), end_of_line_diagnostics: DiagnosticFilter::Disable, clipboard_provider: ClipboardProvider::default(), + backup: BackupConfig::default(), } } } From 877054bfaadf0b5f4791c8456c6e01a42f7f10f7 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Sun, 17 Nov 2024 00:27:52 -0500 Subject: [PATCH 2/9] copy xattr --- helix-loader/src/lib.rs | 2 +- helix-stdx/src/faccess.rs | 81 +++++++++++++++++++++++++++++++------- helix-view/src/document.rs | 34 ++++++++++++++-- 3 files changed, 98 insertions(+), 19 deletions(-) diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index 3d754cb8..ad715a11 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -136,7 +136,7 @@ pub fn state_dir() -> PathBuf { #[cfg(unix)] { let strategy = choose_base_strategy().expect("Unable to find the state directory!"); - let mut path = strategy.state_dir(); + let mut path = strategy.state_dir().unwrap(); path.push("helix"); path } diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index 1b305767..c3be68ac 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -54,20 +54,6 @@ mod imp { Ok(()) } - pub fn chown(p: &Path, uid: Option, gid: Option) -> io::Result<()> { - let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) }); - let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) }); - rustix::fs::chown(p, uid, gid)?; - Ok(()) - } - - pub fn fchown(fd: impl AsFd, uid: Option, gid: Option) -> io::Result<()> { - let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) }); - let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) }); - rustix::fs::fchown(fd, uid, gid)?; - Ok(()) - } - pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { let from_meta = std::fs::metadata(from)?; let to_meta = std::fs::metadata(to)?; @@ -547,6 +533,73 @@ pub fn copy_ownership(from: &Path, to: &Path) -> io::Result<()> { imp::copy_ownership(from, to) } +#[cfg(unix)] +pub fn chown(p: &Path, uid: Option, gid: Option) -> io::Result<()> { + let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) }); + let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) }); + rustix::fs::chown(p, uid, gid)?; + Ok(()) +} + +#[cfg(unix)] +pub fn fchown(fd: impl std::os::fd::AsFd, uid: Option, gid: Option) -> io::Result<()> { + let uid = uid.map(|n| unsafe { rustix::fs::Uid::from_raw(n) }); + let gid = gid.map(|n| unsafe { rustix::fs::Gid::from_raw(n) }); + rustix::fs::fchown(fd, uid, gid)?; + Ok(()) +} + +#[cfg(unix)] +pub fn copy_xattr(from: &Path, to: &Path) -> io::Result<()> { + let size = match rustix::fs::listxattr(from, &mut [])? { + 0 => return Ok(()), // No attributes + len => len, + }; + + let mut buf = vec![0i8; size]; + let read = rustix::fs::listxattr(from, &mut buf)?; + + fn i8_to_u8_slice(input: &[i8]) -> &[u8] { + // SAFETY: Simply reinterprets bytes + unsafe { std::slice::from_raw_parts(input.as_ptr() as *const u8, input.len()) } + } + + // Iterate over null-terminated C-style strings + // Two loops to avoid multiple allocations + // Find max-size for attributes + let mut max_attr_len = 0; + for attr_byte in buf.split(|&b| b == 0) { + let name = std::str::from_utf8(i8_to_u8_slice(attr_byte)) + .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?; + let attr_len = rustix::fs::getxattr(from, name, &mut [])?; + max_attr_len = max_attr_len.max(attr_len); + } + + let mut attr_buf = vec![0u8; max_attr_len]; + for attr_byte in buf.split(|&b| b == 0) { + let name = std::str::from_utf8(i8_to_u8_slice(attr_byte)) + .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?; + let read = rustix::fs::getxattr(from, name, &mut attr_buf)?; + + // If we can't set xattr because it already exists, try to replace it + if read != 0 { + match rustix::fs::setxattr(to, name, &attr_buf[..read], rustix::fs::XattrFlags::CREATE) + { + Err(rustix::io::Errno::EXIST) => rustix::fs::setxattr( + to, + name, + &attr_buf[..read], + rustix::fs::XattrFlags::REPLACE, + )?, + Err(e) => return Err(e.into()), + _ => {} + } + } + } + + Ok(()) +} + /* Neovim backup path function: - If a backup is desired (would be disabled by a user if using a file watcher): diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index d398b930..2cb1f598 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -23,6 +23,7 @@ use std::collections::HashMap; use std::fmt::Display; use std::future::Future; use std::io; +use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::{Arc, Weak}; @@ -169,6 +170,27 @@ impl Backup { // builder.permissions() if let Ok(f) = builder.tempfile() { // Check if we have perms to set perms + #[cfg(unix)] + { + use std::os::{fd::AsFd, unix::fs::MetadataExt}; + + let from_meta = tokio::fs::metadata(&p).await?; + let to_meta = tokio::fs::metadata(&f.path()).await?; + let _ = fchown( + f.as_file().as_fd(), + Some(from_meta.uid()), + Some(from_meta.gid()), + ); + + if from_meta.uid() != to_meta.uid() + || from_meta.gid() != to_meta.gid() + || from_meta.permissions() != to_meta.permissions() + { + copy = true; + } + } + + #[cfg(not(unix))] if copy_metadata(&p, f.path()).is_err() { copy = true; } @@ -209,23 +231,26 @@ impl Backup { #[cfg(unix)] { - let mut meta = tokio::fs::metadata(&p).await?; - let mut perms = meta.permissions(); + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let mut from_meta = tokio::fs::metadata(&p).await?; + let mut perms = from_meta.permissions(); // Strip s-bit perms.set_mode(perms.mode() & 0o0777); + let to_meta = tokio::fs::metadata(&backup).await?; let from_gid = from_meta.gid(); let to_gid = to_meta.gid(); // If chown fails, se the protection bits for the roup the same as the perm bits for others - if from_gid != to_gid && chown(to, None, Some(from_gid)).is_err() { + if from_gid != to_gid && chown(&backup, None, Some(from_gid)).is_err() { let new_perms = (perms.mode() & 0o0707) | ((perms.mode() & 0o07) << 3); perms.set_mode(new_perms); } std::fs::set_permissions(&backup, perms)?; // TODO: Set time - // TODO: set xattr via rustix + copy_xattr(&p, &backup)?; } #[cfg(windows)] @@ -1080,6 +1105,7 @@ impl Document { #[cfg(unix)] { + use std::os::unix::fs::PermissionsExt; let mode = from_meta.permissions().mode(); open_opt.mode(mode); } From 84247887a45b7532466a976a07890e4165abd4a3 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Sun, 17 Nov 2024 00:47:31 -0500 Subject: [PATCH 3/9] fix tests --- Cargo.lock | 1 + helix-stdx/src/faccess.rs | 143 ++--------------- helix-stdx/src/path.rs | 4 +- helix-term/src/application.rs | 8 + helix-term/tests/test/commands/write.rs | 3 + helix-view/Cargo.toml | 1 + helix-view/src/document.rs | 198 ++++++++++++------------ helix-view/src/editor.rs | 31 +++- 8 files changed, 161 insertions(+), 228 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7e445ba6..1e5eea46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1443,6 +1443,7 @@ dependencies = [ "chardetng", "clipboard-win", "crossterm", + "filetime", "futures-util", "helix-core", "helix-dap", diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index c3be68ac..ad2ffd60 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -1,6 +1,5 @@ //! From -use std::fs::File; use std::io; use std::path::Path; @@ -25,10 +24,7 @@ bitflags! { mod imp { use super::*; - use rustix::{ - fd::AsFd, - fs::{Access, OpenOptionsExt}, - }; + use rustix::fs::Access; use std::os::unix::fs::{MetadataExt, PermissionsExt}; pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { @@ -76,24 +72,6 @@ mod imp { let metadata = p.metadata()?; Ok(metadata.nlink()) } - - pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result { - let from_meta = std::fs::metadata(from)?; - let mode = from_meta.permissions().mode(); - let file = std::fs::OpenOptions::new() - .mode(mode) - .read(true) - .write(true) - .create_new(true) - .open(to)?; - - // Change ownership - let from_meta = std::fs::metadata(from)?; - let uid = from_meta.uid(); - let gid = from_meta.gid(); - fchown(file.as_fd(), Some(uid), Some(gid))?; - Ok(file) - } } #[cfg(windows)] @@ -125,6 +103,7 @@ mod imp { use std::ffi::c_void; + use std::fs::File; use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt, io::AsRawHandle}; // Licensed under MIT from faccess @@ -460,26 +439,6 @@ mod imp { let n = file_info(p)?.nNumberOfLinks as u64; Ok(n) } - - pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result { - let sd = SecurityDescriptor::for_path(from)?; - - // read/write still need to be set to true or `create_new` returns an error - let to_file = std::fs::OpenOptions::new() - .read(true) - .write(true) - .access_mode(GENERIC_READ | GENERIC_WRITE | WRITE_OWNER | WRITE_DAC) - .create_new(true) - .open(to)?; - - // Necessary because `security_attributes` is not exposed: https://github.com/rust-lang/libs-team/issues/314 - chown(to_file.as_raw_handle(), sd)?; - - let meta = std::fs::metadata(from)?; - let perms = meta.permissions(); - std::fs::set_permissions(to, perms)?; - Ok(to_file) - } } // Licensed under MIT from faccess except for `copy_metadata` @@ -523,11 +482,6 @@ pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { imp::copy_metadata(from, to) } -// /// Create a file copying permissions, uid, and gid of `from` at the the target destination `to` -// pub fn create_copy_mode(from: &Path, to: &Path) -> io::Result { -// imp::create_copy_mode(from, to) -// } - #[cfg(windows)] pub fn copy_ownership(from: &Path, to: &Path) -> io::Result<()> { imp::copy_ownership(from, to) @@ -556,30 +510,33 @@ pub fn copy_xattr(from: &Path, to: &Path) -> io::Result<()> { len => len, }; - let mut buf = vec![0i8; size]; - let read = rustix::fs::listxattr(from, &mut buf)?; - - fn i8_to_u8_slice(input: &[i8]) -> &[u8] { - // SAFETY: Simply reinterprets bytes - unsafe { std::slice::from_raw_parts(input.as_ptr() as *const u8, input.len()) } - } + let mut buf = vec![0; size]; + let read = rustix::fs::listxattr(from, buf.as_mut_slice())?; // Iterate over null-terminated C-style strings // Two loops to avoid multiple allocations // Find max-size for attributes let mut max_attr_len = 0; - for attr_byte in buf.split(|&b| b == 0) { - let name = std::str::from_utf8(i8_to_u8_slice(attr_byte)) + for attr_byte in buf[..read].split(|&b| b == 0) { + // handle platforms where c_char is i8 + #[allow(clippy::unnecessary_cast)] + let conv = + unsafe { std::slice::from_raw_parts(attr_byte.as_ptr() as *const u8, attr_byte.len()) }; + let name = std::str::from_utf8(conv) .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?; let attr_len = rustix::fs::getxattr(from, name, &mut [])?; max_attr_len = max_attr_len.max(attr_len); } let mut attr_buf = vec![0u8; max_attr_len]; - for attr_byte in buf.split(|&b| b == 0) { - let name = std::str::from_utf8(i8_to_u8_slice(attr_byte)) + for attr_byte in buf[..read].split(|&b| b == 0) { + // handle platforms where c_char is i8 + #[allow(clippy::unnecessary_cast)] + let conv = + unsafe { std::slice::from_raw_parts(attr_byte.as_ptr() as *const u8, attr_byte.len()) }; + let name = std::str::from_utf8(conv) .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?; - let read = rustix::fs::getxattr(from, name, &mut attr_buf)?; + let read = rustix::fs::getxattr(from, name, attr_buf.as_mut_slice())?; // If we can't set xattr because it already exists, try to replace it if read != 0 { @@ -599,69 +556,3 @@ pub fn copy_xattr(from: &Path, to: &Path) -> io::Result<()> { Ok(()) } - -/* -Neovim backup path function: -- If a backup is desired (would be disabled by a user if using a file watcher): - - Checks if user explicitly requested a copy - - Or automatically choose whether to copy or rename - - Offers options for: - - Breaking symlinks or hardlinks (not offered in Helix) - - Offers the ability to have a list of directories where the backup file is written: - - Default is: ".,$XDG_STATE_HOME/nvim/backup//" - - Offers ability to set backup extension -- For copy backup: - - If the file is a link, then the backup will have the name of the link -- Auto backup: - - Immediately set copy if: - - Is hardlink or symlink - - Then, tries to: - - Create a temporary file with the same permissions as the file to test if its ok to rename later - - If it fails, then set copy - - fchown created file - - If it fails or perms weren't copied, then set copy - - Delete test file - - Otherwise, will rename -- Break symlink/hardlink if requested -- Copy backup: - - If there is an error while creating the file, it will be propogated unless force write is true - - Try to create backup path in bdir: - - Tries first directory where this is possible - - If no directory exists, the last directory is created - - Filename is escaped and extension applied - - Check if backup already exists: - - Check if pre-existing file is a symlink to the original file (and don't attempt to create one) - - Dunno what p_bk is, but if false, it tries to create a different backup file path where each character before the extension is changed (if all attempts fail, then error) - - Copies file with UV_FS_COPYFILE_FICLONE - - Sets perm as os_setperm(*backupp, perm & 0777); - - On Unix: - - Attempts to set gid via chown: - - os_setperm(*backupp, (perm & 0707) | ((perm & 07) << 3) if fails - - Sets file time: - os_file_settime(*backupp, - (double)file_info_old->stat.st_atim.tv_sec, - (double)file_info_old->stat.st_mtim.tv_sec); - - On Windows, sets ACL - - Attempts to copy xattr if exists -- Rename backup: - - Backup is created by renaming original file: - - Don't if file is read-only and cpoptions has "W" flag - - Tries to find backup file name w/ bdir (similar to copy) - - Checks if a file with that name already exists: - - Attempts same method as copy backup to create a different filename - -Neovim write: -- On Unix: - - If using :w! and file was read-only, make it writable (if process uid is same as file): -- Reset read-only flag if overwriting -- Executes fsync (will not propogate error if storage does not support op) -- Copies xattr for non-copy backups -- If a rename backup is being performed: - - Check if uid and gid are same as original file, and set if they aren't - - Set perms -- Either way, copy perms from old file to new file -- Either way, if not a backup copy, also set ACL (method seems to not do anything?) -- On failure: - - If a copy, copy contents from copy to original file - - Otherwise, rename backup back to original path -*/ diff --git a/helix-stdx/src/path.rs b/helix-stdx/src/path.rs index 74390212..e68d2a19 100644 --- a/helix-stdx/src/path.rs +++ b/helix-stdx/src/path.rs @@ -318,7 +318,7 @@ fn path_from_bytes(slice: &[u8]) -> Result { fn is_sep_byte(b: u8) -> bool { if cfg!(windows) { - b == b'/' || b == b'\\' + b == b'/' || b == b'\\' || b == b':' } else { b == b'/' } @@ -327,7 +327,7 @@ fn is_sep_byte(b: u8) -> bool { /// Replaces all path separators in a path with % pub fn escape_path(path: &Path) -> PathBuf { let s = path.as_os_str().to_os_string(); - let mut bytes = os_str_as_bytes(&s); + let mut bytes = os_str_as_bytes(s); for b in bytes.iter_mut() { if is_sep_byte(*b) { *b = b'%'; diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index a567815f..f981fe82 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -234,6 +234,14 @@ impl Application { .unwrap_or_else(|_| editor.new_file(Action::VerticalSplit)); } + let bck_config = &config.load().editor.backup; + if bck_config.kind != helix_view::editor::BackupKind::None + && !bck_config.directories.iter().any(|p| p.exists()) + { + // Initialize last directory for backups + std::fs::create_dir_all(bck_config.directories.last().unwrap())?; + } + editor.set_theme(theme); #[cfg(windows)] diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index aba101e9..336ab2ab 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -539,6 +539,7 @@ async fn test_symlink_write() -> anyhow::Result<()> { let dir = tempfile::tempdir()?; let mut file = tempfile::NamedTempFile::new_in(&dir)?; + // NOTE: This will fail on Windows unless ran in administrator let symlink_path = dir.path().join("linked"); symlink(file.path(), &symlink_path)?; @@ -578,6 +579,7 @@ async fn test_symlink_write_fail() -> anyhow::Result<()> { let file = helpers::new_readonly_tempfile_in_dir(&dir)?; let symlink_path = dir.path().join("linked"); + // NOTE: This will fail on Windows unless ran in administrator symlink(file.path(), &symlink_path)?; let mut app = helpers::AppBuilder::new() @@ -622,6 +624,7 @@ async fn test_symlink_write_relative() -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new_in(&inner_dir)?; let symlink_path = dir.path().join("linked"); let relative_path = std::path::PathBuf::from("b").join(file.path().file_name().unwrap()); + // NOTE: This will fail on Windows unless ran in administrator symlink(relative_path, &symlink_path)?; let mut app = helpers::AppBuilder::new() diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 58ab50f6..f44ba1a9 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -30,6 +30,7 @@ crossterm = { version = "0.28", optional = true } tempfile = "3.14" same-file = "1.0.1" +filetime = "0.2" # Conversion traits once_cell = "1.20" diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 2cb1f598..2b9d2235 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1,6 +1,7 @@ use anyhow::{anyhow, bail, Error}; -use arc_swap::access::{Access, DynAccess}; +use arc_swap::access::DynAccess; use arc_swap::ArcSwap; +use filetime::FileTime; use futures_util::future::BoxFuture; use futures_util::FutureExt; use helix_core::auto_pairs::AutoPairs; @@ -23,7 +24,6 @@ use std::collections::HashMap; use std::fmt::Display; use std::future::Future; use std::io; -use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::{Arc, Weak}; @@ -153,7 +153,6 @@ impl Backup { // - it is a hardlink // - it is a symlink // - we don't have file create perms for the dir - // TODO: also set copy when perms can't be set or metadata can't be read if !copy { // Conservatively assume it is a hardlink if we can't read metadata let is_hardlink = { @@ -165,16 +164,16 @@ impl Backup { copy = true; } else { // Check if we have write permissions by creating a temporary file + let from_meta = tokio::fs::metadata(&p).await?; + let perms = from_meta.permissions(); let mut builder = tempfile::Builder::new(); - // TODO: Need a way to create cross-platform perms with more granularity - // builder.permissions() + builder.permissions(perms); if let Ok(f) = builder.tempfile() { // Check if we have perms to set perms #[cfg(unix)] { use std::os::{fd::AsFd, unix::fs::MetadataExt}; - let from_meta = tokio::fs::metadata(&p).await?; let to_meta = tokio::fs::metadata(&f.path()).await?; let _ = fchown( f.as_file().as_fd(), @@ -205,12 +204,14 @@ impl Backup { // - directory is not writable // - path is a directory // - path exists - let mut dir_exists = false; let escaped_p = helix_stdx::path::escape_path(&p); + // `.join` on absolute path replaces instead of append + debug_assert!(escaped_p.is_relative()); + 'outer: for dir in config.directories.iter().filter(|p| p.is_dir()) { let ext = config.extension.as_str(); let bck_base_path = &dir.join(&escaped_p); - let mut backup = bck_base_path.join(&ext); + let mut backup = bck_base_path.with_extension(&ext); // NOTE: Should we just overwrite regardless? // If the backup file already exists, we'll try to add a number before the extension @@ -218,10 +219,11 @@ impl Backup { // NOTE: u8 since if we need more than 256, there might be an issue let mut n: u8 = 1; while backup.exists() { - backup = bck_base_path.join(n.to_string()).join(&ext); - let Some(n) = n.checked_add(1) else { + backup = bck_base_path.with_extension(format!("{n}.{ext}")); + if n == u8::MAX { continue 'outer; - }; + } + n = n + 1; } if copy { @@ -233,7 +235,7 @@ impl Backup { { use std::os::unix::fs::{MetadataExt, PermissionsExt}; - let mut from_meta = tokio::fs::metadata(&p).await?; + let from_meta = tokio::fs::metadata(&p).await?; let mut perms = from_meta.permissions(); // Strip s-bit @@ -249,7 +251,11 @@ impl Backup { perms.set_mode(new_perms); } std::fs::set_permissions(&backup, perms)?; - // TODO: Set time + + let atime = FileTime::from_last_access_time(&from_meta); + let mtime = FileTime::from_last_modification_time(&from_meta); + filetime::set_file_times(&backup, atime, mtime)?; + copy_xattr(&p, &backup)?; } @@ -260,25 +266,15 @@ impl Backup { copy_ownership(&p, &backup_)?; Ok(()) }) - .await?; + .await??; } - - return Ok(Self { - copy: true, - path: backup, - }); } else { tokio::fs::rename(p, &backup).await?; - return Ok(Self { - copy: false, - path: backup, - }); } + return Ok(Self { copy, path: backup }); } - // TODO: Try to initialize last dir if none of the dirs exist - // TODO - bail!("err"); + bail!("Could not write into a backup directory"); } } @@ -1070,19 +1066,8 @@ impl Document { } } } - let write_path = tokio::fs::read_link(&path) - .await - .ok() - .and_then(|p| { - if p.is_relative() { - path.parent().map(|parent| parent.join(p)) - } else { - Some(p) - } - }) - .unwrap_or_else(|| path.clone()); - if readonly(&write_path) { + if readonly(&path) { bail!(std::io::Error::new( std::io::ErrorKind::PermissionDenied, "Path is read only" @@ -1090,61 +1075,72 @@ impl Document { } // Use a backup file + let meta = tokio::fs::metadata(&path).await?; let mut bck = None; - let write_result = if bck_config.kind != crate::editor::BackupKind::None { - bck = Some(Backup::from(write_path.clone(), &bck_config).await?); - let bck = bck.unwrap(); - - // SECURITY: Ensure that the created file has the same perms as the original file - let dst = if !bck.copy { - let from_meta = tokio::fs::metadata(&write_path).await?; - let mut open_opt = tokio::fs::OpenOptions::new() - .read(true) - .write(true) - .create_new(true); - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mode = from_meta.permissions().mode(); - open_opt.mode(mode); + let write_result: Result<(), Error> = async { + if path.exists() && bck_config.kind != crate::editor::BackupKind::None { + match Backup::from(path.clone(), &bck_config).await { + Ok(b) => bck = Some(b), + Err(_) if force => {} + Err(e) => bail!("Could not create backup: {e}"), } + } - let file = open_opt.open(&bck.path).await?; - let to_meta = file.metadata().await?; + if let Some(ref bck) = bck { + // SECURITY: Ensure that the created file has the same perms as the original file + let mut dst = if !bck.copy { + let mut open_opt = tokio::fs::OpenOptions::new(); + open_opt.read(true).write(true).create_new(true); - #[cfg(unix)] - { - // TODO: set gid/uid via fchown - } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = meta.permissions().mode(); + open_opt.mode(mode); + } - #[cfg(windows)] - { - let from = write_path.clone(); - let to = bck.path.clone(); - tokio::task::spawn_blocking(move || { - helix_stdx::faccess::copy_ownership(&from, &to)?; - Ok(()) - }) - .await?; - } - file + let file = open_opt.open(&path).await?; + + #[cfg(unix)] + { + use std::os::fd::AsFd; + use std::os::unix::fs::MetadataExt; + helix_stdx::faccess::fchown( + file.as_fd(), + Some(meta.uid()), + Some(meta.gid()), + )?; + } + + #[cfg(windows)] + { + let from = path.clone(); + let to = bck.path.clone(); + tokio::task::spawn_blocking(move || -> Result<(), Error> { + helix_stdx::faccess::copy_ownership(&from, &to)?; + Ok(()) + }) + .await??; + } + file + } else { + // SECURITY: Backup copy already exists + tokio::fs::File::create(&path).await? + }; + + to_writer(&mut dst, encoding_with_bom_info, &text).await?; + dst.sync_all().await?; + Ok(()) } else { - // SECURITY: Backup copy already exists - tokio::fs::File::create(&write_path).await? - }; + let mut dst = tokio::fs::File::create(&path).await?; + to_writer(&mut dst, encoding_with_bom_info, &text).await?; + dst.sync_all().await?; + Ok(()) + } + } + .await; - to_writer(&mut dst, encoding_with_bom_info, &text).await?; - dst.sync_all().await?; - Ok(()) - } else { - let dst = tokio::fs::File::create(&write_path).await?; - to_writer(&mut dst, encoding_with_bom_info, &text).await?; - dst.sync_all().await?; - Ok(()) - }; - - let save_time = match fs::metadata(&write_path).await { + let save_time = match fs::metadata(&path).await { Ok(metadata) => metadata.modified().map_or(SystemTime::now(), |mtime| mtime), Err(_) => SystemTime::now(), }; @@ -1158,35 +1154,41 @@ impl Document { let mut delete_bck = true; if write_result.is_err() { // If original file no longer exists, then backup is renamed to original file - if !write_path.exists() { - if !tokio::fs::rename(&bck.path, &write_path) + if !path.exists() { + delete_bck = false; + if tokio::fs::rename(&bck.path, &path) .await .map_err(|e| { - delete_bck = false; log::error!("Failed to restore backup on write failure: {e}") }) - .is_err() + .is_ok() { - // TODO: Set timestamps to prior to write + // Reset timestamps + let atime = FileTime::from_last_access_time(&meta); + let mtime = FileTime::from_last_modification_time(&meta); + filetime::set_file_times(&path, atime, mtime)?; } } else { if bck.copy { // Restore backup from copy - let _ = tokio::fs::copy(&bck.path, &write_path).await.map_err(|e| { + let _ = tokio::fs::copy(&bck.path, &path).await.map_err(|e| { delete_bck = false; log::error!("Failed to restore backup on write failure: {e}") }); } else { // restore backup - let _ = tokio::fs::rename(&bck.path, &write_path) - .await - .map_err(|e| { - delete_bck = false; - log::error!("Failed to restore backup on write failure: {e}") - }); + let _ = tokio::fs::rename(&bck.path, &path).await.map_err(|e| { + delete_bck = false; + log::error!("Failed to restore backup on write failure: {e}") + }); } } } + + // Delete backup if we're done with it + if delete_bck { + tokio::fs::remove_file(bck.path).await?; + } } write_result?; diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 25c06b21..57dd8c35 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -362,7 +362,9 @@ pub struct Config { #[serde(rename_all = "kebab-case", default)] pub struct BackupConfig { pub kind: BackupKind, + #[serde(deserialize_with = "deserialize_non_empty_vec")] pub directories: Vec, + #[serde(deserialize_with = "deserialize_non_empty_str")] pub extension: String, } @@ -370,14 +372,39 @@ impl Default for BackupConfig { fn default() -> Self { Self { kind: BackupKind::Auto, - // TODO: Prevent empty vector directories: vec![helix_loader::state_dir().join("backup")], - // TODO: Prevent empty strings extension: String::from("bck"), } } } +fn deserialize_non_empty_vec<'de, D, T>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, + T: Deserialize<'de>, +{ + use serde::de::Error; + + let vec = Vec::::deserialize(deserializer)?; + if vec.is_empty() { + return Err(::custom("vector cannot be empty!")); + } + Ok(vec) +} + +pub fn deserialize_non_empty_str<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + use serde::de::Error; + + let s = String::deserialize(deserializer)?; + if s.is_empty() { + return Err(::custom("string cannot be empty")); + } + Ok(s) +} + #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, PartialOrd, Ord)] #[serde(rename_all = "kebab-case")] pub enum BackupKind { From 9a40080cdcb5e93261013ff11abfe0a93182abd9 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Sun, 17 Nov 2024 14:03:13 -0500 Subject: [PATCH 4/9] fix symlink detection & extension naming --- helix-stdx/src/path.rs | 22 ++++++++++++--- helix-stdx/tests/path.rs | 9 +++--- helix-view/src/document.rs | 56 ++++++++++++++++++++++---------------- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/helix-stdx/src/path.rs b/helix-stdx/src/path.rs index e68d2a19..e0e3e52d 100644 --- a/helix-stdx/src/path.rs +++ b/helix-stdx/src/path.rs @@ -308,12 +308,12 @@ fn os_str_as_bytes>(path: P) -> Vec { fn path_from_bytes(slice: &[u8]) -> Result { #[cfg(windows)] - return Ok(PathBuf::from(std::str::from_utf8(slice)?)); + let res = PathBuf::from(std::str::from_utf8(slice)?); #[cfg(unix)] - return Ok(PathBuf::from( - ::from_bytes(slice), - )); + let res = PathBuf::from(::from_bytes(slice)); + + Ok(res) } fn is_sep_byte(b: u8) -> bool { @@ -336,6 +336,20 @@ pub fn escape_path(path: &Path) -> PathBuf { path_from_bytes(&bytes).unwrap() } +pub fn add_extension<'a, S: AsRef>(p: &'a Path, extension: S) -> Cow<'a, Path> { + let new = extension.as_ref(); + if new.is_empty() { + Cow::Borrowed(p) + } else { + let Some(mut ext) = p.extension().map(std::ffi::OsStr::to_owned) else { + return Cow::Borrowed(p); + }; + ext.push("."); + ext.push(new); + Cow::Owned(p.with_extension(ext)) + } +} + #[cfg(test)] mod tests { use std::{ diff --git a/helix-stdx/tests/path.rs b/helix-stdx/tests/path.rs index cc3c15cb..0bf66370 100644 --- a/helix-stdx/tests/path.rs +++ b/helix-stdx/tests/path.rs @@ -1,10 +1,9 @@ #![cfg(windows)] -use std::{ - env::set_current_dir, - error::Error, - path::{Component, Path, PathBuf}, -}; +use std::{env::set_current_dir, error::Error, path::Component}; + +#[cfg(unix)] +use std::path::{Path, PathBuf}; use helix_stdx::path; use tempfile::Builder; diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 2b9d2235..51ddc8ba 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -159,7 +159,7 @@ impl Backup { let p_ = p.clone(); tokio::task::spawn_blocking(move || hardlink_count(&p_).unwrap_or(2)).await? > 1 }; - let is_symlink = tokio::fs::metadata(&p).await?.is_symlink(); + let is_symlink = tokio::fs::symlink_metadata(&p).await?.is_symlink(); if is_hardlink || is_symlink { copy = true; } else { @@ -211,7 +211,9 @@ impl Backup { 'outer: for dir in config.directories.iter().filter(|p| p.is_dir()) { let ext = config.extension.as_str(); let bck_base_path = &dir.join(&escaped_p); - let mut backup = bck_base_path.with_extension(&ext); + + // NOTE: `escaped_p` will make dot files appear to be extensions, so we need to append + let mut backup = helix_stdx::path::add_extension(&bck_base_path, ext).into_owned(); // NOTE: Should we just overwrite regardless? // If the backup file already exists, we'll try to add a number before the extension @@ -219,11 +221,12 @@ impl Backup { // NOTE: u8 since if we need more than 256, there might be an issue let mut n: u8 = 1; while backup.exists() { - backup = bck_base_path.with_extension(format!("{n}.{ext}")); + backup = helix_stdx::path::add_extension(&bck_base_path, format!("{n}.{ext}")) + .into_owned(); if n == u8::MAX { continue 'outer; } - n = n + 1; + n += 1; } if copy { @@ -231,11 +234,11 @@ impl Backup { // TODO: How handle error? tokio::fs::copy(&p, &backup).await?; + let from_meta = tokio::fs::metadata(&p).await?; #[cfg(unix)] { use std::os::unix::fs::{MetadataExt, PermissionsExt}; - let from_meta = tokio::fs::metadata(&p).await?; let mut perms = from_meta.permissions(); // Strip s-bit @@ -252,12 +255,11 @@ impl Backup { } std::fs::set_permissions(&backup, perms)?; - let atime = FileTime::from_last_access_time(&from_meta); - let mtime = FileTime::from_last_modification_time(&from_meta); - filetime::set_file_times(&backup, atime, mtime)?; - copy_xattr(&p, &backup)?; } + let atime = FileTime::from_last_access_time(&from_meta); + let mtime = FileTime::from_last_modification_time(&from_meta); + filetime::set_file_times(&backup, atime, mtime)?; #[cfg(windows)] { @@ -1075,7 +1077,11 @@ impl Document { } // Use a backup file - let meta = tokio::fs::metadata(&path).await?; + let meta = if path.exists() { + Some(tokio::fs::metadata(&path).await?) + } else { + None + }; let mut bck = None; let write_result: Result<(), Error> = async { if path.exists() && bck_config.kind != crate::editor::BackupKind::None { @@ -1089,6 +1095,9 @@ impl Document { if let Some(ref bck) = bck { // SECURITY: Ensure that the created file has the same perms as the original file let mut dst = if !bck.copy { + #[cfg(unix)] + let meta = meta.as_ref().unwrap(); + let mut open_opt = tokio::fs::OpenOptions::new(); open_opt.read(true).write(true).create_new(true); @@ -1164,30 +1173,29 @@ impl Document { .is_ok() { // Reset timestamps + let meta = meta.as_ref().unwrap(); let atime = FileTime::from_last_access_time(&meta); let mtime = FileTime::from_last_modification_time(&meta); filetime::set_file_times(&path, atime, mtime)?; } + } else if bck.copy { + // Restore backup from copy + let _ = tokio::fs::copy(&bck.path, &path).await.map_err(|e| { + delete_bck = false; + log::error!("Failed to restore backup on write failure: {e}") + }); } else { - if bck.copy { - // Restore backup from copy - let _ = tokio::fs::copy(&bck.path, &path).await.map_err(|e| { - delete_bck = false; - log::error!("Failed to restore backup on write failure: {e}") - }); - } else { - // restore backup - let _ = tokio::fs::rename(&bck.path, &path).await.map_err(|e| { - delete_bck = false; - log::error!("Failed to restore backup on write failure: {e}") - }); - } + delete_bck = false; + // restore backup + let _ = tokio::fs::rename(&bck.path, &path).await.map_err(|e| { + log::error!("Failed to restore backup on write failure: {e}") + }); } } // Delete backup if we're done with it if delete_bck { - tokio::fs::remove_file(bck.path).await?; + let _ = tokio::fs::remove_file(bck.path).await; } } From 3587934b49df4692fbdd5f9c172f6718ab644a5f Mon Sep 17 00:00:00 2001 From: Mason Mac <67773714+kirawi@users.noreply.github.com> Date: Mon, 18 Nov 2024 13:26:15 -0500 Subject: [PATCH 5/9] add test to check if ownership stays the same --- helix-stdx/src/path.rs | 2 +- helix-term/tests/test/commands/write.rs | 19 +++++++++++++++++++ helix-view/src/document.rs | 15 ++++++--------- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/helix-stdx/src/path.rs b/helix-stdx/src/path.rs index e0e3e52d..d32c0f59 100644 --- a/helix-stdx/src/path.rs +++ b/helix-stdx/src/path.rs @@ -336,7 +336,7 @@ pub fn escape_path(path: &Path) -> PathBuf { path_from_bytes(&bytes).unwrap() } -pub fn add_extension<'a, S: AsRef>(p: &'a Path, extension: S) -> Cow<'a, Path> { +pub fn add_extension>(p: &Path, extension: S) -> Cow<'_, Path> { let new = extension.as_ref(); if new.is_empty() { Cow::Borrowed(p) diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index 336ab2ab..8c04bd25 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -687,6 +687,25 @@ async fn test_hardlink_write() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +#[cfg(unix)] +async fn test_write_ownership() -> anyhow::Result<()> { + let mut file = tempfile::NamedTempFile::new()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; + + let old_meta = file.as_file().metadata()?; + + test_key_sequence(&mut app, Some("hello:w"), None, false).await?; + reload_file(&mut file).unwrap(); + + let new_meta = file.as_file().metadata()?; + assert!(old_meta.uid() == new_meta.uid() && old_meta.gid() == new_meta.gid()); + + Ok(()) +} + async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 51ddc8ba..054d5ce7 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -213,7 +213,7 @@ impl Backup { let bck_base_path = &dir.join(&escaped_p); // NOTE: `escaped_p` will make dot files appear to be extensions, so we need to append - let mut backup = helix_stdx::path::add_extension(&bck_base_path, ext).into_owned(); + let mut backup = helix_stdx::path::add_extension(bck_base_path, ext).into_owned(); // NOTE: Should we just overwrite regardless? // If the backup file already exists, we'll try to add a number before the extension @@ -221,7 +221,7 @@ impl Backup { // NOTE: u8 since if we need more than 256, there might be an issue let mut n: u8 = 1; while backup.exists() { - backup = helix_stdx::path::add_extension(&bck_base_path, format!("{n}.{ext}")) + backup = helix_stdx::path::add_extension(bck_base_path, format!("{n}.{ext}")) .into_owned(); if n == u8::MAX { continue 'outer; @@ -1155,12 +1155,9 @@ impl Document { }; if let Some(bck) = bck { - /* - - If original file no longer exists, then backup is renamed to original file - - And the timestamp is preserved by setting timestmaps to prior to write - - Then backup is deleted - */ let mut delete_bck = true; + + // Attempt to restore backup if write_result.is_err() { // If original file no longer exists, then backup is renamed to original file if !path.exists() { @@ -1174,8 +1171,8 @@ impl Document { { // Reset timestamps let meta = meta.as_ref().unwrap(); - let atime = FileTime::from_last_access_time(&meta); - let mtime = FileTime::from_last_modification_time(&meta); + let atime = FileTime::from_last_access_time(meta); + let mtime = FileTime::from_last_modification_time(meta); filetime::set_file_times(&path, atime, mtime)?; } } else if bck.copy { From 6fa1f185fbbefbb3757b84fcafdaa35cce4f5adf Mon Sep 17 00:00:00 2001 From: Mason Mac <67773714+kirawi@users.noreply.github.com> Date: Mon, 18 Nov 2024 13:46:03 -0500 Subject: [PATCH 6/9] use fchown for ownership test --- helix-term/tests/test/commands/write.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index 8c04bd25..b9b9c27b 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -690,11 +690,18 @@ async fn test_hardlink_write() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] #[cfg(unix)] async fn test_write_ownership() -> anyhow::Result<()> { + use std::os::unix::fs::MetadataExt; + let mut file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .build()?; + let nobody_uid = 65534; + let nogroup_gid = 65534; + + helix_stdx::faccess::fchown(&file.as_file_mut(), Some(nobody_uid), Some(nogroup_gid))?; + let old_meta = file.as_file().metadata()?; test_key_sequence(&mut app, Some("hello:w"), None, false).await?; From 9ab43f345d8af9eb43b3e5e6f99e1a636ef58dbd Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Mon, 18 Nov 2024 16:36:23 -0500 Subject: [PATCH 7/9] remove ownership test --- helix-term/tests/test/commands/write.rs | 26 ------------------------- 1 file changed, 26 deletions(-) diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index b9b9c27b..336ab2ab 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -687,32 +687,6 @@ async fn test_hardlink_write() -> anyhow::Result<()> { Ok(()) } -#[tokio::test(flavor = "multi_thread")] -#[cfg(unix)] -async fn test_write_ownership() -> anyhow::Result<()> { - use std::os::unix::fs::MetadataExt; - - let mut file = tempfile::NamedTempFile::new()?; - let mut app = helpers::AppBuilder::new() - .with_file(file.path(), None) - .build()?; - - let nobody_uid = 65534; - let nogroup_gid = 65534; - - helix_stdx::faccess::fchown(&file.as_file_mut(), Some(nobody_uid), Some(nogroup_gid))?; - - let old_meta = file.as_file().metadata()?; - - test_key_sequence(&mut app, Some("hello:w"), None, false).await?; - reload_file(&mut file).unwrap(); - - let new_meta = file.as_file().metadata()?; - assert!(old_meta.uid() == new_meta.uid() && old_meta.gid() == new_meta.gid()); - - Ok(()) -} - async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; From b7e4923deb1b636232606c2942fce8890aef6478 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Mon, 30 Dec 2024 23:40:06 -0500 Subject: [PATCH 8/9] fix copy_xattr --- helix-stdx/src/faccess.rs | 68 +++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index ad2ffd60..11083baf 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -504,47 +504,67 @@ pub fn fchown(fd: impl std::os::fd::AsFd, uid: Option, gid: Option) -> } #[cfg(unix)] -pub fn copy_xattr(from: &Path, to: &Path) -> io::Result<()> { - let size = match rustix::fs::listxattr(from, &mut [])? { +pub fn copy_xattr(src: &Path, dst: &Path) -> io::Result<()> { + use std::ffi::CStr; + + if !src.exists() || !dst.exists() { + return Err(io::Error::new( + io::ErrorKind::NotFound, + "src or dst file was not found while copying attributes", + )); + } + + let size = match rustix::fs::listxattr(src, &mut [])? { 0 => return Ok(()), // No attributes len => len, }; - let mut buf = vec![0; size]; - let read = rustix::fs::listxattr(from, buf.as_mut_slice())?; + let mut key_list = vec![0; size]; + let size = rustix::fs::listxattr(src, key_list.as_mut_slice())?; + if key_list.len() != size { + return Err(io::Error::new( + io::ErrorKind::Other, + format!( + "`{}`'s xattr list changed while copying attributes", + src.to_string_lossy() + ), + )); + } // Iterate over null-terminated C-style strings // Two loops to avoid multiple allocations // Find max-size for attributes - let mut max_attr_len = 0; - for attr_byte in buf[..read].split(|&b| b == 0) { - // handle platforms where c_char is i8 + let mut max_val_len = 0; + for key in key_list[..size].split_inclusive(|&b| b == 0) { + // Needed on macos #[allow(clippy::unnecessary_cast)] - let conv = - unsafe { std::slice::from_raw_parts(attr_byte.as_ptr() as *const u8, attr_byte.len()) }; - let name = std::str::from_utf8(conv) - .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?; - let attr_len = rustix::fs::getxattr(from, name, &mut [])?; - max_attr_len = max_attr_len.max(attr_len); + let conv = unsafe { std::slice::from_raw_parts(key.as_ptr() as *const u8, key.len()) }; + let key = CStr::from_bytes_with_nul(conv) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; + + let attr_len = rustix::fs::getxattr(src, key, &mut [])?; + max_val_len = max_val_len.max(attr_len); } - let mut attr_buf = vec![0u8; max_attr_len]; - for attr_byte in buf[..read].split(|&b| b == 0) { - // handle platforms where c_char is i8 + let mut attr_buf = vec![0u8; max_val_len]; + for key in key_list[..size] + .split(|&b| b == 0) + .filter(|v| !v.is_empty()) + { + // Needed on macos #[allow(clippy::unnecessary_cast)] - let conv = - unsafe { std::slice::from_raw_parts(attr_byte.as_ptr() as *const u8, attr_byte.len()) }; - let name = std::str::from_utf8(conv) - .map_err(|_| std::io::Error::from(std::io::ErrorKind::InvalidData))?; - let read = rustix::fs::getxattr(from, name, attr_buf.as_mut_slice())?; + let conv = unsafe { std::slice::from_raw_parts(key.as_ptr() as *const u8, key.len()) }; + let key = CStr::from_bytes_with_nul(conv) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; + let read = rustix::fs::getxattr(src, key, attr_buf.as_mut_slice())?; // If we can't set xattr because it already exists, try to replace it if read != 0 { - match rustix::fs::setxattr(to, name, &attr_buf[..read], rustix::fs::XattrFlags::CREATE) + match rustix::fs::setxattr(dst, key, &attr_buf[..read], rustix::fs::XattrFlags::CREATE) { Err(rustix::io::Errno::EXIST) => rustix::fs::setxattr( - to, - name, + dst, + key, &attr_buf[..read], rustix::fs::XattrFlags::REPLACE, )?, From d75bafac8a39e4edc0449ae13eece47367d39656 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Tue, 31 Dec 2024 00:05:17 -0500 Subject: [PATCH 9/9] try uid test again --- helix-stdx/src/faccess.rs | 5 +---- helix-term/tests/test/commands/write.rs | 30 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index 11083baf..8be9df1b 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -547,10 +547,7 @@ pub fn copy_xattr(src: &Path, dst: &Path) -> io::Result<()> { } let mut attr_buf = vec![0u8; max_val_len]; - for key in key_list[..size] - .split(|&b| b == 0) - .filter(|v| !v.is_empty()) - { + for key in key_list[..size].split_inclusive(|&b| b == 0) { // Needed on macos #[allow(clippy::unnecessary_cast)] let conv = unsafe { std::slice::from_raw_parts(key.as_ptr() as *const u8, key.len()) }; diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index 336ab2ab..0be0298e 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -687,6 +687,36 @@ async fn test_hardlink_write() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +#[cfg(unix)] +async fn test_write_ownership() -> anyhow::Result<()> { + // GH CI does not possess CAP_CHOWN + if option_env!("GITHUB_ACTIONS").is_some() { + return Ok(()); + } + use std::os::unix::fs::MetadataExt; + + let mut file = tempfile::NamedTempFile::new()?; + let mut app = helpers::AppBuilder::new() + .with_file(file.path(), None) + .build()?; + + let nobody_uid = 9999; + let nogroup_gid = 9999; + + helix_stdx::faccess::fchown(&file.as_file_mut(), Some(nobody_uid), Some(nogroup_gid))?; + + let old_meta = file.as_file().metadata()?; + + test_key_sequence(&mut app, Some("hello:w"), None, false).await?; + reload_file(&mut file).unwrap(); + + let new_meta = file.as_file().metadata()?; + assert!(old_meta.uid() == new_meta.uid() && old_meta.gid() == new_meta.gid()); + + Ok(()) +} + async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?;