diff --git a/src/autosave.rs b/src/autosave.rs index cfbb2db..56f4c8f 100644 --- a/src/autosave.rs +++ b/src/autosave.rs @@ -228,6 +228,8 @@ pub struct AutosaveController { pending_pages: HashMap, pending_manifest: Option, retries: HashMap, + /// Backoff of the manifest after a failed write + manifest_retry: Option, last_errors: Vec, status: AutosaveStatus, in_flight_count: usize, @@ -281,6 +283,7 @@ impl AutosaveController { pending_pages: HashMap::new(), pending_manifest: None, retries: HashMap::new(), + manifest_retry: None, last_errors: Vec::new(), status: AutosaveStatus::Saved, in_flight_count: 0, @@ -428,16 +431,25 @@ impl AutosaveController { retry.backoff = (retry.backoff * 2).min(Duration::from_secs(30)); } }, - AutosaveResponse::ManifestSaved { result } => { - if let Err((err_msg, failed_manifest)) = result { + AutosaveResponse::ManifestSaved { result } => match result { + Ok(()) => self.manifest_retry = None, + Err((err_msg, failed_manifest)) => { eprintln!("[Autosave-Fehler] Manifest fehlgeschlagen: {err_msg}"); self.status = AutosaveStatus::Error; self.last_errors.push(t!("Manifest: {msg}", msg = err_msg)); if self.pending_manifest.is_none() { self.pending_manifest = Some(*failed_manifest); } + // Dispatching reset the debounce timers, so without a retry time of + // its own the manifest would wait for the next edit + let retry = self.manifest_retry.get_or_insert(PageRetry { + next_retry: now, + backoff: Duration::from_secs(1), + }); + retry.next_retry = now + retry.backoff; + retry.backoff = (retry.backoff * 2).min(Duration::from_secs(30)); } - } + }, } if self.in_flight_count == 0 @@ -468,7 +480,12 @@ impl AutosaveController { .get(id) .map(|r| now >= r.next_retry) .unwrap_or(false) - }); + }) + || (self.pending_manifest.is_some() + && self + .manifest_retry + .as_ref() + .is_some_and(|r| now >= r.next_retry)); if max_wait_hit || debounce_hit || retry_ready { self.dispatch_pending(); @@ -515,7 +532,11 @@ impl AutosaveController { } } - if let Some(manifest) = self.pending_manifest.take() { + let manifest_ready = self + .manifest_retry + .as_ref() + .is_none_or(|r| now >= r.next_retry); + if let Some(manifest) = self.pending_manifest.take_if(|_| manifest_ready) { let cmd = AutosaveCommand::SaveManifest { notebook_dir: self.notebook_dir.clone(), manifest: manifest.clone(), @@ -546,6 +567,7 @@ impl AutosaveController { } // Backoff für den Flush ignorieren, damit sofort geschrieben wird self.retries.clear(); + self.manifest_retry = None; if !self.pending_pages.is_empty() || self.pending_manifest.is_some() { self.dispatch_pending(); @@ -650,6 +672,7 @@ impl AutosaveController { self.pending_pages.clear(); self.pending_manifest = None; self.retries.clear(); + self.manifest_retry = None; self.last_errors.clear(); self.first_change_time = None; self.last_change_time = None; @@ -772,6 +795,7 @@ pub mod tests { /// Fake-PageStore zum Simulieren von I/O-Fehlern und Panics. pub struct FakePageStore { pub fail_page_writes: Mutex, + pub fail_manifest_writes: Mutex, pub panic_on_page: Mutex, pub page_writes: Arc, pub manifest_writes: Arc, @@ -781,6 +805,7 @@ pub mod tests { pub fn new() -> Self { Self { fail_page_writes: Mutex::new(0), + fail_manifest_writes: Mutex::new(0), panic_on_page: Mutex::new(false), page_writes: Arc::new(AtomicUsize::new(0)), manifest_writes: Arc::new(AtomicUsize::new(0)), @@ -815,6 +840,13 @@ pub mod tests { _dir: &Path, _manifest: &NotebookManifest, ) -> Result<(), StorageError> { + let mut fails = self.fail_manifest_writes.lock().unwrap(); + if *fails > 0 { + *fails -= 1; + return Err(StorageError::InvalidManifest( + "Simulierter Store-Fehler".to_string(), + )); + } self.manifest_writes.fetch_add(1, Ordering::SeqCst); Ok(()) } @@ -852,6 +884,40 @@ pub mod tests { assert_eq!(controller.status(), AutosaveStatus::Saved); } + #[test] + fn test_failed_manifest_is_retried_without_another_edit() { + let clock = Arc::new(MockClock::new(Instant::now())); + let store = Arc::new(FakePageStore::new()); + *store.fail_manifest_writes.lock().unwrap() = 1; + let mut controller = AutosaveController::with_options( + PathBuf::from("/tmp/manifest_retry"), + Duration::from_millis(100), + Duration::from_secs(10), + Arc::clone(&store) as Arc, + Arc::clone(&clock) as Arc, + ); + let manifest = NotebookManifest::new("nb", "Retry"); + controller.mark_manifest_dirty(&manifest); + + clock.advance(Duration::from_millis(200)); + controller.update(); + controller.sync_worker(); + assert_eq!(controller.update(), AutosaveStatus::Error); + assert_eq!(store.manifest_writes.load(Ordering::SeqCst), 0); + + // Not before the backoff has passed + controller.update(); + controller.sync_worker(); + controller.update(); + assert_eq!(store.manifest_writes.load(Ordering::SeqCst), 0); + + clock.advance(Duration::from_secs(2)); + controller.update(); + controller.sync_worker(); + assert_eq!(controller.update(), AutosaveStatus::Saved); + assert_eq!(store.manifest_writes.load(Ordering::SeqCst), 1); + } + #[test] fn test_debounce_bundles_changes() { let clock = Arc::new(MockClock::new(Instant::now())); diff --git a/src/i18n/en.rs b/src/i18n/en.rs index f7be475..f7bd556 100644 --- a/src/i18n/en.rs +++ b/src/i18n/en.rs @@ -133,6 +133,10 @@ pub const EN: &[(&str, &str)] = &[ ("Fehler beim Schreiben auf die Festplatte: {e}", "Error writing to disk: {e}"), ("Fehler beim Umbenennen: {err}", "Error while renaming: {err}"), ("Fehler beim Wiederherstellen: {err}", "Error while restoring: {err}"), + ( + "Das PDF kann nicht verarbeitet werden (interner Fehler)", + "The PDF cannot be processed (internal error)", + ), ("Fehler beim Öffnen: {err}", "Error while opening: {err}"), ("Fehler: {err}", "Error: {err}"), ("Fehlermeldung: {message}", "Error message: {message}"), diff --git a/src/import.rs b/src/import.rs index 66bb1e3..fae4804 100644 --- a/src/import.rs +++ b/src/import.rs @@ -125,11 +125,24 @@ pub fn page( /// we draw a diameter of `width · (0.5 + p)`, so the widest point gets p = 1 and thinner /// ones less (down to a third of the widest). pub fn fit_widths(widths: &[f32]) -> (f32, Vec) { - let widest = widths.iter().copied().fold(0.0, f32::max).max(0.01); + let widest = widths + .iter() + .copied() + .filter(|w| w.is_finite()) + .fold(0.0, f32::max) + .max(0.01); let width = widest / 1.5; let pressures = widths .iter() - .map(|w| (w / width - 0.5).clamp(0.0, 1.0)) + .map(|w| { + let p = w / width - 0.5; + // NaN survives `clamp`, is stored as `null` and makes the page unreadable + if p.is_finite() { + p.clamp(0.0, 1.0) + } else { + 0.5 + } + }) .collect(); (width, pressures) } @@ -153,17 +166,29 @@ pub fn ink_stroke( color[3] = 1.0; StrokeTool::Pen }; + // Broken files may carry NaN or infinite numbers. They would be written as `null` and + // the page could not be read again, so such points are left out. let samples = points .iter() .zip(pressures) + .filter(|(p, _)| p.is_finite()) .enumerate() .map(|(i, (p, &pressure))| SampleData { x: p.x, y: p.y, - p: pressure, + p: if pressure.is_finite() { + pressure.clamp(0.0, 1.0) + } else { + 0.5 + }, t: i as f64 * 0.005, }) .collect(); + let width = if width.is_finite() && width > 0.0 { + width + } else { + 5.0 + }; StrokeData { id, color, @@ -246,7 +271,12 @@ pub fn unpremultiply(pixels: &mut [u8]) { /// Rasterizes an SVG for a box of `size` page units (about 250 dpi). pub fn rasterize_svg(svg: &str, size: glam::Vec2) -> Option<(u32, u32, Vec)> { use resvg::{tiny_skia, usvg}; - let tree = usvg::Tree::from_str(svg, &usvg::Options::default()).ok()?; + // By default usvg loads `` from disk. An imported note + // must not be able to pull files of this device into the notebook; embedded data URLs + // still work. + let mut options = usvg::Options::default(); + options.image_href_resolver.resolve_string = Box::new(|_, _| None); + let tree = usvg::Tree::from_str(svg, &options).ok()?; let max = crate::assets::MAX_IMPORT_DIMENSION as f32; let px_per_unit = 250.0 / 254.0; let width = (size.x * px_per_unit).round().clamp(1.0, max) as u32; @@ -288,4 +318,44 @@ mod tests { // Thinner than a third of the widest point is not possible assert!((drawn[3] - 10.0).abs() < 1e-3); } + + #[test] + fn test_broken_numbers_never_reach_the_page_file() { + let (width, pressures) = fit_widths(&[f32::NAN, 10.0, f32::INFINITY]); + assert!(width.is_finite() && pressures.iter().all(|p| p.is_finite())); + + let points = [ + glam::Vec2::new(10.0, 10.0), + glam::Vec2::new(f32::NAN, 20.0), + glam::Vec2::new(30.0, 30.0), + ]; + let stroke = ink_stroke( + 1, + [0.0, 0.0, 0.0, 1.0], + false, + &points, + f32::NAN, + &[f32::NAN, 0.5, 2.0], + ); + assert_eq!(stroke.samples.len(), 2); + assert!(stroke.width.is_finite()); + let json = serde_json::to_string(&stroke).unwrap(); + assert!(!json.contains("null"), "{json}"); + assert!(serde_json::from_str::(&json).is_ok()); + } + + #[test] + fn test_svg_cannot_embed_local_files() { + let dir = tempfile::tempdir().unwrap(); + let secret = dir.path().join("secret.png"); + image::RgbaImage::from_pixel(8, 8, image::Rgba([255, 0, 0, 255])) + .save(&secret) + .unwrap(); + let svg = format!( + r#""#, + secret.display() + ); + let (_, _, pixels) = rasterize_svg(&svg, glam::Vec2::new(80.0, 80.0)).unwrap(); + assert!(pixels.iter().all(|&b| b == 0), "the local image was drawn"); + } } diff --git a/src/pdf_doc.rs b/src/pdf_doc.rs index c4a2341..1a580b9 100644 --- a/src/pdf_doc.rs +++ b/src/pdf_doc.rs @@ -40,8 +40,12 @@ pub const MAX_IMPORT_PAGES: usize = 2000; /// Öffnet ein PDF und liefert die angezeigte Größe jeder Seite in Punkt (Drehung /// berücksichtigt). pub fn page_sizes(bytes: Vec) -> Result, String> { - let pdf = open(bytes)?; - let sizes: Vec<(f32, f32)> = pdf.pages().iter().map(|p| p.render_dimensions()).collect(); + // Runs on the main thread during import: a panic in the PDF library must not take the + // app down + let sizes: Vec<(f32, f32)> = guarded(|| { + let pdf = open(bytes)?; + Ok(pdf.pages().iter().map(|p| p.render_dimensions()).collect()) + })?; if sizes.is_empty() { return Err(t!("Das PDF enthält keine Seiten").into()); } @@ -55,6 +59,13 @@ pub fn page_sizes(bytes: Vec) -> Result, String> { Ok(sizes) } +/// Runs PDF code and turns a panic of the PDF library (malformed files) into an error. +fn guarded(work: impl FnOnce() -> Result) -> Result { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(work)).unwrap_or_else(|_| { + Err(t!("Das PDF kann nicht verarbeitet werden (interner Fehler)").to_string()) + }) +} + fn open(bytes: Vec) -> Result { Pdf::new(bytes).map_err(|e| match e { LoadPdfError::Decryption(_) => { @@ -209,21 +220,31 @@ impl PdfRenderService { queue = ready.wait(queue).unwrap_or_else(|e| e.into_inner()); } }; + // A panic here would end the thread and with it every PDF page until + // the next start, so it only fails the page at hand if open_pdf.as_ref().map(|(p, _)| p) != Some(&request.path) { - open_pdf = std::fs::read(&request.path) - .map_err(|e| e.to_string()) - .and_then(open) - .map_err(|e| eprintln!("[PDF] '{}': {e}", request.path.display())) - .ok() - .map(|pdf| (request.path.clone(), pdf)); + open_pdf = guarded(|| { + std::fs::read(&request.path) + .map_err(|e| e.to_string()) + .and_then(open) + }) + .map_err(|e| eprintln!("[PDF] '{}': {e}", request.path.display())) + .ok() + .map(|pdf| (request.path.clone(), pdf)); } let result = match &open_pdf { // Mipmaps hier statt im Haupt-Thread: sonst stockt das Scrollen bei // jeder neu gerenderten Seite (Issue #28) - Some((_, pdf)) => render_page(pdf, request.page, request.max_pixels) - .map(crate::image_layer::mip_chain), + Some((_, pdf)) => guarded(|| { + render_page(pdf, request.page, request.max_pixels) + .map(crate::image_layer::mip_chain) + }), None => Err(t!("PDF kann nicht geöffnet werden").into()), }; + if result.is_err() { + // The document may be in a broken state after a panic + open_pdf = None; + } if res_tx.send((request.key, result)).is_err() { return; } @@ -356,6 +377,13 @@ impl Drop for PdfRenderService { pub(crate) mod tests { use super::*; + #[test] + fn test_panic_in_pdf_code_becomes_an_error() { + let result: Result<(), String> = guarded(|| panic!("simulated panic in the PDF library")); + assert!(result.is_err()); + assert_eq!(guarded(|| Ok(7)), Ok(7)); + } + /// Minimales gültiges PDF mit Seiten der angegebenen Größen (Punkt); jede Seite zeigt ein /// rotes Rechteck in der linken oberen Ecke. Optional mit `/Rotate`. pub(crate) fn sample_pdf(pages: &[(f32, f32, i32)]) -> Vec { diff --git a/src/rnote.rs b/src/rnote.rs index b5e29e4..878dc49 100644 --- a/src/rnote.rs +++ b/src/rnote.rs @@ -561,7 +561,8 @@ fn convert(doc: &RnoteDocument, notebook_dir: &Path) -> Imported { let scale = scale_pt * (72.0 / doc.dpi) as f32; // Rnote grows documents page by page; tolerate rounding in the stored size let count = |length: f64, page: f64| ((length / page - 0.01).ceil() as usize).max(1); - let columns = count(doc.size.x, doc.format.x); + // A broken document size must not ask for millions of pages + let columns = count(doc.size.x, doc.format.x).min(MAX_PAGES); let rows = count(doc.size.y, doc.format.y); let rows = rows.min((MAX_PAGES / columns).max(1)); let targets = (0..rows * columns) @@ -889,6 +890,22 @@ mod tests { assert!(second.strokes[1].is_image()); } + #[test] + fn test_broken_document_size_is_capped() { + let dir = tempfile::tempdir().unwrap(); + let doc = RnoteDocument { + format: DVec2::new(800.0, 1100.0), + dpi: 96.0, + origin: DVec2::ZERO, + size: DVec2::new(1e18, 1e18), + endless: false, + background: PageBackground::Blank, + items: Vec::new(), + }; + let imported = convert(&doc, dir.path()); + assert!(imported.pages.len() <= MAX_PAGES); + } + #[test] fn test_infinite_documents_become_endless() { let temp_dir = tempfile::tempdir().unwrap(); diff --git a/src/storage.rs b/src/storage.rs index 27e4fc9..951be91 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -555,8 +555,12 @@ pub fn atomic_write_file(path: &Path, content: &[u8]) -> Result<(), io::Error> { use std::os::unix::fs::PermissionsExt; let _ = fs::set_permissions(&temp_path, fs::Permissions::from_mode(0o600)); } - file.write_all(content)?; - file.sync_all()?; + // On a full disk every retry would otherwise leave another partial file behind + if let Err(e) = file.write_all(content).and_then(|_| file.sync_all()) { + drop(file); + let _ = fs::remove_file(&temp_path); + return Err(e); + } } // Atomares Rename