From 8ecc1a979a34f2d807a0ca67923e3cf1d2ca760a Mon Sep 17 00:00:00 2001 From: Mark Karpeles Date: Mon, 17 Aug 2026 06:24:10 +0900 Subject: [PATCH] fix(decoders): report completion from terminal states instead of stalling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #123. That fix covered deflate/deflate64/zlib; reviewing all 53 `raw_decode` implementations found the same defect in seven more. A decoder parked in its terminal state that returns `done: false` with nothing consumed and nothing written, while the caller still holds input, is mapped by the RawDecoder->Decoder bridge to `Status::OutputFull` — "call me again" — with nothing to progress on. Any loop waiting for StreamEnd spins: CPU-bound, no allocation, so no output or memory cap catches it. Fixed, each verified by reading its state machine and confirmed with a probe that drives the real encoder output plus trailing bytes: - zstd, xz Done arms returned false; both are terminal (neither supports concatenated frames/streams). - lz4 block, lzo Done after the zero-length terminator block. - lz4 frame only reachable by calling past StreamEnd, but the state is terminal either way. - lzx, amiga_lzx Done arms returned false. - rar5 returns on `State::Done` *before* the block that accepts caller bytes, so a container passing the next header after the payload could never make progress. - gzip subtler: the `BetweenMembers` arm sets `phase = Done` intending to reach the Done arm that swallows trailing bytes, but match arms do not fall through, so the loop's no-progress check returned first and the swallow never ran. Needs an explicit `continue`, like the 0x1F path. Two call shapes matter and only the first was covered before: trailing bytes in the same call, and trailing bytes arriving *after* the payload completes. The second is what a container does when it hands over the next header, and it is what xz and gzip failed — both looked safe under the first shape. Codecs that only ever report `InputEmpty` and produce output on `finish` (brotli, lzma, and the other buffering decoders) are correct as-is: the trait permits that, and it terminates. The invariant asserted is not "must report StreamEnd" but "must never report OutputFull with zero progress". A sweep over all 50 round-trippable algorithms now reports no stalls. tests/terminal_state.rs covers each fixed codec plus controls; rar5 is decoder-only so it gets equivalent tests against its own fixture. --- src/amiga_lzx/decoder.rs | 4 +- src/gzip/mod.rs | 11 ++- src/lz4/frame.rs | 6 +- src/lz4/mod.rs | 7 +- src/lzo/mod.rs | 5 +- src/lzx/decoder.rs | 6 +- src/rar5/decoder.rs | 7 +- src/xz/mod.rs | 8 ++- src/zstd/decoder.rs | 10 ++- tests/rar5.rs | 58 +++++++++++++++ tests/terminal_state.rs | 148 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 260 insertions(+), 10 deletions(-) create mode 100644 tests/terminal_state.rs diff --git a/src/amiga_lzx/decoder.rs b/src/amiga_lzx/decoder.rs index 83327e2..9fa17c4 100644 --- a/src/amiga_lzx/decoder.rs +++ b/src/amiga_lzx/decoder.rs @@ -314,10 +314,12 @@ impl RawDecoder for Decoder { } DecState::Done => { + // Terminal: report completion so the bridge yields + // StreamEnd. See the matching note in `lzx`. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } diff --git a/src/gzip/mod.rs b/src/gzip/mod.rs index d28ffb9..3807e3c 100644 --- a/src/gzip/mod.rs +++ b/src/gzip/mod.rs @@ -465,12 +465,21 @@ impl RawDecoder for Decoder { continue; } // Anything other than the gzip magic means the - // stream ended. Fall through to Done, which will + // stream ended. Hand off to Done, which will // silently swallow the trailing bytes — gzip(1) // does the same (the input could be a concatenated // gzip+something-else file, and decoders are // expected to be permissive). + // + // `continue` is required: match arms do not fall + // through, so without it the no-progress check at the + // bottom of the loop returns first (this iteration + // consumed nothing) and the Done arm never runs. That + // left a caller holding trailing bytes getting + // `OutputFull` with no progress forever — exactly the + // spin the Done arm exists to prevent. self.phase = DecPhase::Done; + continue; } DecPhase::Done => { // Swallow any trailing bytes the caller still has diff --git a/src/lz4/frame.rs b/src/lz4/frame.rs index c3d0b19..3a640a4 100644 --- a/src/lz4/frame.rs +++ b/src/lz4/frame.rs @@ -1156,10 +1156,14 @@ impl RawDecoder for Decoder { }); } DecPhase::Done => { + // Only reachable if the caller keeps calling after the + // StreamEnd reported when the frame completed above, but + // the state is terminal either way — report it as such + // rather than as "call me again with no progress". return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } diff --git a/src/lz4/mod.rs b/src/lz4/mod.rs index 1f70c56..a8ddcc2 100644 --- a/src/lz4/mod.rs +++ b/src/lz4/mod.rs @@ -465,10 +465,15 @@ impl RawDecoder for Decoder { } } DecPhase::Done => { + // Terminal (a zero-length block terminates the stream), so + // report completion and let the bridge yield StreamEnd. + // Reporting `false` left a stream with trailing bytes + // returning OutputFull with no progress, spinning any + // caller that loops until StreamEnd. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } diff --git a/src/lzo/mod.rs b/src/lzo/mod.rs index 8b8f355..2724afe 100644 --- a/src/lzo/mod.rs +++ b/src/lzo/mod.rs @@ -396,10 +396,13 @@ impl RawDecoder for Decoder { } } DecPhase::Done => { + // Terminal (a zero-length block terminates the stream), so + // report completion and let the bridge yield StreamEnd. + // See the matching note in `lz4`. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } diff --git a/src/lzx/decoder.rs b/src/lzx/decoder.rs index e45fe9e..d65a9b9 100644 --- a/src/lzx/decoder.rs +++ b/src/lzx/decoder.rs @@ -347,10 +347,14 @@ impl RawDecoder for Decoder { } DecState::Done => { + // Terminal: report completion so the bridge yields + // StreamEnd. Reporting `false` left a stream with + // trailing bytes returning OutputFull with nothing + // consumed and nothing written, spinning the caller. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } diff --git a/src/rar5/decoder.rs b/src/rar5/decoder.rs index 0cb0981..a5b40a7 100644 --- a/src/rar5/decoder.rs +++ b/src/rar5/decoder.rs @@ -256,10 +256,15 @@ impl RawDecoder for Decoder { self.state = State::Done; } if matches!(self.state, State::Done) { + // Terminal: report completion so the bridge yields StreamEnd. + // This returns before the "accept more bytes" block below, so + // reporting `false` left a container that passes trailing + // bytes (the next header) getting OutputFull with nothing + // consumed and nothing written — an unbreakable caller loop. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } if written == output.len() && !self.ready.is_empty() { diff --git a/src/xz/mod.rs b/src/xz/mod.rs index ac866fb..ea64b72 100644 --- a/src/xz/mod.rs +++ b/src/xz/mod.rs @@ -1746,10 +1746,16 @@ impl RawDecoder for Decoder { } } DecPhase::Done => { + // Terminal (Done follows the Stream Footer; concatenated + // streams are not supported). Report completion so the + // bridge yields StreamEnd. Reporting `false` left a caller + // that hands over trailing bytes after the footer getting + // `OutputFull` with nothing consumed and nothing written, + // spinning on unchanged state. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } diff --git a/src/zstd/decoder.rs b/src/zstd/decoder.rs index e76967d..f07d021 100644 --- a/src/zstd/decoder.rs +++ b/src/zstd/decoder.rs @@ -616,10 +616,16 @@ impl RawDecoder for Decoder { self.phase = DecPhase::Done; } DecPhase::Done => { + // Report completion so the bridge yields + // `Status::StreamEnd`. Concatenated frames are not + // supported (see the module docs), so this is terminal. + // Reporting `false` here left a frame with trailing bytes + // returning `OutputFull` with nothing consumed and nothing + // written, spinning any caller that loops until StreamEnd. return Ok(RawProgress { consumed, written, - done: false, + done: true, }); } } @@ -628,7 +634,7 @@ impl RawDecoder for Decoder { return Ok(RawProgress { consumed, written, - done: false, + done: matches!(self.phase, DecPhase::Done), }); } } diff --git a/tests/rar5.rs b/tests/rar5.rs index 49e6ae0..5b58eec 100644 --- a/tests/rar5.rs +++ b/tests/rar5.rs @@ -520,3 +520,61 @@ mod factory { ); } } + +// ─── regression: no-progress stall at the terminal state ──────────────── + +/// A completed rar5 stream must report `StreamEnd`, not `OutputFull` with +/// nothing consumed and nothing written. +/// +/// `raw_decode` returns as soon as `State::Done` is reached, *before* the +/// block that accepts more caller bytes, so reporting `done: false` there +/// left a container that hands over trailing bytes (the next file header of +/// a solid group, say) receiving `OutputFull` — "call me again" — with +/// nothing to make progress on. A caller looping until `StreamEnd` then +/// spun on unchanged state: CPU-bound, flat RSS, uninterruptible from +/// outside the decoder. +#[test] +fn completed_stream_reports_stream_end_not_a_stall() { + let mut dec = Decoder::with_unpack_size_and_window(FIXTURE_AAA_UNPACK, 128 * 1024); + let mut out = vec![0u8; FIXTURE_AAA_UNPACK as usize + 64]; + let mut total = 0usize; + let mut saw_end = false; + for _ in 0..64 { + let (p, status) = dec.decode(FIXTURE_AAA, &mut out[total..]).unwrap(); + total += p.written; + if matches!(status, Status::StreamEnd) { + saw_end = true; + break; + } + assert!( + !(p.consumed == 0 && p.written == 0 && status == Status::OutputFull), + "decoder asked to be called again without making progress" + ); + } + assert!(saw_end, "a fully decoded stream must report StreamEnd"); + assert_eq!(total, FIXTURE_AAA_UNPACK as usize); +} + +/// The same, with trailing bytes arriving in a *later* call — the shape a +/// container produces when it passes the next header after the payload. +#[test] +fn trailing_bytes_after_completion_do_not_stall() { + let mut dec = Decoder::with_unpack_size_and_window(FIXTURE_AAA_UNPACK, 128 * 1024); + let mut out = vec![0u8; FIXTURE_AAA_UNPACK as usize + 64]; + let mut total = 0usize; + for _ in 0..64 { + let (p, status) = dec.decode(FIXTURE_AAA, &mut out[total..]).unwrap(); + total += p.written; + if matches!(status, Status::StreamEnd) { + break; + } + if p.consumed == 0 && p.written == 0 { + break; + } + } + let (p, status) = dec.decode(&[0xAA; 32], &mut out).unwrap(); + assert!( + !(p.consumed == 0 && p.written == 0 && status == Status::OutputFull), + "trailing bytes after completion must not produce a no-progress OutputFull" + ); +} diff --git a/tests/terminal_state.rs b/tests/terminal_state.rs new file mode 100644 index 0000000..57338db --- /dev/null +++ b/tests/terminal_state.rs @@ -0,0 +1,148 @@ +//! Regression tests for the "no-progress stall" class of bug. +//! +//! A decoder that has reached its terminal state must say so. If it returns +//! `done: false` with nothing consumed and nothing written while the caller +//! still holds input, the `RawDecoder`->`Decoder` bridge maps that to +//! `Status::OutputFull` — "drain and call me again" — even though calling +//! again cannot make progress. A caller looping until `Status::StreamEnd` +//! (`vec::decompress_to_vec` among them) then spins on unchanged state: +//! CPU-bound, no allocation, so neither an output-size cap nor a memory cap +//! can stop it. Only a wall-clock timeout ends it, and nothing outside the +//! decoder can prevent it. +//! +//! Two shapes are checked per codec, because they exercise different paths: +//! +//! 1. **trailing bytes present in the same call** — the terminal state is +//! reached with input still unread. +//! 2. **trailing bytes arriving in a later call** — the decoder is already +//! parked in its terminal state when the next slice shows up. This is +//! what a container does when it hands over the next header after the +//! payload, and it is the shape that survived the first round of fixes. +//! +//! Each codec below had this verified by reading its `raw_decode`; the tests +//! keep it from regressing. + +#![cfg(feature = "alloc")] + +#[allow(dead_code)] +fn payload() -> Vec { + b"hello hello hello world world 1234567890 abcabcabc".repeat(20) +} + +/// Drive `dec` over `encoded` to completion, then assert that the codec +/// neither stalls on trailing bytes supplied in the same call nor on bytes +/// handed over afterwards. +#[allow(dead_code)] +fn assert_no_stall(name: &str) { + use compcol::{Decoder as _, Status}; + + let plain = payload(); + let encoded = compcol::vec::compress_to_vec::(&plain) + .unwrap_or_else(|e| panic!("{name}: encode failed: {e:?}")); + + // Shape 1: trailing garbage in the same buffer. + { + let mut stream = encoded.clone(); + stream.extend_from_slice(&[0xAA; 32]); + let mut dec = A::decoder(); + let mut out = vec![0u8; 1 << 16]; + let mut consumed = 0usize; + let mut total = 0usize; + for _ in 0..256 { + let (p, status) = dec.decode(&stream[consumed..], &mut out).unwrap(); + consumed += p.consumed; + total += p.written; + if matches!(status, Status::StreamEnd) { + break; + } + // The safety property. `StreamEnd` and `InputEmpty` are both fine + // terminations — a decoder that buffers input and produces on + // `finish` legitimately never reports `StreamEnd` from `decode`. + // What must never happen is `OutputFull` ("call me again") with + // nothing consumed and nothing written, which cannot terminate. + assert!( + !(p.consumed == 0 && p.written == 0 && status == Status::OutputFull), + "{name}: asked to be called again without making progress \ + (trailing bytes in the same call)" + ); + if p.consumed == 0 && p.written == 0 { + break; + } + } + assert_eq!(total, plain.len(), "{name}: wrong decoded length"); + } + + // Shape 2: the trailing bytes arrive only after the payload is done. + { + let mut dec = A::decoder(); + let mut out = vec![0u8; 1 << 16]; + let mut consumed = 0usize; + for _ in 0..256 { + let (p, status) = dec.decode(&encoded[consumed..], &mut out).unwrap(); + consumed += p.consumed; + if matches!(status, Status::StreamEnd) || (p.consumed == 0 && p.written == 0) { + break; + } + } + let (p, status) = dec.decode(&[0xAA; 32], &mut out).unwrap(); + assert!( + !(p.consumed == 0 && p.written == 0 && status == Status::OutputFull), + "{name}: asked to be called again without making progress \ + (trailing bytes delivered after completion)" + ); + } +} + +macro_rules! case { + ($feat:literal, $test:ident, $ty:path, $name:literal) => { + #[cfg(feature = $feat)] + #[test] + fn $test() { + assert_no_stall::<$ty>($name); + } + }; +} + +// Codecs whose terminal state reported `done: false`; all fixed. +case!("zstd", zstd_no_stall, compcol::zstd::Zstd, "zstd"); +case!("xz", xz_no_stall, compcol::xz::Xz, "xz"); +case!("lz4", lz4_no_stall, compcol::lz4::Lz4, "lz4"); +case!( + "lz4", + lz4_frame_no_stall, + compcol::lz4::frame::LZ4Frame, + "lz4frame" +); +case!("lzo", lzo_no_stall, compcol::lzo::Lzo, "lzo"); +case!("lzx", lzx_no_stall, compcol::lzx::Lzx, "lzx"); +case!( + "amiga_lzx", + amiga_lzx_no_stall, + compcol::amiga_lzx::AmigaLzx, + "amiga_lzx" +); +// gzip's Done arm swallows trailing input, but the `BetweenMembers` arm that +// hands off to it needs an explicit `continue` — match arms do not fall +// through, so without it the loop's no-progress check returned first. +case!("gzip", gzip_no_stall, compcol::gzip::Gzip, "gzip"); +// Fixed earlier alongside the reported DEFLATE/zlib DoS; kept here so the +// whole class is covered in one place. +case!( + "deflate", + deflate_no_stall, + compcol::deflate::Deflate, + "deflate" +); +case!( + "deflate64", + deflate64_no_stall, + compcol::deflate64::Deflate64, + "deflate64" +); +case!("zlib", zlib_no_stall, compcol::zlib::Zlib, "zlib"); + +// Controls: these already reported completion correctly. They guard against a +// future refactor breaking the codecs that were right all along. +case!("bzip2", bzip2_no_stall, compcol::bzip2::Bzip2, "bzip2"); +case!("brotli", brotli_no_stall, compcol::brotli::Brotli, "brotli"); +case!("lzma", lzma_no_stall, compcol::lzma::Lzma, "lzma");