From 80551a6dfd68c33fa74fc265a97e15bd6d14cc6e Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Thu, 4 Jan 2024 14:14:57 -0500 Subject: [PATCH 01/12] use temporary files for writes --- Cargo.lock | 1 + helix-term/tests/test/commands/write.rs | 90 ++++++++++++------------- helix-term/tests/test/helpers.rs | 8 ++- helix-view/Cargo.toml | 2 + helix-view/src/document.rs | 38 ++++++++++- 5 files changed, 89 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7b73b1b1acb3..b3009a925b2a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1452,6 +1452,7 @@ dependencies = [ "serde", "serde_json", "slotmap", + "tempfile", "tokio", "tokio-stream", "toml", diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index f65352c7e68d..39f4a8ed5d85 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -66,7 +66,7 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> { .await?; // verify if writes are queued up, it finishes them before closing the buffer - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut command = String::new(); const RANGE: RangeInclusive = 1..=1000; @@ -93,15 +93,16 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> { false, ) .await?; + let (mut file, _) = reload_file(file); - helpers::assert_file_has_content(file.as_file_mut(), &platform_line(&RANGE.end().to_string()))?; + helpers::assert_file_has_content(&mut file, &platform_line(&RANGE.end().to_string()))?; Ok(()) } #[tokio::test(flavor = "multi_thread")] async fn test_write() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .build()?; @@ -114,11 +115,9 @@ async fn test_write() -> anyhow::Result<()> { ) .await?; - file.as_file_mut().flush()?; - file.as_file_mut().sync_all()?; - + let (mut file, _) = reload_file(file); let mut file_content = String::new(); - file.as_file_mut().read_to_string(&mut file_content)?; + file.read_to_string(&mut file_content)?; assert_eq!( helpers::platform_line("the gostak distims the doshes"), @@ -145,12 +144,9 @@ async fn test_overwrite_protection() -> anyhow::Result<()> { test_key_sequence(&mut app, Some(":x"), None, false).await?; - file.as_file_mut().flush()?; - file.as_file_mut().sync_all()?; - - file.rewind()?; + let (mut file, _) = reload_file(file); let mut file_content = String::new(); - file.as_file_mut().read_to_string(&mut file_content)?; + file.read_to_string(&mut file_content)?; assert_eq!( helpers::platform_line("extremely important content"), @@ -162,7 +158,7 @@ async fn test_overwrite_protection() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_quit() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .build()?; @@ -175,15 +171,14 @@ async fn test_write_quit() -> anyhow::Result<()> { ) .await?; - file.as_file_mut().flush()?; - file.as_file_mut().sync_all()?; + let (mut file, _) = reload_file(file); let mut file_content = String::new(); - file.as_file_mut().read_to_string(&mut file_content)?; + file.read_to_string(&mut file_content)?; assert_eq!( helpers::platform_line("the gostak distims the doshes"), - file_content + file_content, ); Ok(()) @@ -191,7 +186,7 @@ async fn test_write_quit() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_concurrent() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut command = String::new(); const RANGE: RangeInclusive = 1..=1000; let mut app = helpers::AppBuilder::new() @@ -205,11 +200,9 @@ async fn test_write_concurrent() -> anyhow::Result<()> { test_key_sequence(&mut app, Some(&command), None, false).await?; - file.as_file_mut().flush()?; - file.as_file_mut().sync_all()?; - + let (mut file, _) = reload_file(file); let mut file_content = String::new(); - file.as_file_mut().read_to_string(&mut file_content)?; + file.read_to_string(&mut file_content)?; assert_eq!(platform_line(&RANGE.end().to_string()), file_content); Ok(()) @@ -258,7 +251,7 @@ async fn test_write_fail_mod_flag() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_scratch_to_new_path() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; test_key_sequence( &mut AppBuilder::new().build()?, @@ -276,7 +269,8 @@ async fn test_write_scratch_to_new_path() -> anyhow::Result<()> { ) .await?; - helpers::assert_file_has_content(file.as_file_mut(), &helpers::platform_line("hello"))?; + let (mut file, _) = reload_file(file); + helpers::assert_file_has_content(&mut file, &helpers::platform_line("hello"))?; Ok(()) } @@ -304,7 +298,7 @@ async fn test_write_scratch_no_path_fails() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> { - let mut file = tempfile::Builder::new().suffix(".rs").tempfile()?; + let file = tempfile::Builder::new().suffix(".rs").tempfile()?; let lang_conf = indoc! {r#" [[language]] @@ -320,16 +314,17 @@ async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> { test_key_sequences(&mut app, vec![(Some(":w"), None)], false).await?; + let (mut file, _) = reload_file(file); // file still saves - helpers::assert_file_has_content(file.as_file_mut(), "let foo = 0;\n")?; + helpers::assert_file_has_content(&mut file, "let foo = 0;\n")?; Ok(()) } #[tokio::test(flavor = "multi_thread")] async fn test_write_new_path() -> anyhow::Result<()> { - let mut file1 = tempfile::NamedTempFile::new().unwrap(); - let mut file2 = tempfile::NamedTempFile::new().unwrap(); + let file1 = tempfile::NamedTempFile::new().unwrap(); + let file2 = tempfile::NamedTempFile::new().unwrap(); let mut app = helpers::AppBuilder::new() .with_file(file1.path(), None) .build()?; @@ -359,13 +354,15 @@ async fn test_write_new_path() -> anyhow::Result<()> { ) .await?; + let (mut file1, _) = reload_file(file1); + let (mut file2, _) = reload_file(file2); helpers::assert_file_has_content( - file1.as_file_mut(), + &mut file1, &helpers::platform_line("i can eat glass, it will not hurt me\n"), )?; helpers::assert_file_has_content( - file2.as_file_mut(), + &mut file2, &helpers::platform_line("i can eat glass, it will not hurt me\n"), )?; @@ -427,7 +424,7 @@ async fn test_write_utf_bom_file() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .with_input_text("#[h|]#ave you tried chamomile tea?") @@ -435,8 +432,9 @@ async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<() test_key_sequence(&mut app, Some(":w"), None, false).await?; + let (mut file, _) = reload_file(file); helpers::assert_file_has_content( - file.as_file_mut(), + &mut file, &helpers::platform_line("have you tried chamomile tea?\n"), )?; @@ -445,7 +443,7 @@ async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<() #[tokio::test(flavor = "multi_thread")] async fn test_write_insert_final_newline_unchanged_if_not_missing() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .with_input_text(&helpers::platform_line("#[t|]#en minutes, please\n")) @@ -453,17 +451,15 @@ async fn test_write_insert_final_newline_unchanged_if_not_missing() -> anyhow::R test_key_sequence(&mut app, Some(":w"), None, false).await?; - helpers::assert_file_has_content( - file.as_file_mut(), - &helpers::platform_line("ten minutes, please\n"), - )?; + let (mut file, _) = reload_file(file); + helpers::assert_file_has_content(&mut file, &helpers::platform_line("ten minutes, please\n"))?; Ok(()) } #[tokio::test(flavor = "multi_thread")] async fn test_write_insert_final_newline_unchanged_if_missing_and_false() -> anyhow::Result<()> { - let mut file = tempfile::NamedTempFile::new()?; + let file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_config(Config { editor: helix_view::editor::Config { @@ -478,18 +474,16 @@ async fn test_write_insert_final_newline_unchanged_if_missing_and_false() -> any test_key_sequence(&mut app, Some(":w"), None, false).await?; - helpers::assert_file_has_content( - file.as_file_mut(), - "the quiet rain continued through the night", - )?; + let (mut file, _) = reload_file(file); + helpers::assert_file_has_content(&mut file, "the quiet rain continued through the night")?; Ok(()) } #[tokio::test(flavor = "multi_thread")] async fn test_write_all_insert_final_newline_add_if_missing_and_modified() -> anyhow::Result<()> { - let mut file1 = tempfile::NamedTempFile::new()?; - let mut file2 = tempfile::NamedTempFile::new()?; + let file1 = tempfile::NamedTempFile::new()?; + let file2 = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file1.path(), None) .with_input_text("#[w|]#e don't serve time travelers here") @@ -506,13 +500,15 @@ async fn test_write_all_insert_final_newline_add_if_missing_and_modified() -> an ) .await?; + let (mut file1, _) = reload_file(file1); + let (mut file2, _) = reload_file(file2); helpers::assert_file_has_content( - file1.as_file_mut(), + &mut file1, &helpers::platform_line("we don't serve time travelers here\n"), )?; helpers::assert_file_has_content( - file2.as_file_mut(), + &mut file2, &helpers::platform_line("a time traveler walks into a bar\n"), )?; @@ -557,7 +553,7 @@ async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { ) .await?; - file.rewind()?; + let (mut file, _) = reload_file(file); let mut new_file_content: Vec = Vec::new(); file.read_to_end(&mut new_file_content)?; diff --git a/helix-term/tests/test/helpers.rs b/helix-term/tests/test/helpers.rs index a978f386e335..02e63974d9de 100644 --- a/helix-term/tests/test/helpers.rs +++ b/helix-term/tests/test/helpers.rs @@ -11,7 +11,7 @@ use crossterm::event::{Event, KeyEvent}; use helix_core::{diagnostic::Severity, test, Selection, Transaction}; use helix_term::{application::Application, args::Args, config::Config, keymap::merge_keys}; use helix_view::{current_ref, doc, editor::LspConfig, input::parse_macro, Editor}; -use tempfile::NamedTempFile; +use tempfile::{NamedTempFile, TempPath}; use tokio_stream::wrappers::UnboundedReceiverStream; #[derive(Clone, Debug)] @@ -368,3 +368,9 @@ pub fn assert_status_not_error(editor: &Editor) { assert_ne!(&Severity::Error, sev); } } + +pub fn reload_file(t: NamedTempFile) -> (File, TempPath) { + let p = t.into_temp_path(); + let f = std::fs::File::open(&p).unwrap(); + (f, p) +} diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 335779bc2015..f1875ec47edf 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -27,6 +27,8 @@ bitflags = "2.4" anyhow = "1" crossterm = { version = "0.27", optional = true } +tempfile = "3.9" + # Conversion traits once_cell = "1.19" url = "2.5.0" diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 4e7b1de9f0cd..53db318b9d73 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -838,6 +838,23 @@ impl Document { impl Future> + 'static + Send, anyhow::Error, > { + #[cfg(unix)] + async fn chown(path: &Path, other: &Path) -> anyhow::Result<()> { + use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::MetadataExt; + let meta = tokio::fs::metadata(path).await?; + + // From https://github.com/uutils/coreutils/blob/2c73e978ba882c9cd56f0f16fae6f8dcce1c9850/src/uucore/src/lib/features/perms.rs#L46-L61 + let uid = meta.uid(); + let gid = meta.gid(); + let s = std::ffi::CString::new(other.as_os_str().as_bytes())?; + let ret = unsafe { libc::chown(s.as_ptr(), uid, gid) }; + if ret != 0 { + return Err(anyhow::anyhow!(tokio::io::Error::last_os_error())); + } + Ok(()) + } + log::debug!( "submitting save of doc '{:?}'", self.path().map(|path| path.to_string_lossy()) @@ -892,8 +909,25 @@ impl Document { } } - let mut file = File::create(&path).await?; - to_writer(&mut file, encoding_with_bom_info, &text).await?; + match rustix::fs::access(&path, rustix::fs::Access::WRITE_OK) { + Ok(_) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(_) => bail!("file is readonly"), + }; + + let (mut tmp_file, tmp_path) = tokio::task::spawn_blocking( + move || -> anyhow::Result<(File, tempfile::TempPath)> { + let (f, p) = tempfile::NamedTempFile::new()?.into_parts(); + Ok((tokio::fs::File::from_std(f), p)) + }, + ) + .await??; + to_writer(&mut tmp_file, encoding_with_bom_info, &text).await?; + + let perms = tokio::fs::metadata(&path).await?.permissions(); + tmp_file.set_permissions(perms).await?; + chown(&path, &tmp_path).await?; + tokio::fs::rename(tmp_path, &path).await?; let event = DocumentSavedEvent { revision: current_rev, From eaabcaa01892f243234a0d457780d89a9db678f0 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Fri, 5 Jan 2024 20:11:22 -0500 Subject: [PATCH 02/12] vendor faccess --- Cargo.lock | 24 +- helix-view/Cargo.toml | 2 + helix-view/src/document.rs | 21 +- helix-view/src/faccess.rs | 448 +++++++++++++++++++++++++++++++++++++ helix-view/src/file.rs | 1 + helix-view/src/lib.rs | 3 + 6 files changed, 489 insertions(+), 10 deletions(-) create mode 100644 helix-view/src/faccess.rs create mode 100644 helix-view/src/file.rs diff --git a/Cargo.lock b/Cargo.lock index b3009a925b2a..a52a91e3d834 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1435,6 +1435,7 @@ dependencies = [ "chardetng", "clipboard-win", "crossterm", + "filetime", "futures-util", "helix-core", "helix-dap", @@ -1457,6 +1458,8 @@ dependencies = [ "tokio-stream", "toml", "url", + "which", + "windows 0.52.0", ] [[package]] @@ -1488,7 +1491,7 @@ dependencies = [ "iana-time-zone-haiku", "js-sys", "wasm-bindgen", - "windows", + "windows 0.48.0", ] [[package]] @@ -2613,6 +2616,25 @@ dependencies = [ "windows-targets 0.48.0", ] +[[package]] +name = "windows" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e48a53791691ab099e5e2ad123536d0fff50652600abaf43bbf952894110d0be" +dependencies = [ + "windows-core", + "windows-targets 0.52.0", +] + +[[package]] +name = "windows-core" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "33ab640c8d7e35bf8ba19b884ba838ceb4fba93a4e8c65a9059d08afcfc683d9" +dependencies = [ + "windows-targets 0.52.0", +] + [[package]] name = "windows-sys" version = "0.45.0" diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index f1875ec47edf..528191db1519 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -28,6 +28,7 @@ anyhow = "1" crossterm = { version = "0.27", optional = true } tempfile = "3.9" +filetime = "0.2" # Conversion traits once_cell = "1.19" @@ -53,6 +54,7 @@ parking_lot = "0.12.1" [target.'cfg(windows)'.dependencies] clipboard-win = { version = "5.2", features = ["std"] } +windows = "0.52" [target.'cfg(unix)'.dependencies] libc = "0.2" diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 53db318b9d73..4c30ebf84a30 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -37,6 +37,7 @@ use helix_core::{ use crate::editor::Config; use crate::events::{DocumentDidChange, SelectionDidChange}; +use crate::faccess::readonly; use crate::{DocumentId, Editor, Theme, View, ViewId}; /// 8kB of buffer space for encoding and decoding `Rope`s. @@ -909,11 +910,9 @@ impl Document { } } - match rustix::fs::access(&path, rustix::fs::Access::WRITE_OK) { - Ok(_) => {} - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(_) => bail!("file is readonly"), - }; + if readonly(&path) { + bail!("File is readonly"); + } let (mut tmp_file, tmp_path) = tokio::task::spawn_blocking( move || -> anyhow::Result<(File, tempfile::TempPath)> { @@ -924,10 +923,14 @@ impl Document { .await??; to_writer(&mut tmp_file, encoding_with_bom_info, &text).await?; - let perms = tokio::fs::metadata(&path).await?.permissions(); - tmp_file.set_permissions(perms).await?; - chown(&path, &tmp_path).await?; - tokio::fs::rename(tmp_path, &path).await?; + { + let path = path.clone(); + tokio::task::spawn_blocking(move || -> anyhow::Result<()> { + tmp_path.persist(path)?; + Ok(()) + }) + } + .await??; let event = DocumentSavedEvent { revision: current_rev, diff --git a/helix-view/src/faccess.rs b/helix-view/src/faccess.rs new file mode 100644 index 000000000000..2b87e594a21f --- /dev/null +++ b/helix-view/src/faccess.rs @@ -0,0 +1,448 @@ +//! Forked from https://github.com/Freaky/faccess +//! Licensed under MIT + +use std::io; +use std::path::Path; + +use filetime::FileTime; + +use bitflags::bitflags; + +bitflags! { + /// Access mode flags for `access` function to test for. + pub struct AccessMode: u8 { + /// Path exists + const EXISTS = 0b0001; + /// Path can likely be read + const READ = 0b0010; + /// Path can likely be written to + const WRITE = 0b0100; + /// Path can likely be executed + const EXECUTE = 0b1000; + } +} + +#[cfg(unix)] +mod imp { + use super::*; + + use rustix::fs::Access; + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { + let mut imode = Access::empty(); + + if mode.contains(AccessMode::EXISTS) { + imode |= Access::EXISTS; + } + + if mode.contains(AccessMode::READ) { + imode |= Access::READ_OK; + } + + if mode.contains(AccessMode::WRITE) { + imode |= Access::WRITE_OK; + } + + if mode.contains(AccessMode::EXECUTE) { + imode |= Access::EXEC_OK; + } + + rustix::fs::access(p, imode)?; + Ok(()) + } + + fn chown(p: &Path, uid: Option, gid: Option) -> std::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 copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { + let meta = std::fs::File::open(from)?.metadata()?; + let uid = meta.gid(); + let gid = meta.uid(); + chown(to, Some(uid), Some(gid))?; + + let mut perms = meta.permissions(); + let new_perms = (perms.mode() & 0x0707) | (perms.mode() & 0x07) << 3; + perms.set_mode(new_perms); + + std::fs::set_permissions(to, perms)?; + + // TODO: Can be replaced by std::fs::FileTimes on 1.75 + let atime = FileTime::from_last_access_time(&meta); + let mtime = FileTime::from_last_modification_time(&meta); + filetime::set_file_times(to, atime, mtime)?; + + Ok(()) + } +} + +#[cfg(windows)] +mod imp { + use windows::core::PCWSTR; + use windows::Win32::Foundation::{CloseHandle, LocalFree, BOOL, HANDLE, HLOCAL, PSID}; + use windows::Win32::Security::Authorization::{ + GetNamedSecurityInfoW, SetNamedSecurityInfoW, SE_FILE_OBJECT, + }; + use windows::Win32::Security::{ + AccessCheck, AclSizeInformation, GetAce, GetAclInformation, GetSidIdentifierAuthority, + ImpersonateSelf, IsValidAcl, IsValidSid, MapGenericMask, RevertToSelf, + SecurityImpersonation, ACCESS_ALLOWED_ACE, ACL, ACL_SIZE_INFORMATION, + DACL_SECURITY_INFORMATION, GENERIC_MAPPING, GROUP_SECURITY_INFORMATION, INHERITED_ACE, + LABEL_SECURITY_INFORMATION, OBJECT_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, + PRIVILEGE_SET, PROTECTED_DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, + SID_IDENTIFIER_AUTHORITY, TOKEN_DUPLICATE, TOKEN_QUERY, + }; + use windows::Win32::Storage::FileSystem::{ + FILE_ACCESS_RIGHTS, FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, + FILE_GENERIC_WRITE, + }; + use windows::Win32::System::Threading::{GetCurrentThread, OpenThreadToken}; + + use super::*; + + use std::ffi::c_void; + + use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt}; + + struct SecurityDescriptor { + sd: PSECURITY_DESCRIPTOR, + owner: PSID, + group: PSID, + dacl: *mut ACL, + } + + impl Drop for SecurityDescriptor { + fn drop(&mut self) { + if !self.sd.0.is_null() { + unsafe { + let _ = LocalFree(HLOCAL(self.sd.0)); + } + } + } + } + + impl SecurityDescriptor { + fn for_path(p: &Path) -> std::io::Result { + let path = std::fs::canonicalize(p)?; + let pathos = path.into_os_string(); + let mut pathw: Vec = Vec::with_capacity(pathos.len() + 1); + pathw.extend(pathos.encode_wide()); + pathw.push(0); + + let mut sd = PSECURITY_DESCRIPTOR::default(); + let mut owner = PSID::default(); + let mut group = PSID::default(); + let mut dacl = std::ptr::null_mut(); + + unsafe { + GetNamedSecurityInfoW( + PCWSTR::from_raw(pathw.as_ptr()), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION + | GROUP_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | LABEL_SECURITY_INFORMATION, + Some(&mut owner), + Some(&mut group), + Some(&mut dacl), + None, + &mut sd, + )? + }; + + Ok(SecurityDescriptor { + sd, + owner, + group, + dacl, + }) + } + + fn is_acl_inherited(&self) -> std::io::Result { + let mut acl_info = ACL_SIZE_INFORMATION::default(); + let acl_info_ptr: *mut c_void = &mut acl_info as *mut _ as *mut c_void; + let mut ace = ACCESS_ALLOWED_ACE::default(); + + // Causes access violation when dacl is null. Is that UB? + unsafe { + GetAclInformation( + self.dacl, + acl_info_ptr, + std::mem::size_of_val(&acl_info) as u32, + AclSizeInformation, + ) + }?; + + for i in 0..acl_info.AceCount { + // TODO: check casting and returning result + let mut ptr = &mut ace as *mut _ as *mut c_void; + unsafe { GetAce(self.dacl, i, &mut ptr) }?; + if (ace.Header.AceFlags as u32 & INHERITED_ACE.0) != 0 { + return Ok(true); + } + } + + Ok(false) + } + + fn descriptor(&self) -> &PSECURITY_DESCRIPTOR { + &self.sd + } + + fn owner(&self) -> &PSID { + &self.owner + } + } + + struct ThreadToken(HANDLE); + impl Drop for ThreadToken { + fn drop(&mut self) { + unsafe { + let _ = CloseHandle(self.0); + } + } + } + + impl ThreadToken { + fn new() -> io::Result { + unsafe { + ImpersonateSelf(SecurityImpersonation)?; + let mut token = HANDLE::default(); + let err = OpenThreadToken( + GetCurrentThread(), + TOKEN_DUPLICATE | TOKEN_QUERY, + false, + &mut token, + ); + + RevertToSelf()?; + + err?; + + Ok(Self(token)) + } + } + + fn as_handle(&self) -> &HANDLE { + &self.0 + } + } + + // 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<()> { + let md = p.metadata()?; + + if !md.is_dir() { + // Read Only is ignored for directories + if mode & FILE_GENERIC_WRITE == FILE_GENERIC_WRITE && md.permissions().readonly() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "File is read only", + )); + } + + // If it doesn't have the correct extension it isn't executable + if mode & FILE_GENERIC_EXECUTE == FILE_GENERIC_EXECUTE { + if let Some(ext) = p.extension().and_then(|s| s.to_str()) { + match ext { + "exe" | "com" | "bat" | "cmd" => (), + _ => { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "File not executable", + )) + } + } + } + } + + return std::fs::OpenOptions::new() + .access_mode(mode.0) + .open(p) + .map(|_| ()); + } + + let sd = SecurityDescriptor::for_path(p)?; + + // Unmapped Samba users are assigned a top level authority of 22 + // ACL tests are likely to be misleading + const SAMBA_UNMAPPED: SID_IDENTIFIER_AUTHORITY = SID_IDENTIFIER_AUTHORITY { + Value: [0, 0, 0, 0, 0, 22], + }; + unsafe { + let owner = sd.owner(); + if IsValidSid(*owner).as_bool() + && (*GetSidIdentifierAuthority(*owner)).Value == SAMBA_UNMAPPED.Value + { + return Ok(()); + } + } + + let token = ThreadToken::new()?; + + let mut privileges: PRIVILEGE_SET = PRIVILEGE_SET::default(); + let mut granted_access: u32 = 0; + let mut privileges_length = std::mem::size_of::() as u32; + let mut result = BOOL(0); + + let mut mapping = GENERIC_MAPPING { + GenericRead: FILE_GENERIC_READ.0, + GenericWrite: FILE_GENERIC_WRITE.0, + GenericExecute: FILE_GENERIC_EXECUTE.0, + GenericAll: FILE_ALL_ACCESS.0, + }; + + unsafe { MapGenericMask(&mut mode.0, &mut mapping) }; + + unsafe { + AccessCheck( + *sd.descriptor(), + *token.as_handle(), + mode.0, + &mut mapping, + Some(&mut privileges), + &mut privileges_length as *mut _, + &mut granted_access as *mut _, + &mut result, + )? + }; + if !result.as_bool() { + Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Permission Denied", + )) + } else { + Ok(()) + } + } + + pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { + let mut imode = FILE_ACCESS_RIGHTS(0); + + if mode.contains(AccessMode::READ) { + imode |= FILE_GENERIC_READ; + } + + if mode.contains(AccessMode::WRITE) { + imode |= FILE_GENERIC_WRITE; + } + + if mode.contains(AccessMode::EXECUTE) { + imode |= FILE_GENERIC_EXECUTE; + } + + if imode.0 == 0 { + if p.exists() { + Ok(()) + } else { + Err(io::Error::new(io::ErrorKind::NotFound, "Not Found")) + } + } else { + eaccess(p, imode) + } + } + + fn chown(p: &Path, sd: SecurityDescriptor) -> std::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); + + let mut owner = PSID::default(); + let mut group = PSID::default(); + let mut dacl = None; + + let mut si = OBJECT_SECURITY_INFORMATION::default(); + if unsafe { IsValidSid(sd.owner) }.as_bool() { + si |= OWNER_SECURITY_INFORMATION; + owner = sd.owner; + } + + if unsafe { IsValidSid(sd.group) }.as_bool() { + si |= GROUP_SECURITY_INFORMATION; + group = sd.group; + } + + if unsafe { IsValidAcl(sd.dacl) }.as_bool() { + si |= DACL_SECURITY_INFORMATION; + if !sd.is_acl_inherited()? { + si |= PROTECTED_DACL_SECURITY_INFORMATION; + } + dacl = Some(sd.dacl as *const _); + } + + unsafe { + SetNamedSecurityInfoW( + PCWSTR::from_raw(pathw.as_ptr()), + SE_FILE_OBJECT, + si, + owner, + group, + dacl, + None, + )?; + } + + Ok(()) + } + + pub fn copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { + let sd = SecurityDescriptor::for_path(from)?; + chown(to, sd)?; + + let meta = std::fs::File::open(from)?.metadata()?; + let perms = meta.permissions(); + + std::fs::set_permissions(to, perms)?; + + // TODO: Can be replaced by std::fs::FileTimes on 1.75 + let atime = FileTime::from_last_access_time(&meta); + let mtime = FileTime::from_last_modification_time(&meta); + filetime::set_file_times(to, atime, mtime)?; + + Ok(()) + } +} + +#[cfg(not(any(unix, windows)))] +mod imp { + use super::*; + + pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { + if mode.contains(AccessMode::WRITE) { + if std::fs::metadata(p)?.permissions().readonly() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Path is read only", + )); + } else { + return Ok(()); + } + } + + if p.exists() { + Ok(()) + } else { + Err(io::Error::new(io::ErrorKind::NotFound, "Path not found")) + } + } + + pub fn copy_metadata(_from: &path, _to: &Path) -> std::io::Result<()> { + // Not possible + Ok(()) + } +} + +pub fn readonly(p: &Path) -> bool { + imp::access(p, AccessMode::READ).is_ok() && imp::access(p, AccessMode::WRITE).is_err() +} + +pub fn copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { + imp::copy_metadata(from, to) +} diff --git a/helix-view/src/file.rs b/helix-view/src/file.rs new file mode 100644 index 000000000000..8b137891791f --- /dev/null +++ b/helix-view/src/file.rs @@ -0,0 +1 @@ + diff --git a/helix-view/src/lib.rs b/helix-view/src/lib.rs index 14b6e1ce8138..ada5004c140c 100644 --- a/helix-view/src/lib.rs +++ b/helix-view/src/lib.rs @@ -2,6 +2,9 @@ pub mod macros; pub mod base64; + +mod faccess; + pub mod clipboard; pub mod document; pub mod editor; From 29e1e4dac5af44ce177e1c8fd93cd29848472113 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Mon, 8 Jan 2024 17:58:36 -0500 Subject: [PATCH 03/12] use faccess for readonly detection --- helix-view/Cargo.toml | 2 +- helix-view/src/document.rs | 86 +++++++++++++------------------------- helix-view/src/faccess.rs | 28 +++++++++---- helix-view/src/file.rs | 1 - 4 files changed, 48 insertions(+), 69 deletions(-) delete mode 100644 helix-view/src/file.rs diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 528191db1519..7d3083ff024c 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -58,7 +58,7 @@ windows = "0.52" [target.'cfg(unix)'.dependencies] libc = "0.2" -rustix = { version = "0.38", features = ["fs"] } +rustix = { version = "0.38", features = ["fs", "process"] } [dev-dependencies] helix-tui = { path = "../helix-tui" } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 4c30ebf84a30..4cd02ac78620 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -37,7 +37,7 @@ use helix_core::{ use crate::editor::Config; use crate::events::{DocumentDidChange, SelectionDidChange}; -use crate::faccess::readonly; +use crate::faccess::{copy_metadata, readonly}; use crate::{DocumentId, Editor, Theme, View, ViewId}; /// 8kB of buffer space for encoding and decoding `Rope`s. @@ -839,23 +839,6 @@ impl Document { impl Future> + 'static + Send, anyhow::Error, > { - #[cfg(unix)] - async fn chown(path: &Path, other: &Path) -> anyhow::Result<()> { - use std::os::unix::ffi::OsStrExt; - use std::os::unix::fs::MetadataExt; - let meta = tokio::fs::metadata(path).await?; - - // From https://github.com/uutils/coreutils/blob/2c73e978ba882c9cd56f0f16fae6f8dcce1c9850/src/uucore/src/lib/features/perms.rs#L46-L61 - let uid = meta.uid(); - let gid = meta.gid(); - let s = std::ffi::CString::new(other.as_os_str().as_bytes())?; - let ret = unsafe { libc::chown(s.as_ptr(), uid, gid) }; - if ret != 0 { - return Err(anyhow::anyhow!(tokio::io::Error::last_os_error())); - } - Ok(()) - } - log::debug!( "submitting save of doc '{:?}'", self.path().map(|path| path.to_string_lossy()) @@ -911,26 +894,36 @@ impl Document { } if readonly(&path) { - bail!("File is readonly"); + bail!(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "Path is read only" + )); } - let (mut tmp_file, tmp_path) = tokio::task::spawn_blocking( - move || -> anyhow::Result<(File, tempfile::TempPath)> { - let (f, p) = tempfile::NamedTempFile::new()?.into_parts(); - Ok((tokio::fs::File::from_std(f), p)) - }, - ) - .await??; - to_writer(&mut tmp_file, encoding_with_bom_info, &text).await?; + if cfg!(any(windows, unix)) { + let (mut tmp_file, tmp_path) = tokio::task::spawn_blocking( + move || -> anyhow::Result<(File, tempfile::TempPath)> { + let (f, p) = tempfile::NamedTempFile::new()?.into_parts(); + Ok((tokio::fs::File::from_std(f), p)) + }, + ) + .await??; + to_writer(&mut tmp_file, encoding_with_bom_info, &text).await?; - { - let path = path.clone(); - tokio::task::spawn_blocking(move || -> anyhow::Result<()> { - tmp_path.persist(path)?; - Ok(()) - }) + { + let path = path.clone(); + tokio::task::spawn_blocking(move || -> anyhow::Result<()> { + // Vim ignores errors + let _ = copy_metadata(&path, &tmp_path); + tmp_path.persist(path)?; + Ok(()) + }) + } + .await??; + } else { + let mut file = File::create(&path).await?; + to_writer(&mut file, encoding_with_bom_info, &text).await?; } - .await??; let event = DocumentSavedEvent { revision: current_rev, @@ -991,35 +984,12 @@ impl Document { } } - #[cfg(unix)] // Detect if the file is readonly and change the readonly field if necessary (unix only) pub fn detect_readonly(&mut self) { - use rustix::fs::{access, Access}; // Allows setting the flag for files the user cannot modify, like root files self.readonly = match &self.path { None => false, - Some(p) => match access(p, Access::WRITE_OK) { - Ok(_) => false, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => false, - Err(_) => true, - }, - }; - } - - #[cfg(not(unix))] - // Detect if the file is readonly and change the readonly field if necessary (non-unix os) - pub fn detect_readonly(&mut self) { - // TODO Use the Windows' function `CreateFileW` to check if a file is readonly - // Discussion: https://github.com/helix-editor/helix/pull/7740#issuecomment-1656806459 - // Vim implementation: https://github.com/vim/vim/blob/4c0089d696b8d1d5dc40568f25ea5738fa5bbffb/src/os_win32.c#L7665 - // Windows binding: https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Storage/FileSystem/fn.CreateFileW.html - self.readonly = match &self.path { - None => false, - Some(p) => match std::fs::metadata(p) { - Err(err) if err.kind() == std::io::ErrorKind::NotFound => false, - Err(_) => false, - Ok(metadata) => metadata.permissions().readonly(), - }, + Some(p) => readonly(p), }; } diff --git a/helix-view/src/faccess.rs b/helix-view/src/faccess.rs index 2b87e594a21f..62ed6a574175 100644 --- a/helix-view/src/faccess.rs +++ b/helix-view/src/faccess.rs @@ -1,5 +1,4 @@ -//! Forked from https://github.com/Freaky/faccess -//! Licensed under MIT +//! From use std::io; use std::path::Path; @@ -8,6 +7,7 @@ use filetime::FileTime; use bitflags::bitflags; +// Licensed under MIT from faccess bitflags! { /// Access mode flags for `access` function to test for. pub struct AccessMode: u8 { @@ -52,21 +52,21 @@ mod imp { Ok(()) } - fn chown(p: &Path, uid: Option, gid: Option) -> std::io::Result<()> { + fn chown(p: &Path, uid: Option, gid: Option) -> anyhow::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 copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { + pub fn copy_metadata(from: &Path, to: &Path) -> anyhow::Result<()> { let meta = std::fs::File::open(from)?.metadata()?; let uid = meta.gid(); let gid = meta.uid(); chown(to, Some(uid), Some(gid))?; let mut perms = meta.permissions(); - let new_perms = (perms.mode() & 0x0707) | (perms.mode() & 0x07) << 3; + let new_perms = (perms.mode() & 0o0707) | (perms.mode() & 0o07) << 3; perms.set_mode(new_perms); std::fs::set_permissions(to, perms)?; @@ -80,6 +80,7 @@ mod imp { } } +// Licensed under MIT from faccess except for `chown` and `copy_metadata` #[cfg(windows)] mod imp { use windows::core::PCWSTR; @@ -410,6 +411,7 @@ mod imp { } } +// Licensed under MIT from faccess #[cfg(not(any(unix, windows)))] mod imp { use super::*; @@ -434,15 +436,23 @@ mod imp { } pub fn copy_metadata(_from: &path, _to: &Path) -> std::io::Result<()> { - // Not possible + let meta = std::fs::File::open(from)?.metadata()?; + let perms = meta.permissions(); + std::fs::set_permissions(to, perms)?; + Ok(()) } } pub fn readonly(p: &Path) -> bool { - imp::access(p, AccessMode::READ).is_ok() && imp::access(p, AccessMode::WRITE).is_err() + match imp::access(p, AccessMode::WRITE) { + Ok(_) => false, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => false, + Err(_) => true, + } } -pub fn copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { - imp::copy_metadata(from, to) +pub fn copy_metadata(from: &Path, to: &Path) -> anyhow::Result<()> { + imp::copy_metadata(from, to)?; + Ok(()) } diff --git a/helix-view/src/file.rs b/helix-view/src/file.rs deleted file mode 100644 index 8b137891791f..000000000000 --- a/helix-view/src/file.rs +++ /dev/null @@ -1 +0,0 @@ - From 34a9420b87647cce01370c8299426b4b055b6a75 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 9 Jan 2024 15:15:30 +0100 Subject: [PATCH 04/12] write to the same directory as the destination --- helix-view/src/document.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 4cd02ac78620..d9443cf33737 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -901,24 +901,27 @@ impl Document { } if cfg!(any(windows, unix)) { + let path: Arc = path.as_path().into(); + let path_ = path.clone(); let (mut tmp_file, tmp_path) = tokio::task::spawn_blocking( move || -> anyhow::Result<(File, tempfile::TempPath)> { - let (f, p) = tempfile::NamedTempFile::new()?.into_parts(); + let (f, p) = tempfile::Builder::new() + .prefix(path_.file_name().unwrap()) + .suffix(".bck") + .tempfile_in(path_.parent().unwrap())? + .into_parts(); Ok((tokio::fs::File::from_std(f), p)) }, ) .await??; to_writer(&mut tmp_file, encoding_with_bom_info, &text).await?; - { - let path = path.clone(); - tokio::task::spawn_blocking(move || -> anyhow::Result<()> { - // Vim ignores errors - let _ = copy_metadata(&path, &tmp_path); - tmp_path.persist(path)?; - Ok(()) - }) - } + tokio::task::spawn_blocking(move || -> anyhow::Result<()> { + // Vim ignores errors + let _ = copy_metadata(&path, &tmp_path); + tmp_path.persist(path).unwrap(); + Ok(()) + }) .await??; } else { let mut file = File::create(&path).await?; From 2569cedc3081efecbc3a7e11412cf758eaba6374 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 9 Jan 2024 15:15:57 +0100 Subject: [PATCH 05/12] cleanup integration tests --- helix-term/tests/test/commands/write.rs | 51 ++++++++++--------------- helix-term/tests/test/helpers.rs | 19 ++++----- helix-term/tests/test/splits.rs | 11 ++---- helix-view/src/document.rs | 2 +- 4 files changed, 36 insertions(+), 47 deletions(-) diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index 39f4a8ed5d85..6c279212b17d 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -66,7 +66,7 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> { .await?; // verify if writes are queued up, it finishes them before closing the buffer - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut command = String::new(); const RANGE: RangeInclusive = 1..=1000; @@ -93,7 +93,6 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> { false, ) .await?; - let (mut file, _) = reload_file(file); helpers::assert_file_has_content(&mut file, &platform_line(&RANGE.end().to_string()))?; @@ -102,7 +101,7 @@ async fn test_buffer_close_concurrent() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .build()?; @@ -115,9 +114,9 @@ async fn test_write() -> anyhow::Result<()> { ) .await?; - let (mut file, _) = reload_file(file); + reload_file(&mut file).unwrap(); let mut file_content = String::new(); - file.read_to_string(&mut file_content)?; + file.as_file_mut().read_to_string(&mut file_content)?; assert_eq!( helpers::platform_line("the gostak distims the doshes"), @@ -144,7 +143,7 @@ async fn test_overwrite_protection() -> anyhow::Result<()> { test_key_sequence(&mut app, Some(":x"), None, false).await?; - let (mut file, _) = reload_file(file); + reload_file(&mut file).unwrap(); let mut file_content = String::new(); file.read_to_string(&mut file_content)?; @@ -158,7 +157,7 @@ async fn test_overwrite_protection() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_quit() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .build()?; @@ -171,7 +170,7 @@ async fn test_write_quit() -> anyhow::Result<()> { ) .await?; - let (mut file, _) = reload_file(file); + reload_file(&mut file).unwrap(); let mut file_content = String::new(); file.read_to_string(&mut file_content)?; @@ -186,7 +185,7 @@ async fn test_write_quit() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_concurrent() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut command = String::new(); const RANGE: RangeInclusive = 1..=1000; let mut app = helpers::AppBuilder::new() @@ -200,7 +199,7 @@ async fn test_write_concurrent() -> anyhow::Result<()> { test_key_sequence(&mut app, Some(&command), None, false).await?; - let (mut file, _) = reload_file(file); + reload_file(&mut file).unwrap(); let mut file_content = String::new(); file.read_to_string(&mut file_content)?; assert_eq!(platform_line(&RANGE.end().to_string()), file_content); @@ -251,7 +250,7 @@ async fn test_write_fail_mod_flag() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_scratch_to_new_path() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; test_key_sequence( &mut AppBuilder::new().build()?, @@ -269,7 +268,6 @@ async fn test_write_scratch_to_new_path() -> anyhow::Result<()> { ) .await?; - let (mut file, _) = reload_file(file); helpers::assert_file_has_content(&mut file, &helpers::platform_line("hello"))?; Ok(()) @@ -298,7 +296,7 @@ async fn test_write_scratch_no_path_fails() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> { - let file = tempfile::Builder::new().suffix(".rs").tempfile()?; + let mut file = tempfile::Builder::new().suffix(".rs").tempfile()?; let lang_conf = indoc! {r#" [[language]] @@ -314,7 +312,6 @@ async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> { test_key_sequences(&mut app, vec![(Some(":w"), None)], false).await?; - let (mut file, _) = reload_file(file); // file still saves helpers::assert_file_has_content(&mut file, "let foo = 0;\n")?; @@ -323,8 +320,8 @@ async fn test_write_auto_format_fails_still_writes() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_new_path() -> anyhow::Result<()> { - let file1 = tempfile::NamedTempFile::new().unwrap(); - let file2 = tempfile::NamedTempFile::new().unwrap(); + let mut file1 = tempfile::NamedTempFile::new().unwrap(); + let mut file2 = tempfile::NamedTempFile::new().unwrap(); let mut app = helpers::AppBuilder::new() .with_file(file1.path(), None) .build()?; @@ -354,8 +351,6 @@ async fn test_write_new_path() -> anyhow::Result<()> { ) .await?; - let (mut file1, _) = reload_file(file1); - let (mut file2, _) = reload_file(file2); helpers::assert_file_has_content( &mut file1, &helpers::platform_line("i can eat glass, it will not hurt me\n"), @@ -424,7 +419,7 @@ async fn test_write_utf_bom_file() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .with_input_text("#[h|]#ave you tried chamomile tea?") @@ -432,7 +427,6 @@ async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<() test_key_sequence(&mut app, Some(":w"), None, false).await?; - let (mut file, _) = reload_file(file); helpers::assert_file_has_content( &mut file, &helpers::platform_line("have you tried chamomile tea?\n"), @@ -443,7 +437,7 @@ async fn test_write_insert_final_newline_added_if_missing() -> anyhow::Result<() #[tokio::test(flavor = "multi_thread")] async fn test_write_insert_final_newline_unchanged_if_not_missing() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file.path(), None) .with_input_text(&helpers::platform_line("#[t|]#en minutes, please\n")) @@ -451,7 +445,6 @@ async fn test_write_insert_final_newline_unchanged_if_not_missing() -> anyhow::R test_key_sequence(&mut app, Some(":w"), None, false).await?; - let (mut file, _) = reload_file(file); helpers::assert_file_has_content(&mut file, &helpers::platform_line("ten minutes, please\n"))?; Ok(()) @@ -459,7 +452,7 @@ async fn test_write_insert_final_newline_unchanged_if_not_missing() -> anyhow::R #[tokio::test(flavor = "multi_thread")] async fn test_write_insert_final_newline_unchanged_if_missing_and_false() -> anyhow::Result<()> { - let file = tempfile::NamedTempFile::new()?; + let mut file = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_config(Config { editor: helix_view::editor::Config { @@ -474,7 +467,7 @@ async fn test_write_insert_final_newline_unchanged_if_missing_and_false() -> any test_key_sequence(&mut app, Some(":w"), None, false).await?; - let (mut file, _) = reload_file(file); + reload_file(&mut file).unwrap(); helpers::assert_file_has_content(&mut file, "the quiet rain continued through the night")?; Ok(()) @@ -482,8 +475,8 @@ async fn test_write_insert_final_newline_unchanged_if_missing_and_false() -> any #[tokio::test(flavor = "multi_thread")] async fn test_write_all_insert_final_newline_add_if_missing_and_modified() -> anyhow::Result<()> { - let file1 = tempfile::NamedTempFile::new()?; - let file2 = tempfile::NamedTempFile::new()?; + let mut file1 = tempfile::NamedTempFile::new()?; + let mut file2 = tempfile::NamedTempFile::new()?; let mut app = helpers::AppBuilder::new() .with_file(file1.path(), None) .with_input_text("#[w|]#e don't serve time travelers here") @@ -500,8 +493,6 @@ async fn test_write_all_insert_final_newline_add_if_missing_and_modified() -> an ) .await?; - let (mut file1, _) = reload_file(file1); - let (mut file2, _) = reload_file(file2); helpers::assert_file_has_content( &mut file1, &helpers::platform_line("we don't serve time travelers here\n"), @@ -527,7 +518,7 @@ async fn test_write_all_insert_final_newline_do_not_add_if_unmodified() -> anyho test_key_sequence(&mut app, Some(":wa"), None, false).await?; - helpers::assert_file_has_content(file.as_file_mut(), "i lost on Jeopardy!")?; + helpers::assert_file_has_content(&mut file, "i lost on Jeopardy!")?; Ok(()) } @@ -553,7 +544,7 @@ async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { ) .await?; - let (mut file, _) = reload_file(file); + reload_file(&mut file).unwrap(); let mut new_file_content: Vec = Vec::new(); file.read_to_end(&mut new_file_content)?; diff --git a/helix-term/tests/test/helpers.rs b/helix-term/tests/test/helpers.rs index 02e63974d9de..906fa32249f9 100644 --- a/helix-term/tests/test/helpers.rs +++ b/helix-term/tests/test/helpers.rs @@ -1,5 +1,4 @@ use std::{ - fs::File, io::{Read, Write}, mem::replace, path::PathBuf, @@ -11,7 +10,7 @@ use crossterm::event::{Event, KeyEvent}; use helix_core::{diagnostic::Severity, test, Selection, Transaction}; use helix_term::{application::Application, args::Args, config::Config, keymap::merge_keys}; use helix_view::{current_ref, doc, editor::LspConfig, input::parse_macro, Editor}; -use tempfile::{NamedTempFile, TempPath}; +use tempfile::NamedTempFile; use tokio_stream::wrappers::UnboundedReceiverStream; #[derive(Clone, Debug)] @@ -352,9 +351,8 @@ pub async fn run_event_loop_until_idle(app: &mut Application) { app.event_loop_until_idle(&mut rx_stream).await; } -pub fn assert_file_has_content(file: &mut File, content: &str) -> anyhow::Result<()> { - file.flush()?; - file.sync_all()?; +pub fn assert_file_has_content(file: &mut NamedTempFile, content: &str) -> anyhow::Result<()> { + reload_file(file)?; let mut file_content = String::new(); file.read_to_string(&mut file_content)?; @@ -369,8 +367,11 @@ pub fn assert_status_not_error(editor: &Editor) { } } -pub fn reload_file(t: NamedTempFile) -> (File, TempPath) { - let p = t.into_temp_path(); - let f = std::fs::File::open(&p).unwrap(); - (f, p) +pub fn reload_file(file: &mut NamedTempFile) -> anyhow::Result<()> { + let path = file.path(); + let f = std::fs::File::open(&path).unwrap(); + let file = file.as_file_mut(); + *file = f; + file.sync_all()?; + Ok(()) } diff --git a/helix-term/tests/test/splits.rs b/helix-term/tests/test/splits.rs index 3b66c0486421..d76c698c896b 100644 --- a/helix-term/tests/test/splits.rs +++ b/helix-term/tests/test/splits.rs @@ -62,9 +62,9 @@ async fn test_split_write_quit_all() -> anyhow::Result<()> { ) .await?; - helpers::assert_file_has_content(file1.as_file_mut(), &platform_line("hello1"))?; - helpers::assert_file_has_content(file2.as_file_mut(), &platform_line("hello2"))?; - helpers::assert_file_has_content(file3.as_file_mut(), &platform_line("hello3"))?; + helpers::assert_file_has_content(&mut file1, &platform_line("hello1"))?; + helpers::assert_file_has_content(&mut file2, &platform_line("hello2"))?; + helpers::assert_file_has_content(&mut file3, &platform_line("hello3"))?; Ok(()) } @@ -122,10 +122,7 @@ async fn test_split_write_quit_same_file() -> anyhow::Result<()> { ) .await?; - helpers::assert_file_has_content( - file.as_file_mut(), - &helpers::platform_line("hello\ngoodbye"), - )?; + helpers::assert_file_has_content(&mut file, &helpers::platform_line("hello\ngoodbye"))?; Ok(()) } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index d9443cf33737..7f486ad8ccde 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -919,7 +919,7 @@ impl Document { tokio::task::spawn_blocking(move || -> anyhow::Result<()> { // Vim ignores errors let _ = copy_metadata(&path, &tmp_path); - tmp_path.persist(path).unwrap(); + tmp_path.persist(path)?; Ok(()) }) .await??; From 945a15c3de7a9056b7a38010348ff5892c5fb317 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 9 Jan 2024 16:38:20 +0100 Subject: [PATCH 06/12] create backup file instead of writing to temporary file --- helix-term/tests/test/helpers.rs | 6 ++- helix-view/src/document.rs | 69 ++++++++++++++++++++------------ 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/helix-term/tests/test/helpers.rs b/helix-term/tests/test/helpers.rs index 906fa32249f9..dcbb09021182 100644 --- a/helix-term/tests/test/helpers.rs +++ b/helix-term/tests/test/helpers.rs @@ -369,7 +369,11 @@ pub fn assert_status_not_error(editor: &Editor) { pub fn reload_file(file: &mut NamedTempFile) -> anyhow::Result<()> { let path = file.path(); - let f = std::fs::File::open(&path).unwrap(); + let f = std::fs::OpenOptions::new() + .write(true) + .read(true) + .open(&path) + .unwrap(); let file = file.as_file_mut(); *file = f; file.sync_all()?; diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 7f486ad8ccde..a4d22db863e4 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -870,7 +870,7 @@ impl Document { // We encode the file according to the `Document`'s encoding. let future = async move { - use tokio::{fs, fs::File}; + use tokio::fs; if let Some(parent) = path.parent() { // TODO: display a prompt asking the user if the directories should be created if !parent.exists() { @@ -899,35 +899,54 @@ impl Document { "Path is read only" )); } - - if cfg!(any(windows, unix)) { - let path: Arc = path.as_path().into(); + let backup = if path.exists() { let path_ = path.clone(); - let (mut tmp_file, tmp_path) = tokio::task::spawn_blocking( - move || -> anyhow::Result<(File, tempfile::TempPath)> { - let (f, p) = tempfile::Builder::new() - .prefix(path_.file_name().unwrap()) - .suffix(".bck") - .tempfile_in(path_.parent().unwrap())? - .into_parts(); - Ok((tokio::fs::File::from_std(f), p)) - }, - ) - .await??; - to_writer(&mut tmp_file, encoding_with_bom_info, &text).await?; - - tokio::task::spawn_blocking(move || -> anyhow::Result<()> { - // Vim ignores errors - let _ = copy_metadata(&path, &tmp_path); - tmp_path.persist(path)?; - Ok(()) + // 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 { + tempfile::Builder::new() + .prefix(path_.file_name()?) + .suffix(".bck") + .make_in(path_.parent()?, |backup| std::fs::rename(&path_, backup)) + .ok()? + .into_temp_path() + .keep() + .ok() }) - .await??; + .await + .ok() + .flatten() } else { - let mut file = File::create(&path).await?; - to_writer(&mut file, encoding_with_bom_info, &text).await?; + None + }; + + let write_result: anyhow::Result<_> = async { + let mut dst = tokio::fs::File::create(&path).await?; + to_writer(&mut dst, encoding_with_bom_info, &text).await?; + Ok(()) + } + .await; + + if let Some(backup) = backup { + if write_result.is_err() { + // restore backup + let _ = tokio::fs::rename(&backup, &path).await; + } else { + // copy metadata and delete backup + let path_ = path.clone(); + let _ = tokio::task::spawn_blocking(move || { + let _ = copy_metadata(&backup, &path_).unwrap(); + let _ = std::fs::remove_file(backup); + }) + .await; + } } + write_result?; + let event = DocumentSavedEvent { revision: current_rev, doc_id, From 78a5e4640313bdb8a685313dd1c06a5253ac4ee9 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 9 Jan 2024 18:17:26 +0100 Subject: [PATCH 07/12] fixup integration test --- helix-term/tests/test/helpers.rs | 7 ++----- helix-view/src/document.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/helix-term/tests/test/helpers.rs b/helix-term/tests/test/helpers.rs index dcbb09021182..f8387b974dd9 100644 --- a/helix-term/tests/test/helpers.rs +++ b/helix-term/tests/test/helpers.rs @@ -372,10 +372,7 @@ pub fn reload_file(file: &mut NamedTempFile) -> anyhow::Result<()> { let f = std::fs::OpenOptions::new() .write(true) .read(true) - .open(&path) - .unwrap(); - let file = file.as_file_mut(); - *file = f; - file.sync_all()?; + .open(&path)?; + *file.as_file_mut() = f; Ok(()) } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index a4d22db863e4..559c1009cdf5 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -938,7 +938,7 @@ impl Document { // copy metadata and delete backup let path_ = path.clone(); let _ = tokio::task::spawn_blocking(move || { - let _ = copy_metadata(&backup, &path_).unwrap(); + let _ = copy_metadata(&backup, &path_); let _ = std::fs::remove_file(backup); }) .await; From 3f10bb3b6b0397fa91bae21965196d5a09bc2301 Mon Sep 17 00:00:00 2001 From: Mason Mac <67773714+kirawi@users.noreply.github.com> Date: Tue, 9 Jan 2024 14:21:17 -0500 Subject: [PATCH 08/12] move to windows_sys and log errors --- Cargo.lock | 2 +- helix-view/Cargo.toml | 4 +- helix-view/src/document.rs | 10 +- helix-view/src/faccess.rs | 190 ++++++++++++++++++++----------------- 4 files changed, 113 insertions(+), 93 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a52a91e3d834..fe2f896d8fc8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1459,7 +1459,7 @@ dependencies = [ "toml", "url", "which", - "windows 0.52.0", + "windows-sys 0.52.0", ] [[package]] diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 7d3083ff024c..7c61e9d2c137 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -54,11 +54,11 @@ parking_lot = "0.12.1" [target.'cfg(windows)'.dependencies] clipboard-win = { version = "5.2", features = ["std"] } -windows = "0.52" +windows-sys = { version = "0.52", features = ["Win32_Security", "Win32_Security_Authorization"] } [target.'cfg(unix)'.dependencies] libc = "0.2" -rustix = { version = "0.38", features = ["fs", "process"] } +rustix = { version = "0.38", features = ["fs"] } [dev-dependencies] helix-tui = { path = "../helix-tui" } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 559c1009cdf5..35ef2962fc4d 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -933,13 +933,17 @@ impl Document { if let Some(backup) = backup { if write_result.is_err() { // restore backup - let _ = tokio::fs::rename(&backup, &path).await; + let _ = tokio::fs::rename(&backup, &path) + .await + .map_err(|e| log::error!("Failed to restore backup on write failure: {e}")); } else { // copy metadata and delete backup let path_ = path.clone(); let _ = tokio::task::spawn_blocking(move || { - let _ = copy_metadata(&backup, &path_); - let _ = std::fs::remove_file(backup); + let _ = copy_metadata(&backup, &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/faccess.rs b/helix-view/src/faccess.rs index 62ed6a574175..640853949321 100644 --- a/helix-view/src/faccess.rs +++ b/helix-view/src/faccess.rs @@ -80,28 +80,28 @@ mod imp { } } -// Licensed under MIT from faccess except for `chown` and `copy_metadata` +// Licensed under MIT from faccess except for `chown`, `copy_metadata` and `is_acl_inherited` #[cfg(windows)] mod imp { - use windows::core::PCWSTR; - use windows::Win32::Foundation::{CloseHandle, LocalFree, BOOL, HANDLE, HLOCAL, PSID}; - use windows::Win32::Security::Authorization::{ + + use windows_sys::Win32::Foundation::{CloseHandle, LocalFree, ERROR_SUCCESS, HANDLE, PSID}; + use windows_sys::Win32::Security::Authorization::{ GetNamedSecurityInfoW, SetNamedSecurityInfoW, SE_FILE_OBJECT, }; - use windows::Win32::Security::{ + use windows_sys::Win32::Security::{ AccessCheck, AclSizeInformation, GetAce, GetAclInformation, GetSidIdentifierAuthority, ImpersonateSelf, IsValidAcl, IsValidSid, MapGenericMask, RevertToSelf, - SecurityImpersonation, ACCESS_ALLOWED_ACE, ACL, ACL_SIZE_INFORMATION, + SecurityImpersonation, ACCESS_ALLOWED_CALLBACK_ACE, ACL, ACL_SIZE_INFORMATION, DACL_SECURITY_INFORMATION, GENERIC_MAPPING, GROUP_SECURITY_INFORMATION, INHERITED_ACE, LABEL_SECURITY_INFORMATION, OBJECT_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, PRIVILEGE_SET, PROTECTED_DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, SID_IDENTIFIER_AUTHORITY, TOKEN_DUPLICATE, TOKEN_QUERY, }; - use windows::Win32::Storage::FileSystem::{ + use windows_sys::Win32::Storage::FileSystem::{ FILE_ACCESS_RIGHTS, FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE, }; - use windows::Win32::System::Threading::{GetCurrentThread, OpenThreadToken}; + use windows_sys::Win32::System::Threading::{GetCurrentThread, OpenThreadToken}; use super::*; @@ -118,9 +118,9 @@ mod imp { impl Drop for SecurityDescriptor { fn drop(&mut self) { - if !self.sd.0.is_null() { + if !self.sd.is_null() { unsafe { - let _ = LocalFree(HLOCAL(self.sd.0)); + LocalFree(self.sd); } } } @@ -134,41 +134,44 @@ mod imp { pathw.extend(pathos.encode_wide()); pathw.push(0); - let mut sd = PSECURITY_DESCRIPTOR::default(); - let mut owner = PSID::default(); - let mut group = PSID::default(); + let mut sd = std::ptr::null_mut(); + let mut owner = std::ptr::null_mut(); + let mut group = std::ptr::null_mut(); let mut dacl = std::ptr::null_mut(); - unsafe { + let err = unsafe { GetNamedSecurityInfoW( - PCWSTR::from_raw(pathw.as_ptr()), + pathw.as_ptr(), SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION, - Some(&mut owner), - Some(&mut group), - Some(&mut dacl), - None, + &mut owner, + &mut group, + &mut dacl, + std::ptr::null_mut(), &mut sd, - )? + ) }; - Ok(SecurityDescriptor { - sd, - owner, - group, - dacl, - }) + if err == ERROR_SUCCESS { + Ok(SecurityDescriptor { + sd, + owner, + group, + dacl, + }) + } else { + Err(io::Error::last_os_error()) + } } - fn is_acl_inherited(&self) -> std::io::Result { - let mut acl_info = ACL_SIZE_INFORMATION::default(); + fn is_acl_inherited(&self) -> bool { + let mut acl_info: ACL_SIZE_INFORMATION = unsafe { ::core::mem::zeroed() }; let acl_info_ptr: *mut c_void = &mut acl_info as *mut _ as *mut c_void; - let mut ace = ACCESS_ALLOWED_ACE::default(); + let mut ace: ACCESS_ALLOWED_CALLBACK_ACE = unsafe { ::core::mem::zeroed() }; - // Causes access violation when dacl is null. Is that UB? unsafe { GetAclInformation( self.dacl, @@ -176,18 +179,17 @@ mod imp { std::mem::size_of_val(&acl_info) as u32, AclSizeInformation, ) - }?; + }; for i in 0..acl_info.AceCount { - // TODO: check casting and returning result let mut ptr = &mut ace as *mut _ as *mut c_void; - unsafe { GetAce(self.dacl, i, &mut ptr) }?; - if (ace.Header.AceFlags as u32 & INHERITED_ACE.0) != 0 { - return Ok(true); + unsafe { GetAce(self.dacl, i, &mut ptr) }; + if (ace.Header.AceFlags as u32 & INHERITED_ACE) != 0 { + return true; } } - Ok(false) + false } fn descriptor(&self) -> &PSECURITY_DESCRIPTOR { @@ -203,7 +205,7 @@ mod imp { impl Drop for ThreadToken { fn drop(&mut self) { unsafe { - let _ = CloseHandle(self.0); + CloseHandle(self.0); } } } @@ -211,20 +213,21 @@ mod imp { impl ThreadToken { fn new() -> io::Result { unsafe { - ImpersonateSelf(SecurityImpersonation)?; - let mut token = HANDLE::default(); - let err = OpenThreadToken( - GetCurrentThread(), - TOKEN_DUPLICATE | TOKEN_QUERY, - false, - &mut token, - ); + if ImpersonateSelf(SecurityImpersonation) == 0 { + return Err(io::Error::last_os_error()); + } - RevertToSelf()?; + let token: *mut HANDLE = std::ptr::null_mut(); + let err = + OpenThreadToken(GetCurrentThread(), TOKEN_DUPLICATE | TOKEN_QUERY, 0, token); - err?; + RevertToSelf(); + + if err == 0 { + return Err(io::Error::last_os_error()); + } - Ok(Self(token)) + Ok(Self(*token)) } } @@ -263,7 +266,7 @@ mod imp { } return std::fs::OpenOptions::new() - .access_mode(mode.0) + .access_mode(mode) .open(p) .map(|_| ()); } @@ -277,7 +280,7 @@ mod imp { }; unsafe { let owner = sd.owner(); - if IsValidSid(*owner).as_bool() + if IsValidSid(*owner) != 0 && (*GetSidIdentifierAuthority(*owner)).Value == SAMBA_UNMAPPED.Value { return Ok(()); @@ -286,44 +289,48 @@ mod imp { let token = ThreadToken::new()?; - let mut privileges: PRIVILEGE_SET = PRIVILEGE_SET::default(); + let mut privileges: PRIVILEGE_SET = unsafe { std::mem::zeroed() }; let mut granted_access: u32 = 0; let mut privileges_length = std::mem::size_of::() as u32; - let mut result = BOOL(0); + let mut result = 0; let mut mapping = GENERIC_MAPPING { - GenericRead: FILE_GENERIC_READ.0, - GenericWrite: FILE_GENERIC_WRITE.0, - GenericExecute: FILE_GENERIC_EXECUTE.0, - GenericAll: FILE_ALL_ACCESS.0, + GenericRead: FILE_GENERIC_READ, + GenericWrite: FILE_GENERIC_WRITE, + GenericExecute: FILE_GENERIC_EXECUTE, + GenericAll: FILE_ALL_ACCESS, }; - unsafe { MapGenericMask(&mut mode.0, &mut mapping) }; + unsafe { MapGenericMask(&mut mode, &mut mapping) }; - unsafe { + if unsafe { AccessCheck( *sd.descriptor(), *token.as_handle(), - mode.0, + mode, &mut mapping, - Some(&mut privileges), - &mut privileges_length as *mut _, - &mut granted_access as *mut _, + &mut privileges, + &mut privileges_length, + &mut granted_access, &mut result, - )? - }; - if !result.as_bool() { - Err(io::Error::new( - io::ErrorKind::PermissionDenied, - "Permission Denied", - )) + ) + } != 0 + { + if result == 0 { + Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Permission Denied", + )) + } else { + Ok(()) + } } else { - Ok(()) + Err(io::Error::last_os_error()) } } pub fn access(p: &Path, mode: AccessMode) -> io::Result<()> { - let mut imode = FILE_ACCESS_RIGHTS(0); + let mut imode = 0; if mode.contains(AccessMode::READ) { imode |= FILE_GENERIC_READ; @@ -337,7 +344,7 @@ mod imp { imode |= FILE_GENERIC_EXECUTE; } - if imode.0 == 0 { + if imode == 0 { if p.exists() { Ok(()) } else { @@ -355,42 +362,46 @@ mod imp { pathw.extend(pathos.encode_wide()); pathw.push(0); - let mut owner = PSID::default(); - let mut group = PSID::default(); - let mut dacl = None; + let mut owner = std::ptr::null_mut(); + let mut group = std::ptr::null_mut(); + let mut dacl = std::ptr::null(); let mut si = OBJECT_SECURITY_INFORMATION::default(); - if unsafe { IsValidSid(sd.owner) }.as_bool() { + if unsafe { IsValidSid(sd.owner) } != 0 { si |= OWNER_SECURITY_INFORMATION; owner = sd.owner; } - if unsafe { IsValidSid(sd.group) }.as_bool() { + if unsafe { IsValidSid(sd.group) } != 0 { si |= GROUP_SECURITY_INFORMATION; group = sd.group; } - if unsafe { IsValidAcl(sd.dacl) }.as_bool() { + if unsafe { IsValidAcl(sd.dacl) } != 0 { si |= DACL_SECURITY_INFORMATION; - if !sd.is_acl_inherited()? { + if !sd.is_acl_inherited() { si |= PROTECTED_DACL_SECURITY_INFORMATION; } - dacl = Some(sd.dacl as *const _); + dacl = sd.dacl as *const _; } - unsafe { + let err = unsafe { SetNamedSecurityInfoW( - PCWSTR::from_raw(pathw.as_ptr()), + pathw.as_ptr(), SE_FILE_OBJECT, si, owner, group, dacl, - None, - )?; - } + std::ptr::null(), + ) + }; - Ok(()) + if err == ERROR_SUCCESS { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } } pub fn copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { @@ -411,7 +422,7 @@ mod imp { } } -// Licensed under MIT from faccess +// Licensed under MIT from faccess except for `copy_metadata` #[cfg(not(any(unix, windows)))] mod imp { use super::*; @@ -440,6 +451,11 @@ mod imp { let perms = meta.permissions(); std::fs::set_permissions(to, perms)?; + // TODO: Can be replaced by std::fs::FileTimes on 1.75 + let atime = FileTime::from_last_access_time(&meta); + let mtime = FileTime::from_last_modification_time(&meta); + filetime::set_file_times(to, atime, mtime)?; + Ok(()) } } From 313a3a8e84fc80e82b81fa08b12926f2d859a9d0 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Tue, 9 Jan 2024 16:27:48 -0500 Subject: [PATCH 09/12] fix non-unix faccess copy_metadata impl --- helix-view/src/faccess.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-view/src/faccess.rs b/helix-view/src/faccess.rs index 640853949321..eec14aeb69ea 100644 --- a/helix-view/src/faccess.rs +++ b/helix-view/src/faccess.rs @@ -446,7 +446,7 @@ mod imp { } } - pub fn copy_metadata(_from: &path, _to: &Path) -> std::io::Result<()> { + pub fn copy_metadata(from: &path, to: &Path) -> std::io::Result<()> { let meta = std::fs::File::open(from)?.metadata()?; let perms = meta.permissions(); std::fs::set_permissions(to, perms)?; From e9c35cb6cb35098f0f3168471aa1e673dbd4b441 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Thu, 11 Jan 2024 13:19:52 -0500 Subject: [PATCH 10/12] don't copy filetimes --- Cargo.lock | 13 ------------- helix-view/Cargo.toml | 1 - helix-view/src/faccess.rs | 17 ----------------- 3 files changed, 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fe2f896d8fc8..33c5c0946fbd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -445,18 +445,6 @@ dependencies = [ "winapi", ] -[[package]] -name = "filetime" -version = "0.2.23" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ee447700ac8aa0b2f2bd7bc4462ad686ba06baa6727ac149a2d6277f0d240fd" -dependencies = [ - "cfg-if", - "libc", - "redox_syscall 0.4.1", - "windows-sys 0.52.0", -] - [[package]] name = "flate2" version = "1.0.27" @@ -1435,7 +1423,6 @@ dependencies = [ "chardetng", "clipboard-win", "crossterm", - "filetime", "futures-util", "helix-core", "helix-dap", diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 7c61e9d2c137..02deaa5132f8 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -28,7 +28,6 @@ anyhow = "1" crossterm = { version = "0.27", optional = true } tempfile = "3.9" -filetime = "0.2" # Conversion traits once_cell = "1.19" diff --git a/helix-view/src/faccess.rs b/helix-view/src/faccess.rs index eec14aeb69ea..f6ec6a34bd20 100644 --- a/helix-view/src/faccess.rs +++ b/helix-view/src/faccess.rs @@ -3,8 +3,6 @@ use std::io; use std::path::Path; -use filetime::FileTime; - use bitflags::bitflags; // Licensed under MIT from faccess @@ -71,11 +69,6 @@ mod imp { std::fs::set_permissions(to, perms)?; - // TODO: Can be replaced by std::fs::FileTimes on 1.75 - let atime = FileTime::from_last_access_time(&meta); - let mtime = FileTime::from_last_modification_time(&meta); - filetime::set_file_times(to, atime, mtime)?; - Ok(()) } } @@ -413,11 +406,6 @@ mod imp { std::fs::set_permissions(to, perms)?; - // TODO: Can be replaced by std::fs::FileTimes on 1.75 - let atime = FileTime::from_last_access_time(&meta); - let mtime = FileTime::from_last_modification_time(&meta); - filetime::set_file_times(to, atime, mtime)?; - Ok(()) } } @@ -451,11 +439,6 @@ mod imp { let perms = meta.permissions(); std::fs::set_permissions(to, perms)?; - // TODO: Can be replaced by std::fs::FileTimes on 1.75 - let atime = FileTime::from_last_access_time(&meta); - let mtime = FileTime::from_last_modification_time(&meta); - filetime::set_file_times(to, atime, mtime)?; - Ok(()) } } From 3c8cd95f54fb7239458554d3e84e14bc87f4f870 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Thu, 11 Jan 2024 13:34:35 -0500 Subject: [PATCH 11/12] fix copy_metadata perms --- helix-view/src/faccess.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/helix-view/src/faccess.rs b/helix-view/src/faccess.rs index f6ec6a34bd20..d83c4144db46 100644 --- a/helix-view/src/faccess.rs +++ b/helix-view/src/faccess.rs @@ -58,14 +58,16 @@ mod imp { } pub fn copy_metadata(from: &Path, to: &Path) -> anyhow::Result<()> { - let meta = std::fs::File::open(from)?.metadata()?; - let uid = meta.gid(); - let gid = meta.uid(); - chown(to, Some(uid), Some(gid))?; - - let mut perms = meta.permissions(); - let new_perms = (perms.mode() & 0o0707) | (perms.mode() & 0o07) << 3; - perms.set_mode(new_perms); + 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(); + 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(to, perms)?; @@ -401,7 +403,7 @@ mod imp { let sd = SecurityDescriptor::for_path(from)?; chown(to, sd)?; - let meta = std::fs::File::open(from)?.metadata()?; + let meta = std::fs::metadata(from)?; let perms = meta.permissions(); std::fs::set_permissions(to, perms)?; @@ -435,7 +437,7 @@ mod imp { } pub fn copy_metadata(from: &path, to: &Path) -> std::io::Result<()> { - let meta = std::fs::File::open(from)?.metadata()?; + let meta = std::fs::metadata(from)?; let perms = meta.permissions(); std::fs::set_permissions(to, perms)?; From 3e315ef127c3ba055e09db3cc93ed4c1cf08e281 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Thu, 18 Jan 2024 14:16:49 -0500 Subject: [PATCH 12/12] move faccess to helix-stdx --- Cargo.lock | 38 ++++++++++------------- helix-stdx/Cargo.toml | 7 +++++ {helix-view => helix-stdx}/src/faccess.rs | 18 +++++------ helix-stdx/src/lib.rs | 1 + helix-view/Cargo.toml | 1 - helix-view/src/document.rs | 2 +- helix-view/src/lib.rs | 3 -- 7 files changed, 34 insertions(+), 36 deletions(-) rename {helix-view => helix-stdx}/src/faccess.rs (95%) diff --git a/Cargo.lock b/Cargo.lock index 33c5c0946fbd..a79e6555c6d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -445,6 +445,18 @@ dependencies = [ "winapi", ] +[[package]] +name = "filetime" +version = "0.2.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1ee447700ac8aa0b2f2bd7bc4462ad686ba06baa6727ac149a2d6277f0d240fd" +dependencies = [ + "cfg-if", + "libc", + "redox_syscall 0.4.1", + "windows-sys 0.52.0", +] + [[package]] name = "flate2" version = "1.0.27" @@ -1330,12 +1342,15 @@ version = "23.10.0" name = "helix-stdx" version = "23.10.0" dependencies = [ + "bitflags 2.4.2", "dunce", "etcetera", "regex-cursor", "ropey", + "rustix", "tempfile", "which", + "windows-sys 0.52.0", ] [[package]] @@ -1445,8 +1460,6 @@ dependencies = [ "tokio-stream", "toml", "url", - "which", - "windows-sys 0.52.0", ] [[package]] @@ -1478,7 +1491,7 @@ dependencies = [ "iana-time-zone-haiku", "js-sys", "wasm-bindgen", - "windows 0.48.0", + "windows", ] [[package]] @@ -2603,25 +2616,6 @@ dependencies = [ "windows-targets 0.48.0", ] -[[package]] -name = "windows" -version = "0.52.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e48a53791691ab099e5e2ad123536d0fff50652600abaf43bbf952894110d0be" -dependencies = [ - "windows-core", - "windows-targets 0.52.0", -] - -[[package]] -name = "windows-core" -version = "0.52.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "33ab640c8d7e35bf8ba19b884ba838ceb4fba93a4e8c65a9059d08afcfc683d9" -dependencies = [ - "windows-targets 0.52.0", -] - [[package]] name = "windows-sys" version = "0.45.0" diff --git a/helix-stdx/Cargo.toml b/helix-stdx/Cargo.toml index ed23f4e4ffd3..199dd06e4e30 100644 --- a/helix-stdx/Cargo.toml +++ b/helix-stdx/Cargo.toml @@ -17,6 +17,13 @@ etcetera = "0.8" ropey = { version = "1.6.1", default-features = false } which = "6.0" regex-cursor = "0.1.4" +bitflags = "2.4" + +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.52", features = ["Win32_Security", "Win32_Security_Authorization"] } + +[target.'cfg(unix)'.dependencies] +rustix = { version = "0.38", features = ["fs"] } [dev-dependencies] tempfile = "3.10" diff --git a/helix-view/src/faccess.rs b/helix-stdx/src/faccess.rs similarity index 95% rename from helix-view/src/faccess.rs rename to helix-stdx/src/faccess.rs index d83c4144db46..0270c1f8a163 100644 --- a/helix-view/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -50,20 +50,21 @@ mod imp { Ok(()) } - fn chown(p: &Path, uid: Option, gid: Option) -> anyhow::Result<()> { + 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 copy_metadata(from: &Path, to: &Path) -> anyhow::Result<()> { + 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); @@ -122,7 +123,7 @@ mod imp { } impl SecurityDescriptor { - fn for_path(p: &Path) -> std::io::Result { + fn for_path(p: &Path) -> io::Result { let path = std::fs::canonicalize(p)?; let pathos = path.into_os_string(); let mut pathw: Vec = Vec::with_capacity(pathos.len() + 1); @@ -350,7 +351,7 @@ mod imp { } } - fn chown(p: &Path, sd: SecurityDescriptor) -> std::io::Result<()> { + 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); @@ -399,7 +400,7 @@ mod imp { } } - pub fn copy_metadata(from: &Path, to: &Path) -> std::io::Result<()> { + pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { let sd = SecurityDescriptor::for_path(from)?; chown(to, sd)?; @@ -436,7 +437,7 @@ mod imp { } } - pub fn copy_metadata(from: &path, to: &Path) -> std::io::Result<()> { + 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)?; @@ -453,7 +454,6 @@ pub fn readonly(p: &Path) -> bool { } } -pub fn copy_metadata(from: &Path, to: &Path) -> anyhow::Result<()> { - imp::copy_metadata(from, to)?; - Ok(()) +pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { + imp::copy_metadata(from, to) } diff --git a/helix-stdx/src/lib.rs b/helix-stdx/src/lib.rs index 68fe3ec37702..19602c205d0c 100644 --- a/helix-stdx/src/lib.rs +++ b/helix-stdx/src/lib.rs @@ -1,3 +1,4 @@ pub mod env; +pub mod faccess; pub mod path; pub mod rope; diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 02deaa5132f8..f1875ec47edf 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -53,7 +53,6 @@ parking_lot = "0.12.1" [target.'cfg(windows)'.dependencies] clipboard-win = { version = "5.2", features = ["std"] } -windows-sys = { version = "0.52", features = ["Win32_Security", "Win32_Security_Authorization"] } [target.'cfg(unix)'.dependencies] libc = "0.2" diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 35ef2962fc4d..fd08ae0e6665 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -10,6 +10,7 @@ use helix_core::encoding::Encoding; use helix_core::syntax::{Highlight, LanguageServerFeature}; use helix_core::text_annotations::{InlineAnnotation, TextAnnotations}; use helix_lsp::util::lsp_pos_to_pos; +use helix_stdx::faccess::{copy_metadata, readonly}; use helix_vcs::{DiffHandle, DiffProviderRegistry}; use ::parking_lot::Mutex; @@ -37,7 +38,6 @@ use helix_core::{ use crate::editor::Config; use crate::events::{DocumentDidChange, SelectionDidChange}; -use crate::faccess::{copy_metadata, readonly}; use crate::{DocumentId, Editor, Theme, View, ViewId}; /// 8kB of buffer space for encoding and decoding `Rope`s. diff --git a/helix-view/src/lib.rs b/helix-view/src/lib.rs index ada5004c140c..14b6e1ce8138 100644 --- a/helix-view/src/lib.rs +++ b/helix-view/src/lib.rs @@ -2,9 +2,6 @@ pub mod macros; pub mod base64; - -mod faccess; - pub mod clipboard; pub mod document; pub mod editor;