Fix write issues

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,54 +51,113 @@ mod imp {
         Ok(())
     }
 
-    fn chown(p: &Path, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> {
+    pub fn chown(p: &Path, uid: Option<u32>, gid: Option<u32>) -> 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<u32>,
+        gid: Option<u32>,
+    ) -> 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<u64> {
+        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<u64> {
-        let metadata = p.metadata()?;
-        Ok(metadata.nlink())
+        Ok(())
     }
 }
 
 // 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,
@@ -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,17 +295,21 @@ 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();
 
                 if err == 0 {
                     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<u32>, gid: Option<u32>) -> io::Result<()> {
+    imp::chown(p, uid, gid)
+}
+
+#[cfg(unix)]
+pub fn fchown(fd: impl std::os::fd::AsFd, uid: Option<u32>, gid: Option<u32>) -> 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)
 }

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<ret>"), 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(())
+}

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 }