diff --git a/Cargo.lock b/Cargo.lock index e2ff70e61efc..ee1c817a6f42 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1643,6 +1643,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 b98ae153ee10..a5845718b7ed 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -26,7 +26,7 @@ mod imp { use super::*; use rustix::fs::Access; - use std::os::unix::fs::{MetadataExt, PermissionsExt}; + use std::os::unix::fs::MetadataExt; pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { let mut imode = Access::empty(); @@ -51,44 +51,101 @@ 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(rustix::fs::Uid::from_raw); let gid = gid.map(rustix::fs::Gid::from_raw); rustix::fs::chown(p, 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)?; - let from_gid = from_meta.gid(); - let to_gid = to_meta.gid(); - - let mut perms = from_meta.permissions(); - perms.set_mode(perms.mode() & 0o0777); - 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); + pub fn fchown( + fd: impl std::os::fd::AsFd, + uid: Option, + gid: Option, + ) -> io::Result<()> { + let uid = uid.map(rustix::fs::Uid::from_raw); + let gid = gid.map(rustix::fs::Gid::from_raw); + rustix::fs::fchown(fd, uid, gid)?; + Ok(()) + } + + pub fn hardlink_count(p: &Path) -> std::io::Result { + let metadata = p.metadata()?; + Ok(metadata.nlink()) + } + + 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", + )); } - #[cfg(target_os = "macos")] - { - use std::fs::{File, FileTimes}; - use std::os::macos::fs::FileTimesExt; + let size = match rustix::fs::listxattr::<_, &mut [u8]>(src, &mut [])? { + 0 => return Ok(()), // No attributes + len => len, + }; - let to_file = File::options().write(true).open(to)?; - let times = FileTimes::new().set_created(from_meta.created()?); - to_file.set_times(times)?; + 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() + ), + )); } - std::fs::set_permissions(to, perms)?; + // Iterate over null-terminated C-style strings + // Two loops to avoid multiple allocations + // Find max-size for attributes + 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(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::<_, _, &mut [u8]>(src, key, &mut [])?; + max_val_len = max_val_len.max(attr_len); + } - Ok(()) - } + let mut attr_buf = vec![0u8; max_val_len]; + 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()) }; + 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( + dst, + key, + &attr_buf[..read], + rustix::fs::XattrFlags::CREATE, + ) { + Err(rustix::io::Errno::EXIST) => rustix::fs::setxattr( + dst, + key, + &attr_buf[..read], + rustix::fs::XattrFlags::REPLACE, + )?, + Err(e) => return Err(e.into()), + _ => {} + } + } + } - pub fn hardlink_count(p: &Path) -> std::io::Result { - let metadata = p.metadata()?; - Ok(metadata.nlink()) + Ok(()) } } @@ -96,9 +153,11 @@ mod imp { #[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, @@ -111,7 +170,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}; @@ -235,9 +295,13 @@ mod imp { return Err(io::Error::last_os_error()); } - let token: *mut HANDLE = std::ptr::null_mut(); - let err = - OpenThreadToken(GetCurrentThread(), TOKEN_DUPLICATE | TOKEN_QUERY, 0, token); + let mut token: HANDLE = std::ptr::null_mut(); + let err = OpenThreadToken( + GetCurrentThread(), + TOKEN_DUPLICATE | TOKEN_QUERY, + 0, + &mut token, + ); RevertToSelf(); @@ -245,7 +309,7 @@ mod imp { return Err(io::Error::last_os_error()); } - Ok(Self(*token)) + Ok(Self(token)) } } @@ -373,13 +437,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(); @@ -404,8 +463,8 @@ mod imp { } let err = unsafe { - SetNamedSecurityInfoW( - pathw.as_ptr(), + SetSecurityInfo( + handle, SE_FILE_OBJECT, si, owner, @@ -422,9 +481,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(); @@ -484,6 +552,26 @@ mod imp { } } +#[cfg(unix)] +pub fn chown(p: &Path, uid: Option, gid: Option) -> io::Result<()> { + imp::chown(p, uid, gid) +} + +#[cfg(unix)] +pub fn fchown(fd: impl std::os::fd::AsFd, uid: Option, gid: Option) -> io::Result<()> { + imp::fchown(fd, uid, gid) +} + +#[cfg(unix)] +pub fn copy_xattr(src: &Path, dst: &Path) -> io::Result<()> { + imp::copy_xattr(src, dst) +} + +#[cfg(windows)] +pub fn copy_ownership(from: &Path, to: &Path) -> io::Result<()> { + imp::copy_ownership(from, to) +} + pub fn readonly(p: &Path) -> bool { match imp::access(p, AccessMode::WRITE) { Ok(_) => false, @@ -492,6 +580,7 @@ pub fn readonly(p: &Path) -> bool { } } +#[cfg(windows)] pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { imp::copy_metadata(from, to) } diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index 4f44cd368ff8..b606451f31f4 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -933,3 +933,33 @@ async fn test_move_file_when_given_dir_only() -> 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(()) +} diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 24dd0f2aaab2..fc263b0479b9 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -54,6 +54,8 @@ thiserror.workspace = true kstring = "2.0" +filetime = "0.2" + [target.'cfg(windows)'.dependencies] clipboard-win = { version = "5.4", features = ["std"] } crossterm = { version = "0.28", optional = true } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index e52dbe0f956f..1301d241cc90 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::DynAccess; use arc_swap::ArcSwap; +use filetime::FileTime; use futures_util::future::BoxFuture; use futures_util::FutureExt; use helix_core::auto_pairs::AutoPairs; @@ -14,7 +15,7 @@ use helix_core::syntax::config::LanguageServerFeature; use helix_core::text_annotations::{InlineAnnotation, Overlay}; use helix_event::TaskController; 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 once_cell::sync::OnceCell; use thiserror; @@ -138,6 +139,153 @@ pub enum DocumentOpenError { IoError(#[from] io::Error), } +struct Backup { + is_copy: bool, + path: PathBuf, +} + +impl Backup { + async fn from(p: PathBuf) -> Result { + // Just in case + if !p.exists() { + bail!(std::io::Error::new( + std::io::ErrorKind::NotFound, + "File does not exist to create backup file." + )); + } + + // Assume it is a hardlink to prevent data loss if the metadata cant be read (e.g. on certain Windows configurations) + let is_hardlink = { + let p_ = p.clone(); + tokio::task::spawn_blocking(move || { + helix_stdx::faccess::hardlink_count(&p_).unwrap_or(2) + }) + .await? + > 1 + }; + let is_symlink = tokio::fs::symlink_metadata(&p).await?.is_symlink(); + + // We must manually copy the file into a backup because `move` would destroy links. + let mut is_copy = is_hardlink || is_symlink; + + // We are probing the process's permissions here before actually creating a backup. + let from_meta = tokio::fs::metadata(&p).await?; + let perms = from_meta.permissions(); + if !is_copy { + let mut builder = tempfile::Builder::new(); + builder.permissions(perms.clone()); + + if let Ok(file) = builder.tempfile() { + // Check if we have perms to set perms + #[cfg(unix)] + { + use std::os::{fd::AsFd, unix::fs::MetadataExt}; + + let to_meta = tokio::fs::metadata(&file.path()).await?; + let _ = helix_stdx::faccess::fchown( + file.as_file().as_fd(), + Some(from_meta.uid()), + Some(from_meta.gid()), + ); + + // Check if file perms were updated. If not, we assume the process does not have permissions to modify perms of this file and must instead copy. + if from_meta.uid() != to_meta.uid() + || from_meta.gid() != to_meta.gid() + || from_meta.permissions() != to_meta.permissions() + { + is_copy = true; + } + } + + #[cfg(not(unix))] + if helix_stdx::faccess::copy_metadata(&p, file.path()).is_err() { + is_copy = true; + } + } + } + + // Create the backup + let path_ = p.clone(); + let backup = tokio::task::spawn_blocking(move || -> Option { + let mut builder = tempfile::Builder::new(); + builder.prefix(path_.file_name()?).suffix(".bck"); + + let backup_path = if is_copy { + let from_meta = std::fs::metadata(&path_).ok()?; + #[allow(unused_variables)] // Needed on Windows + let (backup, backup_path) = builder.tempfile().ok()?.into_parts(); + std::fs::copy(&path_, &backup_path).ok()?; + + #[cfg(unix)] + { + use std::os::{ + fd::AsFd, + unix::fs::{MetadataExt, PermissionsExt}, + }; + + let to_meta = std::fs::metadata(&backup_path).ok()?; + let from_gid = from_meta.gid(); + let to_gid = to_meta.gid(); + + let mut perms = from_meta.permissions(); + perms.set_mode(perms.mode() & 0o0777); // Strip s-bit + + // If chown fails, se the protection bits for the roup the same as the perm bits for others + if from_gid != to_gid + && helix_stdx::faccess::fchown(backup.as_fd(), None, Some(from_gid)) + .is_err() + { + let new_perms = (perms.mode() & 0o0707) | ((perms.mode() & 0o07) << 3); + perms.set_mode(new_perms); + } + + #[cfg(target_os = "macos")] + { + use std::fs::{File, FileTimes}; + use std::os::macos::fs::FileTimesExt; + + let to_file = File::options().write(true).open(&backup_path).ok()?; + let times = FileTimes::new().set_created(from_meta.created().ok()?); + to_file.set_times(times).ok()?; + } + + std::fs::set_permissions(&backup_path, perms).ok()?; + helix_stdx::faccess::copy_xattr(&path_, &backup_path).ok()?; + } + + #[cfg(windows)] + { + let backup_p_ = backup_path.to_path_buf(); + helix_stdx::faccess::copy_metadata(&p, &backup_p_).ok()?; + } + + let atime = FileTime::from_last_access_time(&from_meta); + let mtime = FileTime::from_last_modification_time(&from_meta); + filetime::set_file_times(&backup_path, atime, mtime).ok()?; + + backup_path + } else { + builder + .make_in(path_.parent()?, |backup| std::fs::rename(&path_, backup)) + .ok()? + .into_temp_path() + }; + + backup_path.keep().ok() + }) + .await?; + + if let Some(backup) = backup { + Ok(Backup { + is_copy, + path: backup, + }) + } else { + bail!("Could not create backup"); + } + } +} + pub struct Document { pub(crate) id: DocumentId, text: Rope, @@ -1014,17 +1162,7 @@ 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()); + let write_path = path.clone(); if readonly(&write_path) { bail!(std::io::Error::new( @@ -1033,44 +1171,97 @@ 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() && atomic_save { - 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"); - - 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() - }; + // Use a backup file + let meta = if path.exists() { + Some(tokio::fs::metadata(&path).await?) + } else { + None + }; - backup_path.keep().ok() - }) - .await - .ok() - .flatten() + let backup = if path.exists() && atomic_save { + let res = Backup::from(write_path.clone()).await; + match res { + Ok(bck) => Some(bck), + Err(e) => { + log::error!("Failed to create backup file: {}", e); + None + } + } } else { None }; 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?; + if let Some(backup) = backup.as_ref() { + let mut dst = if !backup.is_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); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = meta.permissions().mode(); + open_opt.mode(mode); + } + + 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()), + )?; + + // RECALL: File at `path` is newly created, and file at `backup.path` is the original. + helix_stdx::faccess::copy_xattr(&backup.path, &path)?; + } + + #[cfg(target_os = "macos")] + { + use std::fs::{File, FileTimes}; + use std::os::macos::fs::FileTimesExt; + + let file = file.try_clone().await?.into_std().await; + let times = FileTimes::new().set_created(meta.created()?); + file.set_times(times)?; + } + + #[cfg(windows)] + { + let from = path.clone(); + let to = backup.path.clone(); + tokio::task::spawn_blocking(move || -> Result<(), Error> { + helix_stdx::faccess::copy_metadata(&from, &to)?; + Ok(()) + }) + .await??; + } + + file + } else { + // INTEGRITY: Backup copy already exists. But possible TOCTOU race w/ file being changed underneath + let mut open_opt = tokio::fs::File::options(); + open_opt + .create(false) + .write(true) + .truncate(true) + .open(&path) + .await? + }; + to_writer(&mut dst, encoding_with_bom_info, &text).await?; + dst.sync_all().await?; + } else { + 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; @@ -1081,36 +1272,42 @@ impl Document { }; if let Some(backup) = backup { - if is_hardlink { + if backup.is_copy { 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}") - }); + let _ = tokio::fs::copy(&backup.path, &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) + let _ = tokio::fs::remove_file(backup.path) .await .map_err(|e| log::error!("Failed to remove backup file on write: {e}")); } } else if write_result.is_err() { // restore backup - let _ = tokio::fs::rename(&backup, &write_path) + if tokio::fs::rename(&backup.path, &write_path) .await - .map_err(|e| log::error!("Failed to restore backup on write failure: {e}")); + .map_err(|e| log::error!("Failed to restore backup on write failure: {e}")) + .is_ok() + { + // Essentially no modification to the content had been done. + 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 { - // 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; + // We're done; delete backup + let _ = tokio::fs::remove_file(backup.path) + .await + .map_err(|e| log::error!("Failed to remove backup file on write: {e}")); } }