Skip to content

Commit 1291ace

Browse files
authored
fix(zstd): validate frame content sizes (#9164)
## Rationale for this change Fuzzing found that zstd (arrays + buffers) serialised output sizes were not validated against the declared uncompressed sizes in frame headers. ## What changes are included in this PR? Validate serialized zstd output sizes against frame headers before CPU allocation and host-backed GPU planning. Add regressions for zstd arrays and zstd buffers. Note that as long as the size declared in the frame header matches what's declared in metadata, allocations are not bounded. But this is much harder to fix, since zstd can essentially decompress to `⌈compressed_len/3⌉ × 128 KiB` which is ~42GiB per MiB of compressed data so not particularly useful as a bound. ## What APIs are changed? Are there any user-facing changes? Malformed files where output sizes declared in zstd frames don't match the header are now refused instead of allocating a buffer of file-controlled size. Investigated and built with Codex. Signed-off-by: Lorenz Hübschle <lorenz@firebolt.io>
1 parent fae9da1 commit 1291ace

4 files changed

Lines changed: 75 additions & 1 deletion

File tree

‎encodings/zstd/src/array.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ use vortex_session::registry::CachedId;
5959

6060
use crate::ZstdFrameMetadata;
6161
use crate::ZstdMetadata;
62+
use crate::validate_frame_content_size;
6263

6364
// Zstd doesn't support training dictionaries on very few samples.
6465
const MIN_SAMPLES_FOR_DICTIONARY: usize = 8;
@@ -670,6 +671,10 @@ impl ZstdData {
670671
self.frames.len(),
671672
self.metadata.frames.len()
672673
);
674+
for (index, (frame, metadata)) in self.frames.iter().zip(&self.metadata.frames).enumerate()
675+
{
676+
validate_frame_content_size(frame.as_slice(), metadata.uncompressed_size, index)?;
677+
}
673678

674679
Ok(())
675680
}

‎encodings/zstd/src/lib.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
2424
pub use array::*;
2525
use vortex_array::dtype::proto::dtype as pb;
26+
use vortex_error::VortexResult;
27+
use vortex_error::vortex_ensure;
28+
use vortex_error::vortex_err;
2629
#[cfg(feature = "unstable_encodings")]
2730
pub use zstd_buffers::*;
2831

@@ -36,6 +39,22 @@ mod zstd_buffers;
3639
#[cfg(test)]
3740
mod test;
3841

