From 7efacbdf957bad4daaa7db153918c4b11744577f Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Wed, 8 Jul 2026 02:01:28 +0800 Subject: [PATCH] fix(filemeta): validate part array lengths in into_fileinfo (#4382) MetaObject::into_fileinfo indexed part_sizes[i]/part_actual_sizes[i] by part_numbers.len() without checking the arrays are the same length, unlike the adjacent part_etags/part_indices which are length-guarded. decode_from pushes the three arrays independently and the xl.meta CRC only covers bytes, so a CRC-valid but internally inconsistent xl.meta (foreign writer / MinIO interop) triggers an out-of-bounds panic on the GET/HEAD/LIST decode path. Guard the three arrays for equal length and return Err(FileCorrupt) so a divergent shard is skipped and quorum uses the other disks, instead of panicking the request task. Cascade into_fileinfo to Result across its callers, and fix io_primitives early-return to derive the version id from the merged header and fall into the per-disk loop (single-disk survival + heal). The 2118 merge-first path is left as a documented follow-up. Refs backlog#900 (filemeta-01). --- .../src/set_disk/core/io_primitives.rs | 129 +++++++++++--- crates/filemeta/src/filemeta.rs | 98 ++++++++++- crates/filemeta/src/filemeta/version.rs | 157 +++++++++++++++--- crates/filemeta/src/metacache.rs | 59 +++++++ 4 files changed, 398 insertions(+), 45 deletions(-) diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index da34e1690..407e256c4 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -2242,31 +2242,41 @@ impl SetDisks { ..Default::default() }; - let finfo = match meta.into_fileinfo(bucket, object, "", true, incl_free_vers, true) { - Ok(res) => res, - Err(err) => { - for item in errs.iter_mut() { - if item.is_none() { - *item = Some(err.clone().into()); + // Determine the winning version id. When the merged representative decodes to + // a valid FileInfo, use its version id. When it is undecodable (Err from + // corrupt part arrays) OR decodes but is not valid (e.g. a shallow merged + // representative missing erasure detail), do NOT poison every disk: derive the + // winning vid from the intact version header and fall into the per-disk loop + // below, so healthy disks still populate `meta_file_infos` to satisfy + // read_quorum while corrupt disks fail `into_fileinfo` and are flagged + // `FileCorrupt` for heal. If every disk is corrupt, they all fail in the loop, + // leaving no valid FileInfo so the caller's read_quorum fails cleanly instead + // of panicking or returning half-corrupt data. Only when there is no non-free + // version header at all is there genuinely nothing to read. + // + // `into_fileinfo` with an empty version_id selects the first non-free version + // (see FileMeta::into_fileinfo); replicate that selection from the header here. + let vid = match meta.into_fileinfo(bucket, object, "", true, incl_free_vers, true) { + Ok(finfo) if finfo.is_valid() => finfo.version_id.unwrap_or(Uuid::nil()), + _ => match meta + .versions + .iter() + .find(|v| !v.header.free_version()) + .and_then(|v| v.header.version_id) + { + Some(id) => id, + None => { + for item in errs.iter_mut() { + if item.is_none() { + *item = Some(DiskError::FileCorrupt); + } } - } - return (meta_file_infos, errs); - } + return (meta_file_infos, errs); + } + }, }; - if !finfo.is_valid() { - for item in errs.iter_mut() { - if item.is_none() { - *item = Some(DiskError::FileCorrupt); - } - } - - return (meta_file_infos, errs); - } - - let vid = finfo.version_id.unwrap_or(Uuid::nil()); - for (idx, meta_op) in metadata_array.iter().enumerate() { if let Some(meta) = meta_op { match meta.into_fileinfo(bucket, object, vid.to_string().as_str(), read_data, incl_free_vers, true) { @@ -3701,4 +3711,81 @@ mod tests { let key = ReadRepairHealCacheKey::new("bucket", &object, None, 3, 4); release_read_repair_heal_reservation(&key).await; } + + // ------------------------------------------------------------------ + // backlog#900: pick_latest_quorum_files_info must survive a single + // corrupt-part disk (even in the merged representative slot) by deriving + // the vid from the header and falling into the per-disk loop, flagging the + // corrupt disk for heal instead of poisoning the whole read. + // ------------------------------------------------------------------ + + use rustfs_filemeta::{ChecksumAlgo, ErasureAlgo, FileMeta, FileMetaVersion, MetaObject, RawFileInfo, VersionType}; + use time::OffsetDateTime; + use uuid::Uuid; + + fn raw_object_version(vid: Uuid, part_sizes: Vec) -> RawFileInfo { + let mut fm = FileMeta::new(); + fm.add_version_filemata(FileMetaVersion { + version_type: VersionType::Object, + object: Some(MetaObject { + version_id: Some(vid), + erasure_algorithm: ErasureAlgo::ReedSolomon, + erasure_m: 2, + erasure_n: 1, + erasure_index: 1, + erasure_dist: vec![1, 2, 3], + erasure_block_size: 1 << 20, + bitrot_checksum_algo: ChecksumAlgo::HighwayHash, + part_numbers: vec![1, 2], + part_sizes, + part_actual_sizes: vec![10, 20], + mod_time: Some(OffsetDateTime::from_unix_timestamp(1_700_000_000).unwrap()), + ..Default::default() + }), + ..Default::default() + }) + .unwrap(); + RawFileInfo { + buf: fm.marshal_msg().unwrap(), + } + } + + #[tokio::test] + async fn pick_latest_quorum_masks_single_corrupt_disk_in_representative_slot() { + let vid = Uuid::new_v4(); + // Deterministic: the corrupt disk is fixed at index 0 (representative slot). + let fileinfos = vec![ + Some(raw_object_version(vid, vec![10])), + Some(raw_object_version(vid, vec![10, 20])), + Some(raw_object_version(vid, vec![10, 20])), + ]; + let errs = vec![None, None, None]; + + let (infos, out_errs) = SetDisks::pick_latest_quorum_files_info(fileinfos, errs, "bucket", "obj", false, false).await; + + // The corrupt representative disk is flagged for heal. + assert_eq!(out_errs[0], Some(DiskError::FileCorrupt), "corrupt representative disk must be flagged"); + // Good disks still produce valid FileInfo, satisfying read_quorum (3.div_ceil(2)=2). + let good = infos.iter().filter(|fi| fi.is_valid()).count(); + assert!(good >= 2, "quorum of good disks must survive corrupt representative, got {good}"); + } + + #[tokio::test] + async fn pick_latest_quorum_all_corrupt_fails_clean_without_panic() { + let vid = Uuid::new_v4(); + let fileinfos = vec![ + Some(raw_object_version(vid, vec![10])), + Some(raw_object_version(vid, vec![10])), + Some(raw_object_version(vid, vec![10])), + ]; + let errs = vec![None, None, None]; + + let (infos, out_errs) = SetDisks::pick_latest_quorum_files_info(fileinfos, errs, "bucket", "obj", false, false).await; + + assert!( + out_errs.iter().all(|e| e == &Some(DiskError::FileCorrupt)), + "all disks must be flagged corrupt" + ); + assert!(infos.iter().all(|fi| !fi.is_valid()), "no half-corrupt FileInfo may be returned"); + } } diff --git a/crates/filemeta/src/filemeta.rs b/crates/filemeta/src/filemeta.rs index a2e9981ce..6fc8fcae2 100644 --- a/crates/filemeta/src/filemeta.rs +++ b/crates/filemeta/src/filemeta.rs @@ -716,9 +716,21 @@ impl FileMeta { && let Ok(found_free_fi) = ver.parse_version_meta() && found_free_fi.version_type != VersionType::Invalid { - let mut free_fi = found_free_fi.into_fileinfo(volume, path, all_parts); - free_fi.is_latest = true; - found_free_version = Some(free_fi); + // Graceful degradation: the free-version replication-accounting record + // is auxiliary metadata; a corrupt one must not tank an otherwise + // healthy primary-version read. Log and skip rather than propagate. + // Known side effect: if a disk holds only free versions and they are + // corrupt, `into_fileinfo` falls through to `FileNotFound` (not + // `FileCorrupt`), so that disk is not enqueued for heal. + match found_free_fi.into_fileinfo(volume, path, all_parts) { + Ok(mut free_fi) => { + free_fi.is_latest = true; + found_free_version = Some(free_fi); + } + Err(e) => { + warn!(volume, path, error = %e, "skipping corrupt free version during into_fileinfo"); + } + } } if header.version_id != Some(vid) { @@ -1543,6 +1555,84 @@ mod test { } } + // ------------------------------------------------------------------ + // backlog#900: CRC-valid but semantically corrupt part arrays must + // produce Err(FileCorrupt), never panic. + // ------------------------------------------------------------------ + + fn valid_object_version(version_id: Uuid, part_sizes: Vec) -> FileMetaVersion { + FileMetaVersion { + version_type: VersionType::Object, + object: Some(MetaObject { + version_id: Some(version_id), + erasure_algorithm: ErasureAlgo::ReedSolomon, + erasure_m: 2, + erasure_n: 2, + erasure_block_size: 1 << 20, + bitrot_checksum_algo: ChecksumAlgo::HighwayHash, + part_numbers: vec![1, 2], + part_sizes, // caller-injected (short = corrupt) + part_actual_sizes: vec![10, 20], + mod_time: Some(OffsetDateTime::now_utc()), + ..Default::default() + }), + ..Default::default() + } + } + + #[test] + fn crc_valid_but_part_arrays_corrupt_into_fileinfo_errors_not_panics() { + // A short part_sizes Object version round-trips through the real codec: the CRC is + // valid and load succeeds, but into_fileinfo(all_parts) hits the length guard and + // returns Err(FileCorrupt) rather than panicking. + let mut fm = FileMeta::new(); + fm.add_version_filemata(valid_object_version(Uuid::new_v4(), vec![10])) + .expect("add corrupt-parts version"); + + let encoded = fm.marshal_msg().expect("marshal recomputes a valid CRC"); + let loaded = FileMeta::load(&encoded).expect("CRC-valid meta must load (lazy, parts not decoded yet)"); + + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + loaded.into_fileinfo("bucket", "key", "", true, false, true) + })); + let inner = caught.expect("into_fileinfo must not panic on CRC-valid but semantically corrupt parts"); + assert!(matches!(inner, Err(Error::FileCorrupt)), "expected FileCorrupt"); + } + + proptest! { + #[test] + fn into_fileinfo_never_panics_on_arbitrary_loaded_meta(input in vec(any::(), 0..=4096)) { + // Complements filemeta_load_never_panics_on_arbitrary_bytes: for every FileMeta + // that loads, into_fileinfo(all_parts=true) must not panic (Ok or Err). + if let Ok(fm) = FileMeta::load(&input) { + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + fm.into_fileinfo("b", "k", "", true, false, true) + })); + prop_assert!(caught.is_ok(), "into_fileinfo panicked on loaded meta"); + } + } + } + + #[test] + fn into_file_info_versions_fails_whole_listing_on_one_corrupt_version() { + // One corrupt version among healthy ones fails the whole listing (into_file_info_versions + // uses `?`, so no partial results). This is the established failure semantics of the + // merge-first exact-versions path (backlog#900 ยง3.3), strictly better than a panic. + let healthy_id = Uuid::new_v4(); + let corrupt_id = Uuid::new_v4(); + let mut fm = FileMeta::new(); + fm.add_version_filemata(valid_object_version(healthy_id, vec![10, 20])) + .expect("healthy"); + fm.add_version_filemata(valid_object_version(corrupt_id, vec![10])) + .expect("corrupt"); // short part_sizes + + let fm = FileMeta::load(&fm.marshal_msg().expect("marshal")).expect("load"); + + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| fm.into_file_info_versions("bucket", "key", true))); + let inner = caught.expect("must not panic"); + assert!(matches!(inner, Err(Error::FileCorrupt)), "whole-listing must fail with FileCorrupt"); + } + #[test] fn test_performance_with_large_metadata() { // Test performance with large metadata files @@ -2055,7 +2145,7 @@ mod test { fm.update_object_version(update).unwrap(); let (_, version) = fm.find_version(version_id).unwrap(); - let stored = version.into_fileinfo("bucket", "test", true); + let stored = version.into_fileinfo("bucket", "test", true).expect("into_fileinfo"); assert_eq!(stored.metadata.get("x-amz-meta-owner"), Some(&"alice".to_string())); assert_eq!(stored.checksum, Some(checksum)); } diff --git a/crates/filemeta/src/filemeta/version.rs b/crates/filemeta/src/filemeta/version.rs index 15fae8924..06cd42793 100644 --- a/crates/filemeta/src/filemeta/version.rs +++ b/crates/filemeta/src/filemeta/version.rs @@ -350,8 +350,7 @@ impl FileMetaShallowVersion { } pub fn into_fileinfo(&self, volume: &str, path: &str, all_parts: bool) -> Result { - let file_version = self.parse_version_meta()?; - Ok(file_version.into_fileinfo(volume, path, all_parts)) + self.parse_version_meta()?.into_fileinfo(volume, path, all_parts) } } @@ -673,7 +672,9 @@ impl FileMetaVersion { FileMetaVersionHeader::from(self.clone()) } - pub fn into_fileinfo(&self, volume: &str, path: &str, all_parts: bool) -> FileInfo { + pub fn into_fileinfo(&self, volume: &str, path: &str, all_parts: bool) -> Result { + // Only the Object arm carries part arrays and can fail the length guard; the + // Legacy and Delete arms have no part arrays and stay infallible. let mut fi = match self.version_type { VersionType::Invalid | VersionType::Legacy => { if let Some(ref legacy) = self.legacy_object { @@ -691,7 +692,7 @@ impl FileMetaVersion { self.object .as_ref() .unwrap_or(&default_object) - .into_fileinfo(volume, path, all_parts) + .into_fileinfo(volume, path, all_parts)? } VersionType::Delete => { let default_marker = MetaDeleteMarker::default(); @@ -702,7 +703,7 @@ impl FileMetaVersion { } }; fi.uses_legacy_checksum = self.uses_legacy_checksum; - fi + Ok(fi) } /// Support for Legacy version type @@ -2255,22 +2256,37 @@ impl MetaObject { Ok(()) } - pub fn into_fileinfo(&self, volume: &str, path: &str, all_parts: bool) -> FileInfo { + pub fn into_fileinfo(&self, volume: &str, path: &str, all_parts: bool) -> Result { let version_id = self.version_id.filter(|&vid| !vid.is_nil()); let parts = if all_parts { - let mut parts = vec![ObjectPartInfo::default(); self.part_numbers.len()]; + let n = self.part_numbers.len(); + // Required fields: `part_sizes`/`part_actual_sizes` must match `part_numbers` + // exactly. These arrays are decoded from independent msgpack length prefixes + // (`decode_from`), so a truncated/half-written/bitrot xl.meta can leave them + // shorter (or longer) than `part_numbers`. Indexing without this guard would + // panic; silently defaulting to 0 would miscompute Content-Length / Range / + // multipart boundaries and hand corrupt data to the client (worse than an + // error). `n == 0` preserves the legitimate "no-part object / legacy / empty + // array" wire (MinIO parity: empty PartNumbers is valid). + if n != 0 && (self.part_sizes.len() != n || self.part_actual_sizes.len() != n) { + return Err(Error::FileCorrupt); + } + let mut parts = vec![ObjectPartInfo::default(); n]; for (i, part) in parts.iter_mut().enumerate() { part.number = self.part_numbers[i]; part.size = self.part_sizes[i]; part.actual_size = self.part_actual_sizes[i]; - if self.part_etags.len() == self.part_numbers.len() { + // etag/index stay soft-guarded: they are recomputable / optional, an empty + // PartETags is legitimate MinIO interop, and a length mismatch there does + // not corrupt returned data. + if self.part_etags.len() == n { part.etag = self.part_etags[i].clone(); } - if self.part_indices.len() == self.part_numbers.len() { + if self.part_indices.len() == n { part.index = if self.part_indices[i].is_empty() { None } else { @@ -2350,7 +2366,7 @@ impl MetaObject { .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - FileInfo { + Ok(FileInfo { version_id, erasure, data_dir: self.data_dir, @@ -2368,7 +2384,7 @@ impl MetaObject { transition_version_id, transition_tier, ..Default::default() - } + }) } pub fn set_transition(&mut self, fi: &FileInfo) { @@ -3266,6 +3282,97 @@ mod tests { OffsetDateTime::from_unix_timestamp_nanos(1_705_312_200_123_456_789).unwrap() } + // ---------------------------------------------------------------------- + // backlog#900: MetaObject::into_fileinfo part-array length guard. + // ---------------------------------------------------------------------- + + fn object_with_parts(part_numbers: Vec, part_sizes: Vec, part_actual_sizes: Vec) -> MetaObject { + MetaObject { + version_id: Some(sample_version_id()), + erasure_algorithm: ErasureAlgo::ReedSolomon, + erasure_m: 2, + erasure_n: 2, + erasure_block_size: 1_048_576, + bitrot_checksum_algo: ChecksumAlgo::HighwayHash, + part_numbers, + part_sizes, + part_actual_sizes, + mod_time: Some(sample_mod_time()), + ..Default::default() + } + } + + #[test] + fn into_fileinfo_rejects_short_part_sizes() { + // part_numbers=2 but part_sizes=1 -> old code indexed self.part_sizes[1] and panicked. + let obj = object_with_parts(vec![1, 2], vec![10], vec![10, 20]); + let res = obj.into_fileinfo("bucket", "key", true); + assert!(matches!(res, Err(Error::FileCorrupt)), "short part_sizes must map to FileCorrupt"); + } + + #[test] + fn into_fileinfo_rejects_short_part_actual_sizes_including_empty() { + let obj = object_with_parts(vec![1, 2], vec![10, 20], vec![]); + let res = obj.into_fileinfo("bucket", "key", true); + assert!(matches!(res, Err(Error::FileCorrupt)), "empty part_actual_sizes must map to FileCorrupt"); + } + + #[test] + fn into_fileinfo_rejects_over_long_part_sizes() { + // Strict equality: an over-long array is corrupt too (MinIO would panic on any mismatch). + let obj = object_with_parts(vec![1], vec![10, 20], vec![10]); + let res = obj.into_fileinfo("bucket", "key", true); + assert!(matches!(res, Err(Error::FileCorrupt)), "over-long part_sizes must map to FileCorrupt"); + } + + #[test] + fn into_fileinfo_all_parts_false_skips_validation() { + // all_parts=false never enters the parts loop -> Ok, empty parts. Regression guard. + let obj = object_with_parts(vec![1, 2], vec![10], vec![10, 20]); + let fi = obj + .into_fileinfo("bucket", "key", false) + .expect("all_parts=false must not validate parts"); + assert!(fi.parts.is_empty()); + } + + #[test] + fn into_fileinfo_healthy_parts_decode_field_by_field() { + let mut obj = object_with_parts(vec![1, 2], vec![100, 200], vec![111, 222]); + obj.part_etags = vec!["etag-1".to_string(), "etag-2".to_string()]; + obj.part_indices = vec![Bytes::from_static(b"idx1"), Bytes::new()]; + + let fi = obj.into_fileinfo("b", "k", true).expect("healthy parts must decode"); + assert_eq!(fi.parts.len(), 2); + assert_eq!(fi.parts[0].number, 1); + assert_eq!(fi.parts[0].size, 100); + assert_eq!(fi.parts[0].actual_size, 111); + assert_eq!(fi.parts[0].etag, "etag-1"); + assert_eq!(fi.parts[0].index.as_deref(), Some(&b"idx1"[..])); + assert_eq!(fi.parts[1].number, 2); + assert_eq!(fi.parts[1].size, 200); + assert_eq!(fi.parts[1].actual_size, 222); + assert_eq!(fi.parts[1].etag, "etag-2"); + assert_eq!(fi.parts[1].index, None); // empty bytes -> None + } + + #[test] + fn into_fileinfo_empty_part_numbers_is_valid() { + // No-part object (empty part_numbers) is legitimate: n==0 -> no guard -> Ok, no parts. + let obj = object_with_parts(vec![], vec![], vec![]); + let fi = obj.into_fileinfo("b", "k", true).expect("no-part object must be valid"); + assert!(fi.parts.is_empty()); + } + + #[test] + fn into_fileinfo_etag_soft_guard_not_regressed() { + // size/actual match (pass hard guard) but etag length differs -> soft guard: empty etag, not corrupt. + let mut obj = object_with_parts(vec![1, 2], vec![10, 20], vec![10, 20]); + obj.part_etags = vec!["only-one".to_string()]; + let fi = obj.into_fileinfo("b", "k", true).expect("etag soft guard must not fail"); + assert_eq!(fi.parts.len(), 2); + assert_eq!(fi.parts[0].etag, ""); // soft guard: len mismatch -> default empty + } + fn sample_header() -> FileMetaVersionHeader { FileMetaVersionHeader { version_id: Some(sample_version_id()), @@ -3506,7 +3613,7 @@ mod tests { assert!(decoded.valid()); assert!(decoded.legacy_object.is_some()); - let fi = decoded.into_fileinfo("bucket", "hello.txt", true); + let fi = decoded.into_fileinfo("bucket", "hello.txt", true).expect("into_fileinfo"); assert_eq!(fi.volume, "bucket"); assert_eq!(fi.name, "hello.txt"); assert_eq!(fi.size, 11); @@ -3541,7 +3648,7 @@ mod tests { assert!(decoded.delete_marker.is_some()); assert!(decoded.uses_legacy_checksum); - let fi = decoded.into_fileinfo("bucket", "gone.txt", true); + let fi = decoded.into_fileinfo("bucket", "gone.txt", true).expect("into_fileinfo"); assert!(fi.deleted); assert_eq!(fi.volume, "bucket"); assert_eq!(fi.name, "gone.txt"); @@ -3576,7 +3683,7 @@ mod tests { assert_eq!(delete_marker.version_id, Some(version_id)); assert_eq!(delete_marker.mod_time, Some(mod_time)); - let fi = decoded.into_fileinfo("bucket", "deleted.txt", true); + let fi = decoded.into_fileinfo("bucket", "deleted.txt", true).expect("into_fileinfo"); assert!(fi.deleted); assert_eq!(fi.version_id, Some(version_id)); assert_eq!(fi.mod_time, Some(mod_time)); @@ -3650,7 +3757,9 @@ mod tests { assert_eq!(object.version_id, None); assert_eq!(object.data_dir, None); - let fi = decoded.into_fileinfo("bucket", "legacy-nil.txt", true); + let fi = decoded + .into_fileinfo("bucket", "legacy-nil.txt", true) + .expect("into_fileinfo"); assert_eq!(fi.version_id, None); assert_eq!(fi.data_dir, None); assert_eq!(fi.metadata.get("content-type").map(String::as_str), Some("text/plain")); @@ -3678,7 +3787,7 @@ mod tests { assert_eq!(delete_marker.version_id, None); assert_eq!(delete_marker.mod_time, Some(sample_mod_time())); - let fi = decoded.into_fileinfo("bucket", "deleted.txt", true); + let fi = decoded.into_fileinfo("bucket", "deleted.txt", true).expect("into_fileinfo"); assert!(fi.deleted); assert_eq!(fi.version_id, None); assert_eq!(fi.mod_time, Some(sample_mod_time())); @@ -3728,7 +3837,9 @@ mod tests { #[test] fn meta_object_transition_version_id_absent_yields_none() { - let fi = make_meta_object_with_sys(HashMap::new()).into_fileinfo("b", "k", false); + let fi = make_meta_object_with_sys(HashMap::new()) + .into_fileinfo("b", "k", false) + .expect("into_fileinfo"); assert_eq!(fi.transition_version_id, None); } @@ -3736,7 +3847,9 @@ mod tests { fn meta_object_transition_version_id_empty_bytes_yields_none() { let mut sys = HashMap::new(); insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, vec![]); - let fi = make_meta_object_with_sys(sys).into_fileinfo("b", "k", false); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("into_fileinfo"); assert_eq!(fi.transition_version_id, None); } @@ -3745,7 +3858,9 @@ mod tests { // Regression: old code used unwrap_or_default() which turned nil bytes into Some(Uuid::nil()) let mut sys = HashMap::new(); insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, Uuid::nil().as_bytes().to_vec()); - let fi = make_meta_object_with_sys(sys).into_fileinfo("b", "k", false); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("into_fileinfo"); assert_eq!(fi.transition_version_id, None); } @@ -3754,7 +3869,9 @@ mod tests { let id = sample_version_id(); let mut sys = HashMap::new(); insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, id.as_bytes().to_vec()); - let fi = make_meta_object_with_sys(sys).into_fileinfo("b", "k", false); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("into_fileinfo"); assert_eq!(fi.transition_version_id, Some(id)); } diff --git a/crates/filemeta/src/metacache.rs b/crates/filemeta/src/metacache.rs index e311a6bd6..7b1c971ee 100644 --- a/crates/filemeta/src/metacache.rs +++ b/crates/filemeta/src/metacache.rs @@ -1987,4 +1987,63 @@ mod tests { assert_eq!(err.kind(), std::io::ErrorKind::Other); assert_eq!(calls.load(Ordering::SeqCst), 2); } + + // ------------------------------------------------------------------ + // backlog#900: metacache boundaries live outside the HTTP CatchPanicLayer + // (background / listing chains). A corrupt-part xl.meta must yield Err, + // not panic, so it cannot poison a worker. + // ------------------------------------------------------------------ + + fn corrupt_parts_filemeta() -> FileMeta { + use crate::{ChecksumAlgo, ErasureAlgo, MetaObject, VersionType}; + let mut fm = FileMeta::new(); + fm.add_version_filemata(FileMetaVersion { + version_type: VersionType::Object, + object: Some(MetaObject { + version_id: Some(Uuid::new_v4()), + erasure_algorithm: ErasureAlgo::ReedSolomon, + erasure_m: 2, + erasure_n: 2, + erasure_block_size: 1 << 20, + bitrot_checksum_algo: ChecksumAlgo::HighwayHash, + part_numbers: vec![1, 2], + part_sizes: vec![10], // corrupt: shorter than part_numbers + part_actual_sizes: vec![10, 20], + mod_time: Some(time::OffsetDateTime::now_utc()), + ..Default::default() + }), + ..Default::default() + }) + .expect("add version"); + // Round-trip once so the corrupt array survives exactly like on disk. + FileMeta::load(&fm.marshal_msg().expect("marshal")).expect("load") + } + + #[test] + fn metacache_to_fileinfo_returns_err_not_panic_on_corrupt_parts() { + let entry = MetaCacheEntry { + name: "obj".to_string(), + metadata: Vec::new(), + cached: Some(corrupt_parts_filemeta()), + reusable: false, + }; + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| entry.to_fileinfo("bucket"))); + let inner = caught.expect("to_fileinfo must not panic"); + assert!(matches!(inner, Err(Error::FileCorrupt)), "expected FileCorrupt from metacache boundary"); + } + + #[test] + fn metacache_file_info_versions_returns_err_not_panic_on_corrupt_parts() { + // file_info_versions reads self.metadata (not cached), so fill marshaled bytes. + let bytes = corrupt_parts_filemeta().marshal_msg().expect("marshal"); + let entry = MetaCacheEntry { + name: "obj".to_string(), + metadata: bytes, + cached: None, + reusable: false, + }; + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| entry.file_info_versions("bucket"))); + let inner = caught.expect("file_info_versions must not panic"); + assert!(matches!(inner, Err(Error::FileCorrupt)), "expected FileCorrupt"); + } }