From e5372b04a15701d1f2d6f06ceb52be6befe7d27f Mon Sep 17 00:00:00 2001 From: Kirawi <67773714+kirawi@users.noreply.github.com> Date: Sat, 27 Jul 2024 13:21:52 -0400 Subject: [PATCH] Fix writing hardlinks (#11340) * don't use backup files with hardlinks * check if the inodes remain the same in the test * move funcs to faccess and use AsRawHandle * use a copy as a backup for hardlinks * delete backup after copy --- Cargo.lock | 1 + helix-stdx/src/faccess.rs | 27 +++++++++++++-- helix-term/Cargo.toml | 1 + helix-term/tests/test/commands/write.rs | 34 +++++++++++++++++++ helix-view/src/document.rs | 45 ++++++++++++++++++++----- 5 files changed, 96 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 263ca7d7e..c54e481e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1455,6 +1455,7 @@ dependencies = [ "once_cell", "open", "pulldown-cmark", + "same-file", "serde", "serde_json", "signal-hook", diff --git a/helix-stdx/src/faccess.rs b/helix-stdx/src/faccess.rs index 0270c1f8a..c69571b68 100644 --- a/helix-stdx/src/faccess.rs +++ b/helix-stdx/src/faccess.rs @@ -74,6 +74,11 @@ mod imp { Ok(()) } + + pub fn hardlink_count(p: &Path) -> std::io::Result { + let metadata = p.metadata()?; + Ok(metadata.nlink()) + } } // Licensed under MIT from faccess except for `chown`, `copy_metadata` and `is_acl_inherited` @@ -94,8 +99,8 @@ mod imp { SID_IDENTIFIER_AUTHORITY, TOKEN_DUPLICATE, TOKEN_QUERY, }; use windows_sys::Win32::Storage::FileSystem::{ - FILE_ACCESS_RIGHTS, FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, - FILE_GENERIC_WRITE, + GetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION, FILE_ACCESS_RIGHTS, + FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE, }; use windows_sys::Win32::System::Threading::{GetCurrentThread, OpenThreadToken}; @@ -103,7 +108,7 @@ mod imp { use std::ffi::c_void; - use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt}; + use std::os::windows::{ffi::OsStrExt, fs::OpenOptionsExt, io::AsRawHandle}; struct SecurityDescriptor { sd: PSECURITY_DESCRIPTOR, @@ -411,6 +416,18 @@ mod imp { Ok(()) } + + pub fn hardlink_count(p: &Path) -> std::io::Result { + let file = std::fs::File::open(p)?; + let handle = file.as_raw_handle() as isize; + let mut info: BY_HANDLE_FILE_INFORMATION = unsafe { std::mem::zeroed() }; + + if unsafe { GetFileInformationByHandle(handle, &mut info) } == 0 { + Err(std::io::Error::last_os_error()) + } else { + Ok(info.nNumberOfLinks as u64) + } + } } // Licensed under MIT from faccess except for `copy_metadata` @@ -457,3 +474,7 @@ pub fn readonly(p: &Path) -> bool { pub fn copy_metadata(from: &Path, to: &Path) -> io::Result<()> { imp::copy_metadata(from, to) } + +pub fn hardlink_count(p: &Path) -> io::Result { + imp::hardlink_count(p) +} diff --git a/helix-term/Cargo.toml b/helix-term/Cargo.toml index cc90396e4..4887b0a27 100644 --- a/helix-term/Cargo.toml +++ b/helix-term/Cargo.toml @@ -86,3 +86,4 @@ helix-loader = { path = "../helix-loader" } smallvec = "1.13" indoc = "2.0.5" tempfile = "3.10.1" +same-file = "1.0.1" diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index 4db98a046..01824f171 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -649,6 +649,40 @@ async fn test_symlink_write_relative() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_hardlink_write() -> anyhow::Result<()> { + let dir = tempfile::tempdir()?; + + let mut file = tempfile::NamedTempFile::new_in(&dir)?; + let hardlink_path = dir.path().join("linked"); + std::fs::hard_link(file.path(), &hardlink_path)?; + + let mut app = helpers::AppBuilder::new() + .with_file(&hardlink_path, None) + .build()?; + + test_key_sequence( + &mut app, + Some("ithe gostak distims the doshes:w"), + None, + false, + ) + .await?; + + reload_file(&mut file).unwrap(); + let mut file_content = String::new(); + file.as_file_mut().read_to_string(&mut file_content)?; + + assert_eq!( + LineFeedHandling::Native.apply("the gostak distims the doshes"), + file_content + ); + assert!(helix_stdx::faccess::hardlink_count(&hardlink_path)? > 1); + assert!(same_file::is_same_file(file.path(), &hardlink_path)?); + + Ok(()) +} + async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 346f04ed8..885f33e8a 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -934,6 +934,9 @@ impl Document { "Path is read only" )); } + + // Assume it is a hardlink to prevent data loss if the metadata cant be read (e.g. on certain Windows configurations) + let is_hardlink = helix_stdx::faccess::hardlink_count(&write_path).unwrap_or(2) > 1; let backup = if path.exists() { let path_ = write_path.clone(); // hacks: we use tempfile to handle the complex task of creating @@ -942,14 +945,22 @@ impl Document { // 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() + 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() + }; + + backup_path.keep().ok() }) .await .ok() @@ -972,7 +983,23 @@ impl Document { }; if let Some(backup) = backup { - if write_result.is_err() { + if is_hardlink { + let mut delete = true; + if write_result.is_err() { + // Restore backup + let _ = tokio::fs::copy(&backup, &write_path).await.map_err(|e| { + delete = false; + log::error!("Failed to restore backup on write failure: {e}") + }); + } + + if delete { + // Delete backup + let _ = tokio::fs::remove_file(backup) + .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) .await