diff --git a/zstd/fuzz/Cargo.toml b/zstd/fuzz/Cargo.toml index a49ba359a..c28f184fb 100644 --- a/zstd/fuzz/Cargo.toml +++ b/zstd/fuzz/Cargo.toml @@ -26,6 +26,10 @@ members = ["."] name = "decode" path = "fuzz_targets/decode.rs" +[[bin]] +name = "decode_direct" +path = "fuzz_targets/decode_direct.rs" + [[bin]] name = "encode" path = "fuzz_targets/encode.rs" diff --git a/zstd/fuzz/fuzz_targets/decode_direct.rs b/zstd/fuzz/fuzz_targets/decode_direct.rs new file mode 100644 index 000000000..df8481927 --- /dev/null +++ b/zstd/fuzz/fuzz_targets/decode_direct.rs @@ -0,0 +1,26 @@ +#![no_main] +#[macro_use] +extern crate libfuzzer_sys; + +use structured_zstd::decoding::FrameDecoder; + +// Direct-decode (`FrameDecoder::decode_all` into a fixed-capacity slice) +// adversarial-input harness for #246. The existing `decode` target only +// fuzzes the growable `StreamingDecoder` path (FlatBuf / RingBuffer, which +// can never overflow because the backing Vec grows on demand). The direct +// path routes through `UserSliceBackend`, whose writes are bounded by the +// caller-provided slice — a malformed frame that expands past the declared +// `frame_content_size` MUST surface a structured error, never panic or +// write out of bounds. This target drives exactly that path. +fuzz_target!(|data: &[u8]| { + // A fixed output slice forces the direct path when the frame's + // declared content size fits. Cap at 1 MiB so an adversarial frame + // declaring a huge `frame_content_size` can't OOM the fuzzer via the + // up-front output allocation; frames larger than this fall back to the + // growable path (still panic-free) or are rejected. The decoder's own + // window cap (100 MiB) and the per-block fallible write surface are + // what we're exercising — any panic here is a bug. + let mut out = vec![0u8; 1 << 20]; + let mut dec = FrameDecoder::new(); + let _ = dec.decode_all(data, &mut out); +}); diff --git a/zstd/src/decoding/buffer_backend.rs b/zstd/src/decoding/buffer_backend.rs index a844b139d..49d28f579 100644 --- a/zstd/src/decoding/buffer_backend.rs +++ b/zstd/src/decoding/buffer_backend.rs @@ -387,14 +387,21 @@ pub(crate) trait BufferBackend: Sized { // // Parallel `try_*` methods that return `Err(BackendOverflow)` // instead of panicking when the write would exceed the backend's - // capacity. Currently wired on Raw and RLE block paths only; - // Compressed-block sequence execution still uses the panic-on- - // overflow unchecked writes and will be migrated in a follow-up. - // Used by the direct-decode path (`decode_all` + - // descendants) so a malformed Raw/RLE block whose declared - // decompressed payload exceeds the caller-provided output slice - // surfaces as a structured `FrameDecoderError::FrameContentSizeMismatch` - // instead of an abort. + // capacity. Wired across EVERY direct-decode write path: Raw / RLE + // blocks (`try_extend` / `try_extend_and_fill`), the Compressed + // block's sequence executor (`exec_sequence_inline` returns + // `Result`, the fallback chain uses `try_push` + + // `repeat_lookahead_prefetched`, tail literals use `try_push`), and + // the match-repeat pre-check (`try_reserve`). Used by the + // direct-decode path (`decode_all` + descendants) so a malformed + // block whose decompressed payload exceeds the caller-provided + // output slice surfaces as a structured + // `FrameDecoderError::FrameContentSizeMismatch` instead of an abort + // — uniformly for Raw, RLE, and Compressed blocks (see + // `FrameDecoder::run_direct_decode`, which folds the Compressed + // sequence-executor `OutputBufferOverflow` into the same + // `FrameContentSizeMismatch` contract as the Raw/RLE + // `BackendOverflow` arm). // // The growable backends (`FlatBuf`, `RingBuffer`) rely on the // default impls below — they delegate to the corresponding @@ -455,12 +462,16 @@ pub(crate) trait BufferBackend: Sized { /// satisfy the `Self::extend_from_within_unchecked` safety /// contract at the call site. /// - /// NOTE: Currently unused on production paths. The direct - /// decode's Compressed-block sequence executor writes via the - /// existing unchecked path; threading `try_*` through the - /// fused decode+execute pipeline is the next step toward - /// unconditional adversarial-input safety. RLE/Raw blocks - /// already use `try_extend_and_fill` / `try_extend`. + /// NOTE: Retained for the SAFE-surface test matrix and as the + /// wrap-aware reference impl; not called on production paths + /// (hence `#[allow(dead_code)]`). The Compressed-block direct path + /// is already DoS-safe WITHOUT this method: its match-repeat copies + /// go through `DecodeBuffer::repeat_lookahead_prefetched`, which + /// pre-checks capacity via [`Self::try_reserve`] before the + /// unchecked wildcopy, and its literal+match sequence copies go + /// through `exec_sequence_inline` (returns `Result`). RLE/Raw use + /// `try_extend_and_fill` / `try_extend`. So every adversarial + /// overshoot already surfaces as a structured error. #[allow(dead_code)] fn try_extend_from_within(&mut self, start: usize, len: usize) -> Result<(), BackendOverflow> { // Default impl: a SAFE method must NOT delegate to the @@ -541,9 +552,12 @@ pub(crate) trait BufferBackend: Sized { /// donor-inline paths inside the sequence executor) or /// `DecodeBufferError::OutputBufferOverflow` (the match-repeat /// `try_reserve` pre-check inside `DecodeBuffer::repeat_inner`). -/// Both bubble up as a structured `FrameDecoderError` (typically -/// wrapped in `FailedToReadBlockBody`) — callers never see -/// `BackendOverflow` directly. +/// On the direct-decode path both are folded by +/// `FrameDecoder::run_direct_decode` into +/// `FrameDecoderError::FrameContentSizeMismatch` — the same +/// caller-visible error a Raw / RLE overshoot yields, so the +/// "content exceeded declared size" contract is uniform across block +/// types. Callers never see `BackendOverflow` directly. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct BackendOverflow { /// Current physical write cursor at the moment the write was diff --git a/zstd/src/decoding/errors.rs b/zstd/src/decoding/errors.rs index 95aad9f50..85ac31ebd 100644 --- a/zstd/src/decoding/errors.rs +++ b/zstd/src/decoding/errors.rs @@ -1020,6 +1020,32 @@ impl From for ExecuteSequences } } +impl ExecuteSequencesError { + /// `Some(requested)` when this error is a fixed-capacity output-buffer + /// overshoot from the Compressed-block sequence executor — either the + /// donor-inline path ([`Self::OutputBufferOverflow`]) or the + /// match-repeat fallback ([`DecodeBufferError::OutputBufferOverflow`] + /// wrapped in [`Self::DecodebufferError`]). `requested` is the byte + /// count the failing write tried to append past the slice end. + /// + /// `None` for every non-overflow variant. Used by + /// `FrameDecoder::run_direct_decode` to fold an in-block Compressed + /// overshoot into the same `FrameContentSizeMismatch` contract the + /// Raw/RLE [`DecodeBlockContentError::BackendOverflow`] arm already + /// produces — both mean "the frame's content expands past the + /// declared `frame_content_size`". + pub(crate) fn output_overflow_requested(&self) -> Option { + match self { + ExecuteSequencesError::OutputBufferOverflow { requested, .. } => Some(*requested), + ExecuteSequencesError::DecodebufferError(DecodeBufferError::OutputBufferOverflow { + requested, + .. + }) => Some(*requested), + _ => None, + } + } +} + #[derive(Debug)] #[non_exhaustive] pub enum DecodeSequenceError { @@ -1545,10 +1571,46 @@ mod tests { use alloc::{string::ToString, vec}; use super::{ - BlockTypeError, DecodeBlockContentError, DecodeSequenceError, DecompressBlockError, - DecompressLiteralsError, FSETableError, FrameDecoderError, HuffmanTableError, + BlockTypeError, DecodeBlockContentError, DecodeBufferError, DecodeSequenceError, + DecompressBlockError, DecompressLiteralsError, ExecuteSequencesError, FSETableError, + FrameDecoderError, HuffmanTableError, }; + #[test] + fn execute_sequences_output_overflow_requested_covers_all_arms() { + // #246: `run_direct_decode` folds a Compressed-block overshoot into + // `FrameContentSizeMismatch` by reading `requested` from whichever + // overflow shape the executor produced. Cover all three arms: + // 1. donor-inline path -> `OutputBufferOverflow` directly, + // 2. match-repeat path -> `DecodebufferError(OutputBufferOverflow)`, + // 3. any other variant -> None (no fold). + let inline = ExecuteSequencesError::OutputBufferOverflow { + tail: 10, + requested: 7, + capacity: 12, + }; + assert_eq!(inline.output_overflow_requested(), Some(7)); + + let repeat = + ExecuteSequencesError::DecodebufferError(DecodeBufferError::OutputBufferOverflow { + tail: 3, + requested: 99, + capacity: 4, + }); + assert_eq!(repeat.output_overflow_requested(), Some(99)); + + // Non-overflow variants (and non-overflow DecodeBufferError) -> None. + assert_eq!( + ExecuteSequencesError::ZeroOffset.output_overflow_requested(), + None + ); + assert_eq!( + ExecuteSequencesError::DecodebufferError(DecodeBufferError::ZeroOffset) + .output_overflow_requested(), + None + ); + } + #[test] fn block_and_sequence_display_messages_are_specific() { assert_eq!( diff --git a/zstd/src/decoding/frame_decoder.rs b/zstd/src/decoding/frame_decoder.rs index 71f2eadfc..4a90dff27 100644 --- a/zstd/src/decoding/frame_decoder.rs +++ b/zstd/src/decoding/frame_decoder.rs @@ -2101,6 +2101,32 @@ impl FrameDecoder { .saturating_add(u64::from(block_header.decompressed_size)), }); } + // Compressed-block in-block overshoot: the sequence + // executor (donor-inline path) or the match-repeat + // fallback tripped the fixed-capacity backend's per-write + // check. Unlike Raw/RLE, a Compressed block carries no + // header-declared output size, so `produced` is computed + // from the partial fill: `tail` bytes were written before + // the failing op, and `requested` is what overflowed — + // their sum is a strict lower bound on the frame's true + // expanded size and is always > `content_size` (the + // direct path is only entered when the slice is sized to + // `content_size + WILDCOPY_OVERLENGTH`, so any overflow + // means the frame exceeded the declared FCS, never a + // caller-undersized buffer). Folds into the same + // `FrameContentSizeMismatch` contract as Raw/RLE. + Err(crate::decoding::errors::DecodeBlockContentError::DecompressBlockError( + crate::decoding::errors::DecompressBlockError::ExecuteSequencesError(ref e), + )) if e.output_overflow_requested().is_some() => { + let requested = e + .output_overflow_requested() + .expect("guard guarantees Some") as u64; + let tail = direct.buffer.buffer_ref().tail() as u64; + return Err(err::FrameContentSizeMismatch { + declared: content_size, + produced: tail.saturating_add(requested), + }); + } Err(e) => { return Err(block_body_decode_error( e, @@ -2420,6 +2446,81 @@ mod tests { ); } + #[test] + fn decode_all_compressed_block_fcs_overflow_returns_structured_error() { + // Acceptance test for #246: a malformed frame whose *Compressed* + // block expands past the declared `frame_content_size` must + // surface `FrameContentSizeMismatch` from the direct-decode path + // (UserSliceBackend sequence executor), NOT panic and NOT a + // generic FailedToReadBlockBody. The Raw-block sibling above + // covers the `BackendOverflow` arm; this covers the Compressed + // sequence-executor overflow arm (`ExecuteSequencesError:: + // OutputBufferOverflow` folded into FrameContentSizeMismatch in + // `run_direct_decode`). + // + // Construction: compress a compressible payload to get a genuine + // Compressed block + a header-declared FCS, then surgically patch + // the FCS field down to a tiny value. The block body still + // decodes (literals/sequences are independent of FCS) and the + // sequence executor overflows the small output slice. + // Highly compressible payload (repeated phrase) → Compressed + // block whose sequence executor produces ~4 KiB of output. + let unit = b"The quick brown fox jumps over the lazy dog. "; + let mut payload = Vec::with_capacity(4 * 1024); + while payload.len() < 4 * 1024 { + payload.extend_from_slice(unit); + } + let mut compressor = FrameCompressor::new(CompressionLevel::Default); + compressor.set_source(payload.as_slice()); + let mut frame = Vec::new(); + compressor.set_drain(&mut frame); + compressor.compress(); + // Sanity: the encoder actually compressed (=> a Compressed block, + // not a raw-stored fallback) so we exercise the sequence path. + assert!(frame.len() < payload.len()); + + // Locate the FCS field: it is the last `fcs_len` bytes of the + // frame header, whose total size `header_size` includes the magic. + // A ~4 KiB single-segment frame declares FCS = 4096, which lands in + // the 2-byte field range [256, 65791] (RFC 8878 §3.1.1.1.4) — assert + // that so the patch logic below stays a single deterministic branch. + let (header, header_size) = + super::super::frame::read_frame_header(frame.as_slice()).expect("valid header"); + let fcs_len = header + .descriptor + .frame_content_size_bytes() + .expect("fcs present") as usize; + assert_eq!( + fcs_len, 2, + "4 KiB single-segment frame must use a 2-byte FCS" + ); + let fcs_off = header_size as usize - fcs_len; + + // Patch the 2-byte FCS to its floor: stored bytes 0 decode to 256 + // (the field's `+256` bias), far below the 4 KiB the block actually + // produces, so the sequence executor overflows the output slice. + let patched_declared: u64 = 256; + frame[fcs_off] = 0; + frame[fcs_off + 1] = 0; + + // Size the output to declared + WILDCOPY slack so the direct path + // is eligible (output.len() >= content_size + slack) — the + // overflow then comes from the frame, not an undersized buffer. + let slack = super::super::buffer_backend::WILDCOPY_OVERLENGTH; + let mut out = alloc::vec![0u8; patched_declared as usize + slack]; + let mut dec = FrameDecoder::new(); + let err = dec + .decode_all(frame.as_slice(), &mut out) + .expect_err("Compressed block exceeding FCS must fail decode"); + match err { + super::FrameDecoderError::FrameContentSizeMismatch { declared, produced } => { + assert_eq!(declared, patched_declared, "declared echoes patched FCS"); + assert!(produced > declared, "produced must exceed declared"); + } + other => panic!("expected FrameContentSizeMismatch, got {other:?}"), + } + } + /// Block-precise error positions (#174): a failing block header / body /// reports its 0-based index and frame-absolute offset, consistent with /// the encoder's `FrameEmitInfo.blocks[index].offset_in_frame`. diff --git a/zstd/src/decoding/user_slice_buf.rs b/zstd/src/decoding/user_slice_buf.rs index fd055fedc..f44742dc3 100644 --- a/zstd/src/decoding/user_slice_buf.rs +++ b/zstd/src/decoding/user_slice_buf.rs @@ -101,11 +101,14 @@ use super::buffer_backend::{BufferBackend, WILDCOPY_OVERLENGTH}; /// `exec_sequence_inline` (which returns /// `Result<(), ExecuteSequencesError>`). A malformed Compressed /// block whose payload expands past the declared -/// `frame_content_size` surfaces as +/// `frame_content_size` is caught per-write as /// `ExecuteSequencesError::OutputBufferOverflow` (literal-push / /// donor-inline path) or `DecodeBufferError::OutputBufferOverflow` -/// (match-repeat path), both of which propagate up the call stack -/// as a structured `FrameDecoderError` instead of panicking. +/// (match-repeat path); `FrameDecoder::run_direct_decode` folds both +/// into a structured `FrameDecoderError::FrameContentSizeMismatch` — +/// the SAME error a Raw / RLE overshoot produces — instead of +/// panicking. The contract is uniform across all three block types: +/// "the frame's content exceeded its declared size". /// /// The INFALLIBLE entry points (`extend`, `extend_and_fill`, /// `extend_from_within_unchecked`) remain on the type as defense in @@ -1146,6 +1149,41 @@ mod tests { /// callee. Tests cover: short-literal + short-match, long /// literal (wildcopy tail), short-offset match (overlapCopy8 + /// 8-byte stride), long-offset match (wildcopy_no_overlap). + #[test] + fn exec_sequence_inline_overflow_returns_output_buffer_overflow() { + // #246: the donor-inline sequence executor must return + // `ExecuteSequencesError::OutputBufferOverflow` (NOT panic, NOT + // an out-of-bounds unsafe write) when a sequence's literal+match + // copy plus wildcopy overshoot would land past the fixed slice. + // This is the per-sequence guard that keeps the unsafe write + // surface inside the user slice on the way to the post-block FCS + // check; the higher-level acceptance test + // (`decode_all_compressed_block_fcs_overflow_returns_structured_error`) + // proves the whole chain folds into `FrameContentSizeMismatch`. + use super::super::errors::ExecuteSequencesError; + // Tiny slice: 16-byte capacity, tail already at 8. A sequence + // writing 8 literals + 8 match (16 bytes) plus the 15-byte + // wildcopy overshoot cannot fit in `16 - 8 = 8` remaining bytes. + let mut buf = vec![0u8; 16]; + for (i, slot) in buf.iter_mut().take(8).enumerate() { + *slot = i as u8; + } + let mut b = UserSliceBackend::from_slice(&mut buf); + b.tail = 8; + let lits = [0xAAu8; 16]; + // SAFETY: `lits` is a 16-byte parent buffer (donor 16-byte read + // slack satisfied); the call must return Err before any write + // past the slice end. + let err = unsafe { b.exec_sequence_inline(lits.as_ptr(), 8, 4, 8) } + .expect_err("overshoot must return OutputBufferOverflow"); + assert!( + matches!(err, ExecuteSequencesError::OutputBufferOverflow { .. }), + "expected OutputBufferOverflow, got {err:?}" + ); + // Backend left untouched on Err — tail not advanced. + assert_eq!(b.tail, 8, "tail must not advance on overflow"); + } + #[test] fn exec_sequence_inline_short_literal_plus_long_offset_match() { // Layout: pre-fill `tail = 8` with a "history" region so