From 34c5f4196e0f56de8f574a199caae878290b86ff Mon Sep 17 00:00:00 2001 From: jose Date: Wed, 30 Sep 2026 11:13:26 -0400 Subject: [PATCH] Return errors instead of aborting on damaged USDC files A damaged or hostile .usdc could make the crate reader allocate whatever a count in the file asked for (a few bytes of input requested exabytes), index past tables, reach todo!() for old versions, or overflow while sizing buffers. Each of these aborts an application that builds with panic = "abort", and a failed allocation aborts any application. Counts are now checked against the file length, LZ4 output is capped at its maximum expansion of the compressed input and at 4 GiB, vectors are read before they are allocated, preallocation from file counts is capped, table lookups and slicing are checked, unsupported old versions return an error, and data-dependent debug assertions are errors in every build. tests/usdc_malformed.rs mutates five fixtures deterministically and decodes every field; before this change it aborted the test process. --- crates/openusd/src/usdc/coding.rs | 17 ++++- crates/openusd/src/usdc/reader.rs | 101 ++++++++++++++++++------- crates/openusd/tests/usdc_malformed.rs | 99 ++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 33 deletions(-) create mode 100644 crates/openusd/tests/usdc_malformed.rs diff --git a/crates/openusd/src/usdc/coding.rs b/crates/openusd/src/usdc/coding.rs index b8668bfa..537aabd5 100644 --- a/crates/openusd/src/usdc/coding.rs +++ b/crates/openusd/src/usdc/coding.rs @@ -19,7 +19,9 @@ pub fn encoded_buffer_size(count: usize) -> usize { 0 } else { let sz = mem::size_of::(); - sz + (count * 2).div_ceil(8) + (sz * count) + // Saturating: a count from a damaged file must not overflow here. + sz.saturating_add(count.saturating_mul(2).div_ceil(8)) + .saturating_add(sz.saturating_mul(count)) } } @@ -45,15 +47,22 @@ where codes_reader.read_pod::()? as i64 }; - let num_code_bytes = (count * 2).div_ceil(8); + let num_code_bytes = count + .checked_mul(2) + .ok_or_else(|| ReadError::corrupt(format!("integer count {count} overflows")))? + .div_ceil(8); let mut ints_reader = { let offset = mem::size_of::() + num_code_bytes; - io::Cursor::new(&data[offset..]) + let rest = data + .get(offset..) + .ok_or_else(|| ReadError::corrupt(format!("{count} integers need more than {} bytes", data.len())))?; + io::Cursor::new(rest) }; let mut prev = 0_i64; - let mut output = Vec::with_capacity(count); + // Every integer takes at least two bits of `data`. + let mut output = Vec::with_capacity(count.min(data.len().saturating_mul(4))); for _ in 0..num_code_bytes { // Code byte stores integer types for the next 4 integers. diff --git a/crates/openusd/src/usdc/reader.rs b/crates/openusd/src/usdc/reader.rs index 6ca4f5ac..63626bd6 100644 --- a/crates/openusd/src/usdc/reader.rs +++ b/crates/openusd/src/usdc/reader.rs @@ -34,6 +34,10 @@ macro_rules! corrupt { }; } +/// Largest single LZ4-decompressed block the reader allocates (4 GiB). +/// (Saturates on 32-bit targets.) +const MAX_DECOMPRESSED_BYTES: usize = if usize::BITS > 32 { 4 << 30 } else { usize::MAX }; + // Maximum supported USDC crate version. // See USD Core Specification v1.0.1 §16.3.8.2 for version history: // 0.10.0 — Path Expression value types @@ -46,6 +50,8 @@ const SW_VERSION: Version = version(0, 12, 0); pub struct CrateFile { /// File reader. reader: R, + /// Length of the whole file: what bounds every count read from it. + file_length: usize, /// File header. pub bootstrap: Bootstrap, @@ -84,9 +90,13 @@ impl CrateFile { /// Read structural sections of a crate file. pub fn open(mut reader: R) -> Result { let bootstrap = Self::read_header(&mut reader)?; + let here = reader.stream_position()?; + let file_length = reader.seek(io::SeekFrom::End(0))? as usize; + reader.seek(io::SeekFrom::Start(here))?; let mut file = CrateFile { reader, + file_length, bootstrap, sections: Vec::new(), tokens: Vec::new(), @@ -212,7 +222,7 @@ impl CrateFile { let count = self.reader.read_count()?; self.tokens = if file_ver < version(0, 4, 0) { - todo!("Support TOKENS reader for < 0.4.0 files"); + return Err(ReadError::unsupported("Support TOKENS reader for < 0.4.0 files")); } else { let uncompressed_size = self.reader.read_count()?; let mut buffer = self.read_compressed(uncompressed_size)?; @@ -289,7 +299,7 @@ impl CrateFile { let file_ver = self.version(); self.fields = if file_ver < version(0, 4, 0) { - todo!("Support FIELDS reader before < 0.4.0") + return Err(ReadError::unsupported("Support FIELDS reader before < 0.4.0")); } else { let field_count = self.reader.read_count()?; @@ -305,7 +315,7 @@ impl CrateFile { .map(|(index, value)| Field::new(*index, *value)) .collect(); - debug_assert_eq!(fields.len(), field_count); + corrupt!(fields.len() == field_count); fields }; @@ -323,7 +333,7 @@ impl CrateFile { let file_ver = self.version(); self.fieldsets = if file_ver < version(0, 4, 0) { - todo!("Support FIELDSETS reader for < 0.4.0 files"); + return Err(ReadError::unsupported("Support FIELDSETS reader for < 0.4.0 files")); } else { let count = self.reader.read_count()?; @@ -336,7 +346,7 @@ impl CrateFile { .map(|i| if i == INVALID_INDEX { None } else { Some(i as usize) }) .collect::>(); - debug_assert_eq!(sets.len(), count); + corrupt!(sets.len() == count); sets }; @@ -354,12 +364,17 @@ impl CrateFile { let file_ver = self.version(); if file_ver == version(0, 0, 1) { - todo!("Support PATHS reader for == 0.0.1 files"); + return Err(ReadError::unsupported("Support PATHS reader for == 0.0.1 files")); } else if file_ver < version(0, 4, 0) { - todo!("Support PATHS reader for < 0.4.0 files"); + return Err(ReadError::unsupported("Support PATHS reader for < 0.4.0 files")); } else { // Read # of paths. let path_count = self.reader.read_count()?; + corrupt!( + path_count <= self.file_length, + "path count {path_count} is larger than a {}-byte file can hold", + self.file_length + ); self.paths = vec![sdf::Path::default(); path_count]; self.read_compressed_paths()?; @@ -376,13 +391,13 @@ impl CrateFile { // Read compressed data. let path_indexes = self.read_encoded_ints::(count)?; - debug_assert_eq!(path_indexes.len(), count); + corrupt!(path_indexes.len() == count); let element_token_indexes = self.read_encoded_ints::(count)?; - debug_assert_eq!(element_token_indexes.len(), count); + corrupt!(element_token_indexes.len() == count); let jumps = self.read_encoded_ints::(count)?; - debug_assert_eq!(jumps.len(), count); + corrupt!(jumps.len() == count); self.build_compressed_paths(&path_indexes, &element_token_indexes, &jumps)?; @@ -492,9 +507,9 @@ impl CrateFile { let file_ver = self.version(); self.specs = if file_ver == version(0, 0, 1) { - todo!("Support SPECS reader for == 0.0.1 files"); + return Err(ReadError::unsupported("Support SPECS reader for == 0.0.1 files")); } else if file_ver < version(0, 4, 0) { - todo!("Support SPECS reader for < 0.4.0 files"); + return Err(ReadError::unsupported("Support SPECS reader for < 0.4.0 files")); } else { // Version 0.4.0 specs are compressed @@ -592,11 +607,25 @@ impl CrateFile { ) -> Result, ReadError> { // Read data to memory. let compressed_size = self.reader.read_count()?; + corrupt!( + compressed_size <= self.file_length, + "compressed size {compressed_size} is larger than the {}-byte file", + self.file_length + ); let mut input = vec![0_u8; compressed_size]; self.reader.read_exact(&mut input)?; - // Decompress to output buffer. - let mut output = vec![T::default(); estimated_count]; + // Decompress to a buffer no larger than LZ4 can expand this input + // (`estimated_count` is an upper bound from the file, not a size). + // A single decoded value over MAX_DECOMPRESSED_BYTES is refused like + // the other suspiciously large counts: LZ4 alone would allow 255x a + // compressed block that may be most of the file. + let most = compressed_size + .saturating_mul(255) + .saturating_add(64) + .min(MAX_DECOMPRESSED_BYTES) + / mem::size_of::().max(1); + let mut output = vec![T::default(); estimated_count.min(most)]; let actual_size = decompress_lz4(&input, cast_slice_mut(&mut output))?; let actual_count = actual_size / mem::size_of::(); @@ -618,7 +647,7 @@ impl CrateFile { let buffer = self.read_compressed::(estimated_size)?; let ints = coding::decode_ints(buffer.as_slice(), count)?; - debug_assert_eq!(ints.len(), count); + corrupt!(ints.len() == count); Ok(ints) } @@ -627,7 +656,7 @@ impl CrateFile { // Implements various logic and compatibility checks to figure out the array length and whether it's compressed. fn unpack_array_len(&mut self, value: ValueRep, kind: ArrayKind) -> Result<(usize, bool), ReadError> { - debug_assert!(!value.is_inlined()); + corrupt!(!value.is_inlined()); // Empty array. if value.payload() == 0 { @@ -661,7 +690,7 @@ impl CrateFile { ArrayKind::Other => { // Fallback to uncompressed. // See https://github.com/PixarAnimationStudios/OpenUSD/blob/0b18ad3f840c24eb25e16b795a5b0821cf05126e/pxr/usd/usd/crateFile.cpp#L1868 - debug_assert!(!value.is_compressed()); + corrupt!(!value.is_compressed()); compressed = false; } } @@ -729,7 +758,9 @@ impl CrateFile { let mut output = vec![T::zero(); count]; for (i, index) in indexes.into_iter().enumerate() { - output[i] = lut[index as usize]; + output[i] = *lut.get(index as usize).ok_or_else(|| { + ReadError::corrupt(format!("lookup index {index} is outside a {}-entry table", lut.len())) + })?; } output @@ -954,7 +985,7 @@ impl CrateFile { /// the on-disk element — safe for all gf vec types since they are /// `#[repr(C)]` and `bytemuck::Pod`. fn read_gf_array(&mut self, value: ValueRep) -> Result, ReadError> { - debug_assert!(value.is_array() && !value.is_compressed()); + corrupt!(value.is_array() && !value.is_compressed()); let (count, _) = self.unpack_array_len(value, ArrayKind::Other)?; if count == 0 { return Ok(Vec::default()); @@ -966,8 +997,8 @@ impl CrateFile { &mut self, value: ValueRep, ) -> Result, ReadError> { - debug_assert!(value.is_array()); - debug_assert!(!value.is_compressed()); + corrupt!(value.is_array()); + corrupt!(!value.is_compressed()); let (count, _) = self.unpack_array_len(value, ArrayKind::Other)?; @@ -1282,7 +1313,7 @@ impl CrateFile { let list = self.read_list_op(value, |file: &mut Self| { let count = file.reader.read_count()?; - let mut vec = Vec::with_capacity(count); + let mut vec = Vec::with_capacity(count.min(1024)); for _ in 0..count { let reference = file.read_reference()?; @@ -1384,7 +1415,7 @@ impl CrateFile { Type::PayloadListOp => { let list = self.read_list_op(value, |file: &mut Self| { let count = file.reader.read_count()?; - let mut vec = Vec::with_capacity(count); + let mut vec = Vec::with_capacity(count.min(1024)); for _ in 0..count { let payload = file.read_payload()?; vec.push(payload); @@ -1404,7 +1435,7 @@ impl CrateFile { self.set_position(value.payload())?; let count = self.reader.read_count()?; - let mut map = HashMap::with_capacity(count); + let mut map = HashMap::with_capacity(count.min(1024)); for _ in 0..count { let key = self.read_string()?; @@ -1448,7 +1479,7 @@ impl CrateFile { corrupt!(count == times.len(), "Invalid time samples count"); let value_reps = self.reader.read_vec::(count)?; - debug_assert_eq!(value_reps.len(), count); + corrupt!(value_reps.len() == count); let samples = times .into_iter() @@ -1523,7 +1554,7 @@ impl CrateFile { corrupt!(!value.is_inlined()); self.set_position(value.payload())?; let count = self.reader.read_count()?; - let mut pairs = Vec::with_capacity(count); + let mut pairs = Vec::with_capacity(count.min(1024)); for _ in 0..count { let src_idx: u32 = self.reader.read_pod()?; let tgt_idx: u32 = self.reader.read_pod()?; @@ -1606,7 +1637,7 @@ fn decompress_lz4(mut input: &[u8], output: &mut [u8]) -> Result ReadExt for R { return Ok(Vec::new()); } + // Read what the stream really holds before trusting `count`: a + // truncated or lying file fails here instead of allocating first. + let bytes = count + .checked_mul(mem::size_of::()) + .ok_or_else(|| ReadError::corrupt(format!("vector of {count} elements overflows")))?; + let mut raw = Vec::new(); + io::Read::read_to_end(&mut io::Read::take(&mut *self, bytes as u64), &mut raw).ctx("vec")?; + if raw.len() != bytes { + return Err(ReadError::corrupt(format!( + "vec: {} of {bytes} bytes before the end", + raw.len() + ))); + } let mut vec = vec![T::default(); count]; - self.read_exact(cast_slice_mut(&mut vec)).ctx("vec")?; - + cast_slice_mut(&mut vec).copy_from_slice(&raw); Ok(vec) } } diff --git a/crates/openusd/tests/usdc_malformed.rs b/crates/openusd/tests/usdc_malformed.rs new file mode 100644 index 00000000..0855ef71 --- /dev/null +++ b/crates/openusd/tests/usdc_malformed.rs @@ -0,0 +1,99 @@ +//! Damaged `.usdc` input must come back as an error, never a panic, an +//! allocation failure or a stack overflow: an application built with +//! `panic = "abort"` would otherwise crash on a bad file. Each case mutates +//! a known-good fixture deterministically and reads every field value. + +use std::io::Cursor; +use std::panic; + +use openusd::sdf::AbstractData; +use openusd::usdc::CrateData; + +const FIXTURES: [&str; 5] = [ + "reference.usdc", + "ints.usdc", + "sdf_types.usdc", + "payload.usdc", + "floats.usdc", +]; + +/// xorshift64: the same mutations on every run and platform. +struct Rng(u64); + +impl Rng { + fn next(&mut self) -> u64 { + self.0 ^= self.0 << 13; + self.0 ^= self.0 >> 7; + self.0 ^= self.0 << 17; + self.0 + } + + fn below(&mut self, n: usize) -> usize { + (self.next() % n as u64) as usize + } +} + +/// One of: flipped bits, an extreme 64-bit value (the shape of a lying +/// count or offset), a truncation, or a large 32-bit value. +fn mutate(bytes: &mut Vec, rng: &mut Rng) { + match rng.next() % 4 { + 0 => { + for _ in 0..1 + rng.next() % 8 { + let i = rng.below(bytes.len()); + bytes[i] ^= 1 << (rng.next() % 8); + } + } + 1 => { + let i = rng.below(bytes.len() - 8); + let value = [u64::MAX, i64::MAX as u64, u32::MAX as u64, 1 << 30][rng.below(4)]; + bytes[i..i + 8].copy_from_slice(&value.to_le_bytes()); + } + 2 => { + let len = rng.below(bytes.len()).max(1); + bytes.truncate(len); + } + _ => { + let i = rng.below(bytes.len() - 4); + bytes[i..i + 4].copy_from_slice(&((rng.next() as u32) | 0x1000_0000).to_le_bytes()); + } + } +} + +/// Opens the crate and decodes every value, as a stage would. +fn read_everything(bytes: Vec) { + let Ok(data) = CrateData::open(Cursor::new(bytes), true) else { + return; + }; + for path in data.spec_paths() { + for field in data.list_fields(&path).unwrap_or_default() { + let _ = data.try_field(&path, &field); + } + } +} + +#[test] +fn mutated_crate_files_are_errors_not_panics() { + let rounds: usize = std::env::var("OPENUSD_USDC_MUTATIONS") + .ok() + .and_then(|value| value.parse().ok()) + .unwrap_or(2_000); + let mut rng = Rng(0x2545_F491_4F6C_DD1D); + let mut panics = Vec::new(); + for fixture in FIXTURES { + let path = format!("{}/fixtures/{fixture}", env!("CARGO_MANIFEST_DIR")); + let seed = std::fs::read(&path).unwrap_or_else(|e| panic!("cannot read {path}: {e}")); + for round in 0..rounds { + let mut bytes = seed.clone(); + mutate(&mut bytes, &mut rng); + if panic::catch_unwind(|| read_everything(bytes)).is_err() { + panics.push(format!("{fixture} round {round}")); + } + } + } + assert!( + panics.is_empty(), + "{} panics, first: {:?}", + panics.len(), + &panics[..panics.len().min(5)] + ); +}