42+
/// Ensure Vortex metadata agrees with the content size declared by a zstd frame.
43+
pub(crate) fn validate_frame_content_size(
44+
frame: &[u8],
45+
metadata_size: u64,
46+
index: usize,
47+
) -> VortexResult<()> {
48+
let frame_content_size = zstd::zstd_safe::get_frame_content_size(frame)
49+
.map_err(|error| vortex_err!("Invalid zstd frame {index}: {error}"))?
50+
.ok_or_else(|| vortex_err!("Zstd frame {index} does not declare a content size"))?;
51+
vortex_ensure!(
52+
metadata_size == frame_content_size,
53+
"Zstd frame {index} metadata declares {metadata_size} uncompressed bytes, but its header declares {frame_content_size}"
54+
);
55+
Ok(())
56+
}
57+
3958
#[derive(Clone, prost::Message)]
4059
/// Metadata for one zstd frame.
4160
pub struct ZstdFrameMetadata {

‎encodings/zstd/src/test.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use vortex_array::assert_nth_scalar;
1313
use vortex_array::builders::DynVarBinBuilder;
1414
use vortex_array::dtype::DType;
1515
use vortex_array::dtype::Nullability;
16+
use vortex_array::dtype::PType;
1617
use vortex_array::validity::Validity;
1718
use vortex_buffer::Alignment;
1819
use vortex_buffer::Buffer;
@@ -289,3 +290,20 @@ fn test_zstd_frame_start_buffer_alignment() {
289290

290291
assert!(compressed.is_ok());
291292
}
293+
294+
#[test]
295+
fn test_zstd_rejects_mismatched_frame_content_size() {
296+
let mut ctx = array_session().create_execution_ctx();
297+
let compressed =
298+
Zstd::from_primitive(&PrimitiveArray::from_iter([1_i32, 2, 3]), 0, 0, &mut ctx).unwrap();
299+
let mut data = compressed.data().clone();
300+
data.metadata.frames[0].uncompressed_size = 16 * 1024 * 1024 * 1024;
301+
302+
let error = Zstd::try_new(
303+
DType::Primitive(PType::I32, Nullability::NonNullable),
304+
data,
305+
Validity::NonNullable,
306+
)
307+
.unwrap_err();
308+
assert!(error.to_string().contains("metadata declares"));
309+
}

‎encodings/zstd/src/zstd_buffers.rs‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ use vortex_session::VortexSession;
3939
use vortex_session::registry::CachedId;
4040

4141
use crate::ZstdBuffersMetadata;
42+
use crate::validate_frame_content_size;
4243

4344
/// A [`ZstdBuffers`]-encoded Vortex array.
4445
pub type ZstdBuffersArray = Array<ZstdBuffers>;
@@ -258,6 +259,8 @@ impl ZstdBuffersData {
258259
let alignment = self.buffer_alignments.get(i).copied().unwrap_or(1);
259260

260261
let aligned = Alignment::try_from(alignment)?;
262+
let compressed = buf.clone().try_to_host_sync()?;
263+
validate_frame_content_size(compressed.as_slice(), uncompressed_size, i)?;
261264
let mut output = ByteBufferMut::with_capacity_aligned(size, aligned);
262265
let spare = output.spare_capacity_mut();
263266

@@ -274,7 +277,6 @@ impl ZstdBuffersData {
274277
// `set_len(size)` after zstd reports how many bytes were written.
275278
let dst =
276279
unsafe { std::slice::from_raw_parts_mut(spare.as_mut_ptr().cast::<u8>(), size) };
277-
let compressed = buf.clone().try_to_host_sync()?;
278280
let written = decompressor.decompress_to_buffer(compressed.as_slice(), dst)?;
279281
if written != size {
280282
return Err(vortex_err!(
@@ -295,6 +297,18 @@ impl ZstdBuffersData {
295297
// If invariants are somehow broken, device decompression could have UB, so ensure
296298
// they still hold.
297299
self.validate()?;
300+
// Only host-resident frames are checked: reading a device frame's header would cost a D2H
301+
// copy per frame, and the device output allocation is fallible so it cannot abort.
302+
for (index, (buffer, &metadata_size)) in self
303+
.compressed_buffers
304+
.iter()
305+
.zip(&self.uncompressed_sizes)
306+
.enumerate()
307+
{
308+
if let Some(frame) = buffer.as_host_opt() {
309+
validate_frame_content_size(frame.as_slice(), metadata_size, index)?;
310+
}
311+
}
298312

299313
let output_sizes = self
300314
.uncompressed_sizes
@@ -653,6 +667,24 @@ mod tests {
653667
Ok(())
654668
}
655669

670+
#[test]
671+
fn test_rejects_mismatched_frame_content_size_before_output_allocation() -> VortexResult<()> {
672+
let compressed = ZstdBuffers::compress(&make_primitive_array(), 3, &array_session())?;
673+
let mut data = compressed.data().clone();
674+
data.uncompressed_sizes[0] = 16 * 1024 * 1024 * 1024;
675+
676+
for error in [
677+
data.decompress_buffers().unwrap_err(),
678+
data.decode_plan().unwrap_err(),
679+
] {
680+
assert!(
681+
error.to_string().contains("metadata declares"),
682+
"unexpected error: {error}"
683+
);
684+
}
685+
Ok(())
686+
}
687+
656688
#[test]
657689
fn test_validity_delegates_for_nullable_input() -> VortexResult<()> {
658690
let input = make_nullable_primitive_array();

0 commit comments

Comments
 (0)