diff --git a/crates/symbolicator-js/src/sourcemap_cache.rs b/crates/symbolicator-js/src/sourcemap_cache.rs index 15471428e..e33a7384c 100644 --- a/crates/symbolicator-js/src/sourcemap_cache.rs +++ b/crates/symbolicator-js/src/sourcemap_cache.rs @@ -104,6 +104,13 @@ fn write_sourcemap_cache(file: &mut File, source: &str, sourcemap: &str) -> Cach let file = writer.into_inner().map_err(io::Error::from)?; file.sync_all()?; + // Parse the sourcemapcache file to verify integrity + let bv = ByteView::map_file_ref(file)?; + if SourceMapCache::parse(&bv).is_err() { + tracing::error!("Failed to verify integrity of freshly written SourceMapCache"); + return Err(CacheError::InternalError); + } + Ok(()) } diff --git a/crates/symbolicator-native/src/caches/ppdb_caches.rs b/crates/symbolicator-native/src/caches/ppdb_caches.rs index d2b11e00b..dfa208890 100644 --- a/crates/symbolicator-native/src/caches/ppdb_caches.rs +++ b/crates/symbolicator-native/src/caches/ppdb_caches.rs @@ -149,5 +149,12 @@ fn write_ppdb_cache(file: &mut File, object_handle: &ObjectHandle) -> CacheEntry let file = writer.into_inner().map_err(io::Error::from)?; file.sync_all()?; + // Parse the ppdbcache file to verify integrity + let bv = ByteView::map_file_ref(file)?; + if PortablePdbCache::parse(&bv).is_err() { + tracing::error!("Failed to verify integrity of freshly written PortablePDB Cache"); + return Err(CacheError::InternalError); + } + Ok(()) } diff --git a/crates/symbolicator-native/src/caches/symcaches.rs b/crates/symbolicator-native/src/caches/symcaches.rs index 42db3c0b2..ed496c5d5 100644 --- a/crates/symbolicator-native/src/caches/symcaches.rs +++ b/crates/symbolicator-native/src/caches/symcaches.rs @@ -271,6 +271,13 @@ fn write_symcache( let file = writer.into_inner().map_err(io::Error::from)?; file.sync_all()?; + // Parse the symcache file to verify integrity + let bv = ByteView::map_file_ref(file)?; + if SymCache::parse(&bv).is_err() { + tracing::error!("Failed to verify integrity of freshly written SymCache"); + return Err(CacheError::InternalError); + } + Ok(()) } diff --git a/crates/symbolicator-service/src/caching/memory.rs b/crates/symbolicator-service/src/caching/memory.rs index f279da712..9e873f4de 100644 --- a/crates/symbolicator-service/src/caching/memory.rs +++ b/crates/symbolicator-service/src/caching/memory.rs @@ -212,6 +212,12 @@ impl Cacher { let byte_view = ByteView::map_file_ref(temp_file.as_file())?; entry = Ok(byte_view); } + Err(CacheError::InternalError) => { + // If there was an `InternalError` during computation (for instance because + // of an io error), we return immediately without writing the error + // or persisting the temp file. + return Err(CacheError::InternalError); + } Err(err) => { let mut temp_fd = tokio::fs::File::from_std(temp_file.reopen()?); err.write(&mut temp_fd).await?; diff --git a/crates/symbolicator-service/src/caching/tests.rs b/crates/symbolicator-service/src/caching/tests.rs index 630c5cf22..afc8b2ea6 100644 --- a/crates/symbolicator-service/src/caching/tests.rs +++ b/crates/symbolicator-service/src/caching/tests.rs @@ -1,5 +1,5 @@ use std::fs::{self, File}; -use std::io::Write; +use std::io::{self, Write}; use std::path::Path; use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::sleep; @@ -917,3 +917,79 @@ async fn test_lazy_computation_limit() { assert_eq!(num_outdated, 2); } + +/// A request to compute a cache item that always fails. +#[derive(Clone)] +struct FailingTestCacheItem(CacheError); + +impl CacheItemRequest for FailingTestCacheItem { + type Item = String; + + const VERSIONS: CacheVersions = CacheVersions { + current: 1, + fallbacks: &[], + }; + + fn compute<'a>(&'a self, temp_file: &'a mut NamedTempFile) -> BoxFuture<'a, CacheEntry> { + Box::pin(async move { + fs::write(temp_file.path(), "garbage data")?; + Err(self.0.clone()) + }) + } + + fn load(&self, data: ByteView<'static>) -> CacheEntry { + Ok(std::str::from_utf8(data.as_slice()).unwrap().to_owned()) + } +} + +/// Verifies that an internal error during computation results in the temporary +/// file being discarded instead of persisted. +#[tokio::test] +async fn test_failing_cache_write() { + test::setup(); + let cache_dir = test::tempdir(); + + let config = Config { + cache_dir: Some(cache_dir.path().to_path_buf()), + ..Default::default() + }; + let cache = Cache::from_config( + CacheName::Objects, + &config, + CacheConfig::from(CacheConfigs::default().derived), + Arc::new(AtomicIsize::new(1)), + 1024, + ) + .unwrap(); + let cacher = Cacher::new(cache, Default::default()); + + // Case 1: internal error + let request = FailingTestCacheItem(CacheError::InternalError); + let key = CacheKey::for_testing("global/internal_error"); + + let entry = cacher + .compute_memoized(request, key.clone()) + .await + .unwrap_err(); + assert_eq!(entry, CacheError::InternalError); + + // The computation returned `InternalError`, so the file should not have been + // persisted + let cache_file_path = cache_dir.path().join("objects").join(key.cache_path(1)); + assert!(!fs::exists(cache_file_path).unwrap()); + + // Case 2: malformed error + let request = FailingTestCacheItem(CacheError::Malformed("this is garbage".to_owned())); + let key = CacheKey::for_testing("global/malformed"); + + let entry = cacher + .compute_memoized(request, key.clone()) + .await + .unwrap_err(); + assert_eq!(entry, CacheError::Malformed("this is garbage".to_owned())); + + // The computation returned `Malformed`, so the file should have been + // persisted + let cache_file_path = cache_dir.path().join("objects").join(key.cache_path(1)); + assert!(fs::exists(cache_file_path).unwrap()); +}