Skip to content

Commit 24ec2b9

Browse files
committed
splat3d/PR7-fix: reject overflowing PLY vertex counts before allocation
External-reviewer bug report against PR #153: > When a malformed or fuzzed PLY header advertises a vertex count > larger than usize::MAX / (62 * 4), this size calculation overflows > (panics in debug, wraps in release). In release that allocates a > too-small bytes buffer and the subsequent per-vertex loop indexes > past it instead of returning a PlyError, so a bad input can crash > the loader; use checked multiplication before allocating/reading > the body. ## Root cause `read_ply` computed the body byte count via: let mut bytes = vec![0u8; n_vertices * PROPERTIES_PER_VERTEX * 4]; For `n_vertices > usize::MAX / 248`: - debug: panic on the unchecked `*`. - release: wraps to a small number, allocates a too-small buffer, `read_exact` succeeds (reads only the wrapped count of bytes — often zero), then the per-vertex loop indexes far past the allocation. Crash or — worse — silent corruption if the wrapped size happens to land at a valid index. ## Fix Gate the body size with `checked_mul` BEFORE allocation: let body_bytes = n_vertices .checked_mul(PROPERTIES_PER_VERTEX) .and_then(|n| n.checked_mul(4)) .ok_or_else(|| PlyError::BadElement(format!( "vertex count {n_vertices} × {PROPERTIES_PER_VERTEX} props × 4 bytes \ overflows usize on this target ({} bits)", usize::BITS, )))?; let mut bytes = vec![0u8; body_bytes]; The downstream per-vertex `i * stride` math is now safe by transitivity — for any `i < n_vertices`, `i * stride ≤ body_bytes ≤ usize::MAX`. No further bounds work needed. ## Regression test `rejects_overflowing_vertex_count`: - Computes `overflow_count = usize::MAX / (PROPERTIES_PER_VERTEX * 4) + 1` (the smallest count that overflows on the current target). - Builds a valid PLY header advertising that count, with NO body bytes — the overflow check must fire BEFORE any I/O is attempted. - Asserts `PlyError::BadElement` with a message containing "overflows". Verified green in BOTH debug and release builds, where the wrapping (not panicking) release path is the actual security concern. ## Test count cargo test --features splat3d --lib hpc::splat3d::ply → 5 passed; 0 failed (was 4: +1 overflow regression) cargo test --features splat3d --lib hpc::splat3d → 91 passed; 0 failed (was 90: +1) cargo test --features splat3d --release --lib hpc::splat3d::ply → 5 passed; 0 failed (release-build confirms no wrap-then-corrupt) https://claude.ai/code/session_017GFLBnDy23AWBqvkbHHC41
1 parent 9e96459 commit 24ec2b9

1 file changed

Lines changed: 63 additions & 1 deletion

File tree

src/hpc/splat3d/ply.rs

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,29 @@ pub fn read_ply<R: Read>(reader: R) -> Result<GaussianBatch, PlyError> {
182182
}
183183

184184
// Read the binary body — n_vertices × 62 f32 little-endian.
185-
let mut bytes = vec![0u8; n_vertices * PROPERTIES_PER_VERTEX * 4];
185+
//
186+
// External-reviewer bug class: malformed / fuzzed headers can
187+
// advertise a vertex count large enough that
188+
// `n_vertices * PROPERTIES_PER_VERTEX * 4` overflows usize:
189+
// - debug: panics on the unchecked mul.
190+
// - release: wraps to a small number, allocates a too-small
191+
// buffer, `read_exact` returns Ok, the per-vertex loop then
192+
// indexes far past the buffer end (panic OR — worse — silent
193+
// corruption if the wrap happens to land at a valid index).
194+
//
195+
// Gate the size up-front with checked_mul. Any overflow becomes a
196+
// `PlyError::BadElement` — fuzzer-safe, no allocation attempted.
197+
let body_bytes = n_vertices
198+
.checked_mul(PROPERTIES_PER_VERTEX)
199+
.and_then(|n| n.checked_mul(4))
200+
.ok_or_else(|| {
201+
PlyError::BadElement(format!(
202+
"vertex count {n_vertices} × {PROPERTIES_PER_VERTEX} props × 4 bytes \
203+
overflows usize on this target ({} bits)",
204+
usize::BITS
205+
))
206+
})?;
207+
let mut bytes = vec![0u8; body_bytes];
186208
buf.read_exact(&mut bytes).map_err(|_| PlyError::Truncated)?;
187209

188210
// Convert into a GaussianBatch with activations applied.
@@ -349,4 +371,44 @@ mod tests {
349371
Err(e) => panic!("expected UnexpectedProperty, got {e:?}"),
350372
}
351373
}
374+
375+
// External-reviewer bug class: a fuzzed / malformed header that
376+
// advertises a vertex count larger than `usize::MAX / (62 * 4)`
377+
// overflows the pre-allocation size computation. Pre-fix:
378+
// - debug build panics on the unchecked `*`
379+
// - release build wraps to a small number, allocates a too-small
380+
// buffer, then `read_exact` succeeds with zero bytes, and the
381+
// per-vertex loop indexes past the buffer end → crash or
382+
// silent corruption.
383+
// Post-fix: `checked_mul` chain returns `PlyError::BadElement`
384+
// BEFORE any allocation is attempted.
385+
#[test]
386+
fn rejects_overflowing_vertex_count() {
387+
// Smallest count that overflows: usize::MAX / (62*4) + 1.
388+
let max_safe = usize::MAX / (PROPERTIES_PER_VERTEX * 4);
389+
let overflow_count = max_safe.checked_add(1).expect("max_safe + 1 fits in usize");
390+
391+
// Build the header (no body needed — overflow check fires BEFORE
392+
// the read_exact, which is the whole point: no allocation, no
393+
// I/O attempt against a multi-exabyte advertised body).
394+
let mut header = String::new();
395+
header.push_str("ply\n");
396+
header.push_str("format binary_little_endian 1.0\n");
397+
header.push_str(&format!("element vertex {overflow_count}\n"));
398+
for p in &expected_properties() {
399+
header.push_str(&format!("property float {p}\n"));
400+
}
401+
header.push_str("end_header\n");
402+
403+
match read_ply(Cursor::new(header.into_bytes())) {
404+
Err(PlyError::BadElement(msg)) => {
405+
assert!(
406+
msg.contains("overflows"),
407+
"expected overflow message, got: {msg}"
408+
);
409+
}
410+
Ok(_) => panic!("expected BadElement on overflow, got Ok(batch)"),
411+
Err(e) => panic!("expected BadElement on overflow, got {e:?}"),
412+
}
413+
}
352414
}

0 commit comments

Comments
 (0)