diff --git a/src/app/automation.rs b/src/app/automation.rs index f3d6c3d08..f72c30487 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -212,7 +212,7 @@ impl OpenCADStudio { let Some(path) = req["path"].as_str() else { return err("open: missing \"path\""); }; - let bytes = match std::fs::read(path) { + let bytes = match self.read_drawing(std::path::Path::new(path)) { Ok(b) => b, Err(e) => return err(format!("open: {e}")), }; @@ -981,6 +981,42 @@ mod tests { let _ = std::fs::remove_file(&b); } + #[test] + fn saving_over_an_existing_drawing_succeeds() { + for (label, pre_existing) in [("new path", false), ("existing drawing", true)] { + let path = std::env::temp_dir().join(format!( + "ocs_save_over_{}_{}.dxf", + std::process::id(), + pre_existing, + )); + let _ = std::fs::remove_file(&path); + if pre_existing { + std::fs::write(&path, b"a previous drawing").unwrap(); + } + + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let p = path.to_string_lossy().replace('\\', "\\\\"); + let saved = app.automation_op(&format!(r#"{{"op":"save","path":"{p}"}}"#)); + assert_eq!(saved["ok"], true, "{label}: {}", saved["error"]); + let saved_again = app.automation_op(r#"{"op":"save"}"#); + assert_eq!( + saved_again["ok"], + true, + "normal save: {}", + saved_again["error"] + ); + + drop(app); + let sidecar = path.with_file_name(format!( + ".{}.ocs.lock", + path.file_name().unwrap().to_string_lossy() + )); + let _ = std::fs::remove_file(sidecar); + let _ = std::fs::remove_file(&path); + } + } + #[test] fn save_then_open_round_trips() { let mut app = OpenCADStudio::new_for_test(); diff --git a/src/app/update/file.rs b/src/app/update/file.rs index 7776ee7a8..7283db73d 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -711,6 +711,30 @@ pub(super) fn on_open_file(&mut self) -> Task { } } + /// Read through this session's lease when it covers `path`. + #[cfg(not(target_arch = "wasm32"))] + pub(in crate::app) fn read_drawing( + &self, + path: &std::path::Path, + ) -> std::io::Result> { + use std::io::{Read, Seek, SeekFrom}; + + let lease = self + .tab_showing(path) + .and_then(|i| self.tabs[i].edit_lease.as_ref()); + let leased = match lease { + Some(lease) => lease.reader()?, + None => None, + }; + let Some(mut reader) = leased else { + return std::fs::read(path); + }; + reader.seek(SeekFrom::Start(0))?; + let mut bytes = Vec::new(); + reader.read_to_end(&mut bytes)?; + Ok(bytes) + } + /// Start the next drawing a second launch handed us, if any. /// /// Must be called from EVERY path that clears `opening` — completion, error @@ -851,7 +875,7 @@ pub(super) fn on_open_file(&mut self) -> Task { )); } - let destination_lease = if path_changed { + let mut destination_lease = if path_changed { match crate::io::edit_lock::EditLease::acquire(&path) { Ok(lease) => Some(lease), Err(crate::io::edit_lock::EditLeaseError::Locked(error)) => { @@ -869,21 +893,17 @@ pub(super) fn on_open_file(&mut self) -> Task { }; let expected_fingerprint = if path_changed { - match crate::io::edit_lock::FileFingerprint::capture(&path) { - Ok(fingerprint) => Some(fingerprint), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, - Err(error) => { - return Err(crate::io::SaveFailure::other(format!( - "could not verify {} before saving: {error}", - path.display() - ))); - } - } + None } else { - self.tabs[i].disk_fingerprint.clone().or_else(|| { - crate::io::edit_lock::FileFingerprint::capture(&path).ok() - }) + self.tabs[i].disk_fingerprint.clone() }; + let lease = if path_changed { + destination_lease.as_mut() + } else { + self.tabs[i].edit_lease.as_mut() + }; + let (expected_fingerprint, verify_reader) = + Self::native_save_verification(&path, lease, expected_fingerprint)?; self.prepare_native_save(i); let version = self.tabs[i].scene.document.version; @@ -894,6 +914,7 @@ pub(super) fn on_open_file(&mut self) -> Task { version, self.backup_on_save, expected_fingerprint, + verify_reader, )?; if set_current_path { @@ -1380,6 +1401,57 @@ pub(super) fn on_open_file(&mut self) -> Task { self.sync_solid_models_to_acis(i); } + #[cfg(not(target_arch = "wasm32"))] + fn native_save_verification( + path: &std::path::Path, + mut lease: Option<&mut crate::io::edit_lock::EditLease>, + expected: Option, + ) -> Result< + ( + Option, + Option, + ), + crate::io::SaveFailure, + > { + let expected = match expected { + Some(expected) => Some(expected), + None => { + let captured = match lease.as_deref_mut() { + Some(lease) => match lease.fingerprint() { + Ok(Some(fingerprint)) => Ok(fingerprint), + Ok(None) => crate::io::edit_lock::FileFingerprint::capture(path), + Err(error) => Err(error), + }, + None => crate::io::edit_lock::FileFingerprint::capture(path), + }; + match captured { + Ok(fingerprint) => Some(fingerprint), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => { + return Err(crate::io::SaveFailure::other(format!( + "could not verify {} before saving: {error}", + path.display() + ))); + } + } + } + }; + let reader = if expected.is_some() { + match lease.as_deref() { + Some(lease) => lease.reader().map_err(|error| { + crate::io::SaveFailure::other(format!( + "could not verify {} before saving: {error}", + path.display() + )) + })?, + None => None, + } + } else { + None + }; + Ok((expected, reader)) + } + #[cfg(target_arch = "wasm32")] fn stamp_thumbnail(&mut self, i: usize, version: acadrust::DxfVersion) { let scene = &self.tabs[i].scene; @@ -1532,16 +1604,58 @@ pub(super) fn on_open_file(&mut self) -> Task { let previous_autosave = (purpose != crate::app::SavePurpose::Autosave).then(|| self.autosave_target(i)); let backup = purpose != crate::app::SavePurpose::Autosave && self.backup_on_save; - let expected_fingerprint = - if check_external_change && purpose != crate::app::SavePurpose::Autosave { - if set_current_path { - crate::io::edit_lock::FileFingerprint::capture(&path).ok() - } else { - self.tabs[i].disk_fingerprint.clone() - } - } else { + let verification = if check_external_change + && purpose != crate::app::SavePurpose::Autosave + { + let expected = if set_current_path { None + } else { + self.tabs[i].disk_fingerprint.clone() }; + if self.pending_save_leases.contains_key(&tab_id) { + Self::native_save_verification( + &path, + self.pending_save_leases.get_mut(&tab_id), + expected, + ) + } else if destination_is_current { + Self::native_save_verification( + &path, + self.tabs[i].edit_lease.as_mut(), + expected, + ) + } else { + Self::native_save_verification(&path, None, expected) + } + } else { + Ok((None, None)) + }; + let (expected_fingerprint, verify_reader) = match verification { + Ok(verification) => verification, + Err(error) => { + return Task::perform( + async move { + crate::app::SaveOutcome { + job_id, + tab_id, + epoch, + revision, + camera_generation, + path, + version, + previous_autosave, + set_current_path, + purpose, + continuation, + thumbnail_key, + refreshed_preview: None, + result: Err(error), + } + }, + Message::SaveFinished, + ); + } + }; let worker_path = path.clone(); Task::perform( @@ -1572,6 +1686,7 @@ pub(super) fn on_open_file(&mut self) -> Task { version, backup, expected_fingerprint, + verify_reader, ); (result, refreshed_preview) }) diff --git a/src/io/edit_lock.rs b/src/io/edit_lock.rs index 0065cbebe..cbe79fcae 100644 --- a/src/io/edit_lock.rs +++ b/src/io/edit_lock.rs @@ -22,7 +22,11 @@ pub struct FileFingerprint { impl FileFingerprint { pub fn capture(path: &Path) -> std::io::Result { - let mut file = File::open(path)?; + Self::capture_from(&mut File::open(path)?) + } + + /// Fingerprint an already-open drawing handle. + pub fn capture_from(file: &mut File) -> std::io::Result { let metadata = file.metadata()?; let len = metadata.len(); let modified_ns = metadata @@ -66,6 +70,52 @@ impl FileFingerprint { } } +#[cfg(unix)] +pub(crate) fn path_matches_file(path: &Path, file: &File) -> std::io::Result { + use std::os::unix::fs::MetadataExt; + + let path_metadata = std::fs::metadata(path)?; + let file_metadata = file.metadata()?; + Ok(path_metadata.dev() == file_metadata.dev() && path_metadata.ino() == file_metadata.ino()) +} + +#[cfg(target_os = "windows")] +pub(crate) fn path_matches_file(path: &Path, file: &File) -> std::io::Result { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Storage::FileSystem::{ + FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, + }; + + let path_file = OpenOptions::new() + .access_mode(0) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) + .open(path)?; + Ok(windows_file_identity(&path_file)? == windows_file_identity(file)?) +} + +#[cfg(target_os = "windows")] +fn windows_file_identity(file: &File) -> std::io::Result<(u32, u64)> { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Storage::FileSystem::{ + BY_HANDLE_FILE_INFORMATION, GetFileInformationByHandle, + }; + + let mut info = BY_HANDLE_FILE_INFORMATION::default(); + if unsafe { GetFileInformationByHandle(file.as_raw_handle() as _, &mut info) } == 0 { + return Err(std::io::Error::last_os_error()); + } + let index = (u64::from(info.nFileIndexHigh) << 32) | u64::from(info.nFileIndexLow); + Ok((info.dwVolumeSerialNumber, index)) +} + +#[cfg(not(any(unix, target_os = "windows")))] +pub(crate) fn path_matches_file(path: &Path, file: &File) -> std::io::Result { + let mut path_file = File::open(path)?; + let mut leased_file = file.try_clone()?; + Ok(FileFingerprint::capture_from(&mut path_file)? + == FileFingerprint::capture_from(&mut leased_file)?) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum EditLeaseError { Locked(String), @@ -153,6 +203,19 @@ impl EditLease { self.platform_warning.as_deref() } + /// Fingerprint through the lease's drawing handle. + pub fn fingerprint(&mut self) -> std::io::Result> { + self.drawing + .as_mut() + .map(FileFingerprint::capture_from) + .transpose() + } + + /// Clone the leased drawing handle for a worker. + pub fn reader(&self) -> std::io::Result> { + self.drawing.as_ref().map(File::try_clone).transpose() + } + /// Atomic save replaces the path with a new file object. Move the platform /// lock to that new object; the sidecar remains locked throughout. pub fn refresh_drawing_lock(&mut self, path: &Path) -> Result<(), EditLeaseError> { @@ -255,6 +318,66 @@ mod tests { let _ = std::fs::remove_file(path); } + #[test] + fn a_lease_fingerprints_the_drawing_it_has_locked() { + let path = unique_path("leased.dwg"); + std::fs::write(&path, b"drawing bytes").unwrap(); + let unlocked = FileFingerprint::capture(&path).unwrap(); + + let mut lease = EditLease::acquire(&path).unwrap(); + let leased = lease + .fingerprint() + .expect("the lease handle is readable") + .expect("the lease holds a drawing handle"); + assert_eq!(unlocked, leased); + + drop(lease); + let _ = std::fs::remove_file(sidecar_path(&path)); + let _ = std::fs::remove_file(path); + } + + #[test] + fn a_lease_lends_a_reader_for_the_drawing_it_has_locked() { + let path = unique_path("leased_reader.dwg"); + std::fs::write(&path, b"drawing bytes").unwrap(); + + let lease = EditLease::acquire(&path).unwrap(); + let mut reader = lease + .reader() + .expect("the lease handle is cloneable") + .expect("the lease holds a drawing handle"); + let mut bytes = Vec::new(); + reader.read_to_end(&mut bytes).unwrap(); + assert_eq!(b"drawing bytes".as_slice(), bytes.as_slice()); + + drop(reader); + drop(lease); + let _ = std::fs::remove_file(sidecar_path(&path)); + let _ = std::fs::remove_file(path); + } + + #[test] + fn a_replaced_path_no_longer_matches_the_lease() { + let path = unique_path("identity.dwg"); + let replacement = unique_path("replacement.dwg"); + std::fs::write(&path, b"old").unwrap(); + std::fs::write(&replacement, b"new").unwrap(); + + let lease = EditLease::acquire(&path).unwrap(); + let reader = lease + .reader() + .unwrap() + .expect("the lease holds a drawing handle"); + std::fs::remove_file(&path).unwrap(); + std::fs::rename(&replacement, &path).unwrap(); + assert!(!path_matches_file(&path, &reader).unwrap()); + + drop(reader); + drop(lease); + let _ = std::fs::remove_file(sidecar_path(&path)); + let _ = std::fs::remove_file(path); + } + #[test] fn fingerprint_detects_same_size_content_change() { let path = unique_path("fingerprint.dwg"); diff --git a/src/io/mod.rs b/src/io/mod.rs index bd166d7ad..7edef2813 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -1437,6 +1437,41 @@ mod save_failure_tests { acadrust::DxfVersion::AC1032, false, Some(expected), + None, + ) + .unwrap_err(); + + assert!(error.externally_modified); + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + let _ = std::fs::remove_file(path); + } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn external_path_replacement_prevents_atomic_replace() { + let path = std::env::temp_dir().join(format!( + "ocs_external_replace_{}_{}.dwg", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let replacement = path.with_extension("replacement.dwg"); + std::fs::write(&path, b"old").unwrap(); + let expected = super::edit_lock::FileFingerprint::capture(&path).unwrap(); + let reader = std::fs::File::open(&path).unwrap(); + std::fs::write(&replacement, b"new").unwrap(); + std::fs::remove_file(&path).unwrap(); + std::fs::rename(&replacement, &path).unwrap(); + + let error = super::save_owned_as_version_atomic( + acadrust::CadDocument::new(), + &path, + acadrust::DxfVersion::AC1032, + false, + Some(expected), + Some(reader), ) .unwrap_err(); @@ -1509,12 +1544,20 @@ pub fn save_owned_as_version_atomic( version: acadrust::DxfVersion, backup: bool, expected_fingerprint: Option, + verify_reader: Option, ) -> Result<(), SaveFailure> { save_owned_as_version_inner(doc, path, version, backup, 0.0, move |path| { let Some(expected) = expected_fingerprint else { return Ok(()); }; - match edit_lock::FileFingerprint::capture(path) { + let current = match verify_reader { + Some(mut file) => match edit_lock::path_matches_file(path, &file) { + Ok(true) => edit_lock::FileFingerprint::capture_from(&mut file), + Ok(false) | Err(_) => return Err(SaveFailure::externally_modified(path)), + }, + None => edit_lock::FileFingerprint::capture(path), + }; + match current { Ok(current) if current == expected => Ok(()), _ => Err(SaveFailure::externally_modified(path)), }