From b4ba4edc7f3fd8b4ef647dc43f6878c301643724 Mon Sep 17 00:00:00 2001 From: KenM76 Date: Tue, 3 Nov 2026 23:58:08 -0500 Subject: [PATCH] M12: PRC reader: charge copied blocks, refuse overflowing schema expressions Review fixes from #6087: - Picture and uncompressed blocks are charged to the parse ceiling before they are copied. File-structure headers may overlap, so each structure's copy counts. The ceiling test now includes the 3-byte picture. - Schema expressions use checked multiply, add and subtract; an overflow is Malformed rather than a wrapped loop count. - Each face's triangles are counted from its entity sizes and charged before it is triangulated. - Matrix::mirrors divides each row by its largest entry before taking the determinants, so a mirror whose determinant underflows still reverses the winding. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9 --- crates/prc/src/container.rs | 29 ++++++++++++++++++++--------- crates/prc/src/error.rs | 4 ++-- crates/prc/src/geometry.rs | 27 ++++++++++++++++++++++++--- crates/prc/src/schema.rs | 15 ++++++++++++--- crates/prc/src/tess.rs | 5 +++-- crates/prc/src/tess/tests.rs | 12 ++++++++++++ crates/prc/src/tess/triangulate.rs | 13 +++++++++++++ crates/prc/tests/container.rs | 13 +++++++++++-- 8 files changed, 97 insertions(+), 21 deletions(-) diff --git a/crates/prc/src/container.rs b/crates/prc/src/container.rs index 93efdeda3d..3ff18508f1 100644 --- a/crates/prc/src/container.rs +++ b/crates/prc/src/container.rs @@ -17,7 +17,8 @@ pub const PRC_READER_VERSION: u32 = 8137; /// The most file structures [`PrcFile::parse`] accepts. pub const MAX_FILE_STRUCTURES: usize = 65_536; -/// Ceiling on the total inflated size of every section in one PRC stream. +/// Ceiling on the total inflated size of every section in one PRC stream, together with the +/// picture and uncompressed blocks it copies. pub const MAX_INFLATED_BYTES: usize = 512 * 1024 * 1024; /// Sections per file structure [WD 6.1.2]: its header, then the five in [`SectionKind`] @@ -163,8 +164,9 @@ impl<'a> Bytes<'a> { } } - /// `UncompressedBlock`s: a count, then size-prefixed blocks [WD 10.2.2-3]. - fn blocks(&mut self, what: &'static str) -> Result>, PrcError> { + /// `UncompressedBlock`s: a count, then size-prefixed blocks [WD 10.2.2-3], each charged to + /// `budget` before it is copied. + fn blocks(&mut self, what: &'static str, budget: &mut Budget) -> Result>, PrcError> { let n = offset(self.u32(what)?); // Each block needs at least its 4-byte size, so `n` is bounded by the data left. if n > self.data.len().saturating_sub(self.pos) / 4 { @@ -173,7 +175,9 @@ impl<'a> Bytes<'a> { (0..n) .map(|_| { let len = offset(self.u32(what)?); - Ok(self.take(len, what)?.to_vec()) + let block = self.take(len, what)?; + budget.charge(block.len())?; + Ok(block.to_vec()) }) .collect() } @@ -235,7 +239,8 @@ fn file_structure(data: &[u8], id: UniqueId, extent: &Extent, budget: &mut Budge return Err(PrcError::Malformed("file structure id differs from its description".into())); } let _application = h.id("file structure header")?; - let pictures = h.blocks("file structure pictures")?; + // Headers may overlap, so each structure's copy of its pictures is charged. + let pictures = h.blocks("file structure pictures", budget)?; let mut sections: [Vec; 5] = Default::default(); for (i, (slot, kind)) in sections.iter_mut().zip(SectionKind::ALL).enumerate() { let end = if i + 2 < SECTIONS { extent.bound(i + 2) } else { extent.last }; @@ -262,9 +267,15 @@ impl Budget { // One byte past the budget is enough to know it was exceeded. let cap = u64::try_from(self.left).unwrap_or(u64::MAX).saturating_add(1); flate2::read::ZlibDecoder::new(src).take(cap).read_to_end(&mut out).map_err(|e| PrcError::Inflate { section, reason: e.to_string() })?; - self.left = self.left.checked_sub(out.len()).ok_or(PrcError::TooLarge { limit: self.limit })?; + self.charge(out.len())?; Ok(out) } + + /// Spend `n` bytes of the ceiling. + fn charge(&mut self, n: usize) -> Result<(), PrcError> { + self.left = self.left.checked_sub(n).ok_or(PrcError::TooLarge { limit: self.limit })?; + Ok(()) + } } impl PrcFile { @@ -278,7 +289,7 @@ impl PrcFile { /// [`PrcError::NotPrc`] without the magic; [`PrcError::Truncated`] or /// [`PrcError::Malformed`] for a damaged header or offset table; [`PrcError::Inflate`] /// for a section that is not zlib; [`PrcError::TooLarge`] past [`MAX_INFLATED_BYTES`] - /// in total. + /// in total, inflated sections and copied blocks together. /// /// # Examples /// ``` @@ -310,7 +321,8 @@ impl PrcFile { let descriptions = (0..n_fs).map(|_| file_structure_description(&mut r)).collect::, _>>()?; let mf_start = offset(r.u32("model file offsets")?); let mf_end = offset(r.u32("model file offsets")?); - let uncompressed_files = r.blocks("uncompressed files")?; + let mut budget = Budget { left: limit, limit }; + let uncompressed_files = r.blocks("uncompressed files", &mut budget)?; if mf_end < mf_start { return Err(PrcError::Malformed("model file ends before it starts".into())); } @@ -318,7 +330,6 @@ impl PrcFile { return Err(PrcError::Malformed(format!("model file ends at {mf_end}, past the {}-byte stream", data.len()))); } - let mut budget = Budget { left: limit, limit }; // prc-rs: the last section runs to the model file's start. let last = mf_start.min(data.len()); let file_structures = diff --git a/crates/prc/src/error.rs b/crates/prc/src/error.rs index d31bf9f420..c1d927a224 100644 --- a/crates/prc/src/error.rs +++ b/crates/prc/src/error.rs @@ -24,8 +24,8 @@ pub enum PrcError { /// The decoder's message. reason: String, }, - /// The inflated sections together exceed the ceiling ([`crate::MAX_INFLATED_BYTES`] or - /// the caller's limit). + /// The inflated sections and copied blocks together exceed the ceiling + /// ([`crate::MAX_INFLATED_BYTES`] or the caller's limit). #[error("PRC sections inflate past the {limit}-byte ceiling")] TooLarge { /// The ceiling that was hit. diff --git a/crates/prc/src/geometry.rs b/crates/prc/src/geometry.rs index baa48331d5..c8d7680e05 100644 --- a/crates/prc/src/geometry.rs +++ b/crates/prc/src/geometry.rs @@ -79,10 +79,15 @@ impl Matrix { /// Whether mapping points through this matrix turns them inside out. Where the point is /// divided by `w`, the Jacobian's sign is the sign of the 4×4 determinant; a last row of - /// zeros (no division) falls back to the 3×3 block. + /// zeros (no division) falls back to the 3×3 block. Each row is first divided by its + /// largest entry, which keeps both signs while the products stay in range. fn mirrors(&self) -> bool { - let det = self.det4(); - if det == 0.0 { self.det3() < 0.0 } else { det < 0.0 } + let scaled = Matrix(self.0.map(|row| { + let big = row.iter().fold(0.0_f64, |m, c| m.max(c.abs())); + if big.is_finite() && big > 0.0 { row.map(|c| c / big) } else { row } + })); + let det = scaled.det4(); + if det == 0.0 { scaled.det3() < 0.0 } else { det < 0.0 } } /// Cofactor `(i, j)` of the upper-left 3×3 block (`i`, `j` < 3). @@ -247,6 +252,22 @@ mod tests { assert_eq!(mesh.transformed(&positive).triangles, [[0, 1, 2]]); } + #[test] + fn a_mirror_whose_determinant_underflows_still_reverses_the_winding() { + let mesh = TriangleMesh { + positions: vec![[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]], + triangles: vec![[0, 1, 2]], + ..TriangleMesh::default() + }; + let diagonal = |x: f64| { + let mut m = Matrix::IDENTITY; + (m.0[0][0], m.0[1][1], m.0[2][2]) = (x, 1e-200, 1e-200); + m + }; + assert_eq!(mesh.transformed(&diagonal(-1e200)).triangles, [[0, 2, 1]]); + assert_eq!(mesh.transformed(&diagonal(1e200)).triangles, [[0, 1, 2]]); + } + impl Matrix { fn transformed_normal(&self, mesh: &TriangleMesh) -> [f64; 3] { mesh.transformed(self).normals.first().copied().unwrap_or_default() diff --git a/crates/prc/src/schema.rs b/crates/prc/src/schema.rs index 8191537acd..2b505c14ea 100644 --- a/crates/prc/src/schema.rs +++ b/crates/prc/src/schema.rs @@ -303,11 +303,12 @@ impl Exec<'_, '_, '_> { if !run { return Ok(0); } + let overflow = || malformed("a schema expression overflows"); Ok(match t { - 29 => a.wrapping_mul(b), + 29 => a.checked_mul(b).ok_or_else(overflow)?, 30 => a.checked_div(b).ok_or_else(|| malformed("division by zero"))?, - 31 => a.wrapping_add(b), - 32 => a.wrapping_sub(b), + 31 => a.checked_add(b).ok_or_else(overflow)?, + 32 => a.checked_sub(b).ok_or_else(overflow)?, 33 => i64::from(a < b), 34 => i64::from(a <= b), 35 => i64::from(a > b), @@ -492,6 +493,14 @@ mod tests { assert_eq!(used, want); } + #[test] + fn an_expression_that_overflows_is_malformed() { + // For ((2^31 * 2^31) * 4) { UInt }: 2^64 would wrap to 0 and read nothing. + let prog: &[u32] = &[15, 29, 29, 26, 1 << 31, 26, 1 << 31, 26, 4, 3]; + let (res, _) = run(&[(172, prog)], &W::default()); + assert!(matches!(res, Err(PrcError::Malformed(m)) if m.contains("overflows"))); + } + #[test] fn version_blocks_run_only_when_newer_than_the_reader() { let mut d = W::default(); diff --git a/crates/prc/src/tess.rs b/crates/prc/src/tess.rs index 235137c74e..dfb943a39f 100644 --- a/crates/prc/src/tess.rs +++ b/crates/prc/src/tess.rs @@ -203,10 +203,11 @@ impl Ctx<'_, '_> { let styled = faces.iter().any(|f| !f.styles.is_empty()); let mut corners = Corners::default(); for f in &faces { + // Faces may reuse one index range, so each face's triangles are charged before + // they are made. + self.r.take_elements(f.triangle_count()?)?; let start = mesh.triangles.len(); let entities = f.triangulate(&indices, !recalc, mesh.positions.len(), &mut mesh.triangles, &mut corners)?; - // Faces may reuse one index range, so the triangles are charged as they are made. - self.r.take_elements(mesh.triangles.len().saturating_sub(start))?; mesh.faces.push(start..mesh.triangles.len()); if styled { f.styles_into(&entities, &mut mesh.triangle_styles); diff --git a/crates/prc/src/tess/tests.rs b/crates/prc/src/tess/tests.rs index a7f80aa60e..1e142975b4 100644 --- a/crates/prc/src/tess/tests.rs +++ b/crates/prc/src/tess/tests.rs @@ -690,6 +690,18 @@ fn faces_reusing_one_index_range_are_charged_for_each_triangle_they_make() { assert_eq!(three - one, 2 * 4); } +#[test] +fn a_face_is_charged_before_it_is_triangulated() { + // One face claiming a million triangles over a 12-entry index array. + const INDICES: [u32; 12] = [0, 0, 0, 3, 0, 6, 0, 0, 0, 6, 0, 9]; + let mut b = W::default(); + tess_3d(&mut b, 8137, &Mesh { indices: &INDICES, faces: &[(0x2, 0, &[1_000_000])], ..Mesh::default() }); + let bytes = section(1, &b).bytes(); + let schema = Schema::default(); + let r = Ctx::new(BitReader::with_elements(&bytes, 1000), &schema, 8137).file_structure_tessellation(); + assert!(matches!(r, Err(PrcError::TooManyElements { .. })), "{r:?}"); +} + #[test] fn the_globals_schema_and_the_tessellations_share_one_allowance() { // A schema of one entry with three tokens: four elements. diff --git a/crates/prc/src/tess/triangulate.rs b/crates/prc/src/tess/triangulate.rs index b6481ed995..04b9d98e9a 100644 --- a/crates/prc/src/tess/triangulate.rs +++ b/crates/prc/src/tess/triangulate.rs @@ -119,6 +119,19 @@ impl Face { Ok(n) } + /// Triangles [`Self::triangulate`] makes: one per triangle-block count, two fewer than the + /// points of each fan and strip. + pub(super) fn triangle_count(&self) -> Result { + let mut n = 0usize; + for (b, sizes) in self.entities()? { + for s in sizes { + let s = (s & COUNT_MASK) as usize; + n = n.saturating_add(if matches!(b.shape, Shape::Triangles) { s } else { s.saturating_sub(2) }); + } + } + Ok(n) + } + /// Emit the face's triangles into `out`; `normals` = the index array stores normal slots, /// in which case each triangle's corner normal slots go to `corners`, as do its corner /// texture slots. Returns the triangles each triangulation entity emitted, in order: one diff --git a/crates/prc/tests/container.rs b/crates/prc/tests/container.rs index 96f4981f29..3702daf070 100644 --- a/crates/prc/tests/container.rs +++ b/crates/prc/tests/container.rs @@ -95,9 +95,18 @@ fn damaged_containers_are_refused_by_name() { #[test] fn the_inflation_ceiling_is_reported_not_exceeded() { + // 1,200 inflated bytes and the 3-byte picture. let f = build([&[0u8; 600], &[0u8; 600], b"", b"", b""], b"", 0); - assert!(PrcFile::parse_with_limit(&f, 1200).is_ok()); - assert_eq!(PrcFile::parse_with_limit(&f, 1199).unwrap_err(), PrcError::TooLarge { limit: 1199 }); + assert!(PrcFile::parse_with_limit(&f, 1203).is_ok()); + assert_eq!(PrcFile::parse_with_limit(&f, 1202).unwrap_err(), PrcError::TooLarge { limit: 1202 }); +} + +#[test] +fn copied_picture_blocks_are_charged_to_the_ceiling() { + // Nothing inflates to more than 0 bytes; the header's one picture is 3 bytes. + let f = build([b"", b"", b"", b"", b""], b"", 0); + assert!(PrcFile::parse_with_limit(&f, 3).is_ok()); + assert_eq!(PrcFile::parse_with_limit(&f, 2).unwrap_err(), PrcError::TooLarge { limit: 2 }); } #[test]