Skip to content

Commit 0475405

Browse files
committed
fix(ldm): gate behind hash feature + harden insert_absolute preconditions
`encoding::ldm` was compiled unconditionally but depends on `twox_hash::XxHash64` for the per-window XXH64 (`zstd_ldm.c:315`). `twox-hash` is an optional dependency behind the `hash` feature, so `default-features = false` builds (no_std, embedded) failed to compile. The fix mirrors the gating pattern already used for the same crate in `streaming_encoder.rs`, `frame_compressor.rs`, and `decode_buffer.rs`: * `mod ldm;` declaration is `#[cfg(feature = "hash")]`. * `BtMatcher::ldm_producer` field and every integration callsite (`use ... LdmProducer`, `Some(producer) = self.ldm_producer ...`) carry the same gate. Under `default-features = false` the field disappears and `prepare_ldm_candidates` reduces to its legacy `ldm_sequences.clear()` stub. * `prepare_ldm_candidates_translates_absolute_positions_to_slice_ indices` regression also gated. Verified: `cargo build --lib --no-default-features` clean; `cargo clippy --lib --tests --no-default-features -- -D warnings` clean. Separately, `LdmHashTable::insert_absolute` promoted its two preconditions from `debug_assert!` to runtime panics so a contract violation cannot silently corrupt the table in release builds: * `abs_pos < position_base` now panics via `checked_sub` + `unwrap_or_else` with a diagnostic instead of underflowing the subtraction and casting the wraparound to `u32`. * `rel >= u32::MAX as usize` now panics via `assert!` (was `debug_assert!`) — the producer's `ensure_room_for` rebases before this point, but if a custom caller bypasses it the failure is loud and immediate. * The `+1` empty-slot bias was moved inside the `u32` arithmetic (`(rel as u32) + 1` instead of `(rel + 1) as u32`) so the intermediate `usize` add can't overflow on 32-bit targets. New `insert_absolute_panics_below_position_base` regression covers the underflow guard with `#[should_panic]`. README quick-start snippet now passes `&b"hello world"[..]` to `compress_to_vec` — `b"..."` literal alone is `&[u8; N]` which does not implement `std::io::Read`, so the doctest failed under `cargo test --doc` after the earlier doc rewrite landed. 469 lib tests + 11 doctests pass; clippy clean under default features AND under `--no-default-features`.
1 parent 201e7c5 commit 0475405

4 files changed

Lines changed: 69 additions & 6 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ structured-zstd = "0.0.20"
1717
```rust
1818
use structured_zstd::encoding::{compress_to_vec, CompressionLevel};
1919

20-
let compressed = compress_to_vec(b"hello world", CompressionLevel::from_level(7));
20+
let compressed = compress_to_vec(&b"hello world"[..], CompressionLevel::from_level(7));
2121
```
2222

2323
For `no_std` builds disable the default features:

zstd/src/encoding/bt/mod.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use alloc::vec::Vec;
2121

2222
use super::cost_model::{HC_MAX_LIT, HcOptState, HcOptimalCostProfile};
23+
#[cfg(feature = "hash")]
2324
use super::ldm::LdmProducer;
2425
use super::opt::ldm::{HcOptLdmState, HcRawSeq, HcRawSeqStore};
2526
use super::opt::types::{HcOptimalNode, HcOptimalPlanBuffers, HcOptimalSequence, MatchCandidate};
@@ -70,7 +71,13 @@ pub(crate) struct BtMatcher {
7071
/// `prepare_ldm_candidates` call for a frame. `None` while LDM
7172
/// is opt-out (current default) so the table allocation only
7273
/// happens for callers that opt in. See
73-
/// [`super::ldm::LdmProducer`].
74+
/// [`super::ldm::LdmProducer`]. Gated behind the `hash`
75+
/// feature because the producer's per-window XXH64 hashing
76+
/// depends on the optional `twox-hash` dependency; under
77+
/// `default-features = false` the field disappears and the
78+
/// `prepare_ldm_candidates` body shrinks to the legacy
79+
/// `ldm_sequences.clear()` stub.
80+
#[cfg(feature = "hash")]
7481
pub(crate) ldm_producer: Option<LdmProducer>,
7582
}
7683

