diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index 082d1910d1..6e5891d1a4 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -24,10 +24,10 @@ pub(crate) use reader::find; #[cfg(test)] use reader::rfind; pub(crate) use state::ObjectReader; -use state::{ObjStm, Slot}; -pub use state::{ReadState, SourceIdentity}; +use state::{Loaded, ObjStm, Slot}; #[cfg(test)] -use state::{SECTION_READS, objstm_member, owned_bytes}; +use state::{OBJECT_SEARCHES, SECTION_READS, objstm_member, owned_bytes}; +pub use state::{ReadState, SourceIdentity}; use xref::XrefIndex; pub use xref::{Revision, XrefEntry}; @@ -47,7 +47,9 @@ pub struct Document { /// `fill_xref_gaps`. located: Arc>, repair_log: Arc>, - cache: Arc>>>, + /// What loading each file object gave, a failure as well: a damaged object is otherwise + /// searched for across the whole file on every read (`Document::load`). + cache: Arc>>, objstms: Arc>>>, /// What `/JBIG2Globals` streams of the file decode to, by object number: the images of a scan /// share them, and each would otherwise decode them again (see `Document::jbig2_globals`). diff --git a/crates/cos/src/document/objects.rs b/crates/cos/src/document/objects.rs index 2ac18cff03..2ca327de74 100644 --- a/crates/cos/src/document/objects.rs +++ b/crates/cos/src/document/objects.rs @@ -1,5 +1,5 @@ use super::reader::find; -use super::state::{LENGTHS, LoadGuard, ObjStm, ObjectReader, ReadState, Slot, SourceIdentity, objstm_member, owned_bytes}; +use super::state::{LENGTHS, LoadGuard, Loaded, ObjStm, ObjectReader, ReadState, Slot, SourceIdentity, loaded_bytes, objstm_member}; use super::xref::{Revision, XrefEntry}; use super::{ Arc, CosError, Cow, Dict, Document, Lexer, MAX_DECODED, MAX_JBIG2_GLOBALS, ObjRef, Object, Stream, is_whitespace, parse_indirect_shared, @@ -388,20 +388,23 @@ impl Document { Some(Slot::Freed(_)) => return Ok(Arc::new(Object::Null)), None => {} } - if let Some(o) = self.cache.lock().map_err(|_| CosError::Poisoned)?.get(&num) { - return Ok(o.clone()); + if let Some(loaded) = self.cache.lock().map_err(|_| CosError::Poisoned)?.get(&num) { + return loaded.clone(); } // Loading can re-enter (indirect stream `/Length`, object streams); damaged files make // cycles such as `6 0 obj << /Length 6 0 R >>`. Bound the nesting on every path. let _guard = LoadGuard::enter(num).ok_or_else(|| CosError::Syntax { offset: 0, detail: format!("reference cycle while loading object {num}") })?; - let obj = self.load(num, 0)?; - let obj = Arc::new(obj); - // The editing cache is bounded like the scan reader's: re-parsing a file-backed view - // is cheap, and returned objects outlive eviction. - self.cache.lock().map_err(|_| CosError::Poisoned)?.insert(num, obj.clone()); - self.evict_editor_cache_if_over_budget(num, &obj)?; - Ok(obj) + let loaded = self.load(num, 0).map(Arc::new); + // A failure is kept like an object, or every read of a damaged object searches the whole + // file again. A poisoned lock says nothing about the object. + if !matches!(loaded, Err(CosError::Poisoned)) { + // The editing cache is bounded like the scan reader's: re-parsing a file-backed view + // is cheap, and returned objects outlive eviction. + self.cache.lock().map_err(|_| CosError::Poisoned)?.insert(num, loaded.clone()); + self.evict_editor_cache_if_over_budget(num, &loaded)?; + } + loaded } /// Bounds for the editing cache: roomier than the scan reader's (128 objects / 8 MB) @@ -430,17 +433,17 @@ impl Document { /// byte budget can double. const EDITOR_LARGE_OBJECT: usize = 256 * 1024; - fn evict_editor_cache_if_over_budget(&self, fresh_num: u32, fresh: &Object) -> Result<(), CosError> { + fn evict_editor_cache_if_over_budget(&self, fresh_num: u32, fresh: &Loaded) -> Result<(), CosError> { // Fast path: the full fold is O(cache) per insert, so skip it for a small newcomer // below the count cap. `len` is copied out first; nothing here holds the lock. let len = self.cache.lock().map_err(|_| CosError::Poisoned)?.len(); - if len <= Self::EDITOR_CACHE_OBJECTS && owned_bytes(fresh, &self.data, 0) <= Self::EDITOR_LARGE_OBJECT { + if len <= Self::EDITOR_CACHE_OBJECTS && loaded_bytes(fresh, &self.data) <= Self::EDITOR_LARGE_OBJECT { return Ok(()); } // Walk a snapshot off-lock; re-lock only to evict. - let live: Vec<(u32, Arc)> = self.cache.lock().map_err(|_| CosError::Poisoned)?.iter().map(|(n, o)| (*n, o.clone())).collect(); + let live: Vec<(u32, Loaded)> = self.cache.lock().map_err(|_| CosError::Poisoned)?.iter().map(|(n, o)| (*n, o.clone())).collect(); let data = &self.data; - let owned: usize = live.iter().fold(0usize, |n, (_, o)| n.saturating_add(owned_bytes(o, data, 0))); + let owned: usize = live.iter().fold(0usize, |n, (_, o)| n.saturating_add(loaded_bytes(o, data))); if live.len() <= Self::EDITOR_CACHE_OBJECTS && owned <= Self::EDITOR_CACHE_BYTES { return Ok(()); } @@ -450,7 +453,7 @@ impl Document { let mut cache = self.cache.lock().map_err(|_| CosError::Poisoned)?; if cache.len() <= Self::EDITOR_CACHE_OBJECTS { let data = &self.data; - let owned: usize = cache.values().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, data, 0))); + let owned: usize = cache.values().fold(0usize, |n, o| n.saturating_add(loaded_bytes(o, data))); if owned <= Self::EDITOR_CACHE_BYTES { return Ok(()); } @@ -602,6 +605,8 @@ impl Document { /// Last-resort lookup: scan the file for `num G obj`. fn scan_for(&self, num: u32) -> Option<(ObjRef, Object)> { + #[cfg(test)] + super::OBJECT_SEARCHES.with(|c| c.set(c.get() + 1)); let needle = format!("{num} "); let data = &self.data; let mut found = None; diff --git a/crates/cos/src/document/state.rs b/crates/cos/src/document/state.rs index b4323f982e..121a59c2e7 100644 --- a/crates/cos/src/document/state.rs +++ b/crates/cos/src/document/state.rs @@ -30,6 +30,14 @@ impl LoadGuard { } } +/// What loading a file object gave, kept in the document's cache either way. +pub(super) type Loaded = Result, CosError>; + +/// The bytes a load result holds of its own ([`owned_bytes`]); a failure holds none. +pub(super) fn loaded_bytes(loaded: &Loaded, data: &Arc>) -> usize { + loaded.as_ref().map_or(0, |o| owned_bytes(o, data, 0)) +} + impl Drop for LoadGuard { fn drop(&mut self) { let outermost = LOADING.with(|l| { @@ -165,7 +173,7 @@ impl ObjectReader { // have cached large objects. if let Ok(mut cache) = self.document.cache.lock() { let data = &self.document.data; - if cache.len() >= Self::OBJECTS || cache.values().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, data, 0))) > Self::STREAM_BYTES { + if cache.len() >= Self::OBJECTS || cache.values().fold(0usize, |n, o| n.saturating_add(loaded_bytes(o, data))) > Self::STREAM_BYTES { cache.clear(); } } @@ -177,6 +185,8 @@ impl ObjectReader { thread_local! { /// Cross-reference sections read on this thread. pub(super) static SECTION_READS: std::cell::Cell = const { std::cell::Cell::new(0) }; + /// Whole-file searches for an object (`Document::scan_for`) on this thread. + pub(super) static OBJECT_SEARCHES: std::cell::Cell = const { std::cell::Cell::new(0) }; } /// Nesting [`owned_bytes`] walks: past the parser's own nesting limit, so every parsed object diff --git a/crates/cos/src/document/tests.rs b/crates/cos/src/document/tests.rs index cca4b87326..3cebe0136d 100644 --- a/crates/cos/src/document/tests.rs +++ b/crates/cos/src/document/tests.rs @@ -588,6 +588,31 @@ fn an_entry_that_misses_its_object_is_found_by_searching_but_is_not_exact() { assert!(doc.xref_entry_is_exact(1) && doc.xref_entry_is_exact(2)); } +/// An object that can't be read at its entry or anywhere else is searched for in the whole file +/// once: every later read, in a clone too, gives the same error without searching again. An +/// edit of that number still answers for it. +#[test] +fn an_unreadable_object_is_searched_for_once() { + let bodies = ["<< /Type /Catalog /Pages 2 0 R >>", "<< /Type /Pages /Kids [] /Count 0 >>", "<< /Odd Key(x) >>"]; + let doc = Document::open(Arc::new(build(&bodies, "/Root 1 0 R /Info 3 0 R"))).unwrap(); + // Counted wherever the whole-file search is done (`scan_for`). + OBJECT_SEARCHES.with(|c| c.set(0)); + let first = doc.try_get(3); + assert!(first.is_err(), "{first:?}"); + for _ in 0..10 { + assert_eq!(doc.try_get(3), first); + assert_eq!(*doc.get(ObjRef::new(3, 0)), Object::Null); + } + let mut edited = doc.clone(); + assert_eq!(edited.try_get(3), first); + assert_eq!(OBJECT_SEARCHES.with(|c| c.get()), 1, "searches for an object that isn't there"); + edited.set(ObjRef::new(3, 0), Object::Int(7)); + assert_eq!(edited.try_get(3).as_deref(), Ok(&Object::Int(7))); + assert_eq!(doc.try_get(3), first, "the edit is the clone's alone"); + edited.free(ObjRef::new(3, 0)); + assert_eq!(edited.try_get(3).as_deref(), Ok(&Object::Null)); +} + /// A classic table with `/Size` rows and object 0's free entry first, its subsection numbered /// `1 n` instead of `0 n` (#2676): every row is one number off. fn misnumbered_table(start: u32) -> Vec { @@ -1109,7 +1134,7 @@ fn temporary_reader_keeps_file_views_and_evicts_data_of_its_own() { // Data of its own (decrypted streams, strings) counts: past the budget the cache is // cleared after the next read, including a failed one. let owned = Object::Stream(crate::Stream::from_raw(Dict::new(), vec![b'y'; ObjectReader::STREAM_BYTES + 1])); - reader.document.cache.lock().unwrap().insert(3, Arc::new(owned)); + reader.document.cache.lock().unwrap().insert(3, Ok(Arc::new(owned))); assert_eq!(*reader.get(ObjRef::new(99, 0)), Object::Null); assert!(reader.document.cache.lock().unwrap().is_empty(), "owned data over budget is evicted"); } @@ -1198,8 +1223,8 @@ fn editor_cache_counts_deeply_nested_strings() { let Object::Ref(reference) = reference else { panic!("indirect object") }; drop(doc.get(*reference)); let cache = doc.cache.lock().unwrap(); - let owned = cache.values().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, &doc.data, 0))); - let strings = cache.values().filter(|o| o.as_array().is_some()).count(); + let owned = cache.values().flatten().fold(0usize, |n, o| n.saturating_add(owned_bytes(o, &doc.data, 0))); + let strings = cache.values().flatten().filter(|o| o.as_array().is_some()).count(); assert!(strings << 20 <= Document::EDITOR_CACHE_BYTES + (1 << 20), "{strings} MiB strings cached, {owned} bytes counted"); } }