Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/amiga_lzx/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand Down
11 changes: 10 additions & 1 deletion src/gzip/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion src/lz4/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/lz4/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/lzo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/lzx/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/rar5/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
8 changes: 7 additions & 1 deletion src/xz/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand Down
10 changes: 8 additions & 2 deletions src/zstd/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand All @@ -628,7 +634,7 @@ impl RawDecoder for Decoder {
return Ok(RawProgress {
consumed,
written,
done: false,
done: matches!(self.phase, DecPhase::Done),
});
}
}
Expand Down
58 changes: 58 additions & 0 deletions tests/rar5.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}
148 changes: 148 additions & 0 deletions tests/terminal_state.rs
Original file line number Diff line number Diff line change
@@ -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<u8> {
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<A: compcol::Algorithm>(name: &str) {
use compcol::{Decoder as _, Status};

let plain = payload();
let encoded = compcol::vec::compress_to_vec::<A>(&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");
Loading