@@ -123,6 +130,7 @@ impl BtMatcher {
123130
opt_ml_price_generation: Vec::new(),
124131
opt_ml_price_stamp: 0,
125132
ldm_sequences: Vec::new(),
133+
#[cfg(feature = "hash")]
126134
ldm_producer: None,
127135
}
128136
}
@@ -146,6 +154,7 @@ impl BtMatcher {
146154
self.opt_ml_price_generation.clear();
147155
self.opt_ml_price_stamp = 0;
148156
self.ldm_sequences.clear();
157+
#[cfg(feature = "hash")]
149158
if let Some(producer) = self.ldm_producer.as_mut() {
150159
producer.clear();
151160
}
@@ -359,6 +368,7 @@ impl BtMatcher {
359368
current_len: usize,
360369
) {
361370
self.ldm_sequences.clear();
371+
#[cfg(feature = "hash")]
362372
if let Some(producer) = self.ldm_producer.as_mut() {
363373
debug_assert!(current_abs_start >= history_abs_start);
364374
let block_end_abs = current_abs_start
@@ -372,6 +382,19 @@ impl BtMatcher {
372382
&mut self.ldm_sequences,
373383
);
374384
}
385+
#[cfg(not(feature = "hash"))]
386+
{
387+
// Under `default-features = false` (no `hash`),
388+
// `LdmProducer` is not compiled — `live_history` /
389+
// `history_abs_start` / `current_abs_start` /
390+
// `current_len` would otherwise be unused.
391+
let _ = (
392+
live_history,
393+
history_abs_start,
394+
current_abs_start,
395+
current_len,
396+
);
397+
}
375398
}
376399

377400
/// Donor parity: `ZSTD_storeSeq` — encode `actual_offset` into the
@@ -700,6 +723,7 @@ mod ldm_helper_tests {
700723
/// panic on the `history[block_start..block_end]` slice access
701724
/// inside `generate_into`.
702725
#[test]
726+
#[cfg(feature = "hash")]
703727
fn prepare_ldm_candidates_translates_absolute_positions_to_slice_indices() {
704728
use crate::encoding::ldm::{LdmProducer, params::LdmParams};
705729

zstd/src/encoding/ldm/table.rs

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -271,15 +271,31 @@ impl LdmHashTable {
271271
/// must call `ensure_room_for` before the first insert that
272272
/// could exceed the window.
273273
pub(crate) fn insert_absolute(&mut self, hash_id: u32, abs_pos: usize, checksum: u32) {
274-
debug_assert!(abs_pos >= self.position_base);
275-
let rel = abs_pos - self.position_base;
276-
debug_assert!(
274+
// Use runtime panics (rather than `debug_assert!`) for both
275+
// preconditions so a contract violation never silently
276+
// wraps in release: an unchecked `abs_pos - position_base`
277+
// would underflow and the subsequent `as u32` cast would
278+
// corrupt the table, hiding the bug far from its source.
279+
let rel = abs_pos.checked_sub(self.position_base).unwrap_or_else(|| {
280+
panic!(
281+
"insert position {abs_pos} is below position_base {}; \
282+
callers must rebase via `ensure_room_for` or clear before \
283+
reaching this state",
284+
self.position_base
285+
)
286+
});
287+
assert!(
277288
rel < u32::MAX as usize,
278289
"insert position {abs_pos} (rel {rel}) exceeds u32 window; \
279290
producer must call `ensure_room_for` before insert"
280291
);
281292
// +1 bias so 0 stays reserved for the empty-slot sentinel.
282-
let stored = (rel + 1) as u32;
293+
// The cast happens BEFORE the `+ 1` so the addition runs in
294+
// `u32` arithmetic — guarded by the `rel < u32::MAX` assert
295+
// above, the `+ 1` cannot overflow, and intermediate `usize`
296+
// values stay bounded on 32-bit targets where
297+
// `usize == u32`.
298+
let stored = (rel as u32) + 1;
283299
self.insert(
284300
hash_id,
285301
LdmEntry {
@@ -575,6 +591,20 @@ mod tests {
575591
assert_eq!(t.resolve(&t.bucket(2)[0]), Some(trigger_pos));
576592
}
577593

594+
/// `insert_absolute` must panic in BOTH debug and release
595+
/// builds when called with an `abs_pos` below the current
596+
/// `position_base` — silently underflowing the subtraction
597+
/// would store a wraparound relative offset and corrupt the
598+
/// table far from the bug's source. Regression for PR #139
599+
/// round-6 review (Copilot + CodeRabbit, Major).
600+
#[test]
601+
#[should_panic(expected = "below position_base")]
602+
fn insert_absolute_panics_below_position_base() {
603+
let mut t = LdmHashTable::new(4, 2);
604+
t.reduce(100); // shift position_base to 100
605+
t.insert_absolute(0, 50, 0xCAFE); // 50 < 100 → panic
606+
}
607+
578608
/// `clear()` must reset `position_base` to 0 so a subsequent
579609
/// frame can insert at any absolute position (including
580610
/// values below the previous frame's `position_base`)

zstd/src/encoding/mod.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,15 @@ pub(crate) mod bt;
4545
pub(crate) mod cost_model;
4646
pub(crate) mod dfast;
4747
pub(crate) mod hc;
48+
// LDM uses `twox_hash::XxHash64` (per-window XXH64 over the
49+
// `min_match_length` byte slice, donor `zstd_ldm.c:315`). The
50+
// `twox-hash` dependency is gated behind the `hash` feature so
51+
// `default-features = false` builds (no_std, embedded) don't pull
52+
// it in. The `BtMatcher::ldm_producer` field and every callsite
53+
// in `match_generator.rs` share the same `cfg(feature = "hash")`
54+
// gate so the codepath stays consistent across feature
55+
// combinations.
56+
#[cfg(feature = "hash")]
4857
pub(crate) mod ldm;
4958
pub(crate) mod match_table;
5059
pub(crate) mod opt;

0 commit comments

Comments
 (0)