diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b637a5615..26e5fd63d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -13,3 +13,27 @@ Pure Rust zstd implementation. Focus: dictionary compression improvements and pe - **Clippy:** Must pass `cargo clippy -p structured-zstd --features hash,std,dict_builder -- -D warnings` (`rustc-dep-of-std` is excluded — it's an internal feature for Rust stdlib builds only; `fuzz_exports` is excluded — fuzzing-specific entry points are validated separately from the regular lint gate) - Performance-critical code: benchmark before/after any changes - **Do not flag `use std::vec` (or `use alloc::vec`) as an unused import.** This crate is `no_std` with an `std` feature; the std prelude is not implicitly in scope, and the `vec![..]` macro resolves through the same path as the module. The import is required wherever the macro is used. Removing it fails the build with `cannot find macro 'vec' in this scope`. + +## no-std-First Design Rules + +**`structured-zstd` MUST compile and work under `no_std + alloc`.** These rules govern all library code (`src/lib.rs` and its submodules). They are a flat contract — there are no opt-out crate tiers and no per-crate carve-outs; every primitive choice is judged by the rules below. + +1. The crate MUST support `no_std + alloc` builds. This is both a CI gate and a design constraint applied during review. +2. Choose primitives in this order: `core::*` → `alloc::*` → external `no_std + alloc` crate → `std::*` behind `#[cfg(feature = "std")]` → unconditional `std::*` (last resort, requires explicit justification in the PR description). +3. `Cargo.toml` MUST expose a `std` feature in `[features]` and include it in `default` so that `--no-default-features` produces a `no_std` build. `src/lib.rs` MUST open with `#![cfg_attr(not(feature = "std"), no_std)]` and (when the crate uses heap types) `extern crate alloc;`. A separate `alloc` feature is OPTIONAL — this crate uses `extern crate alloc;` unconditionally and does not gate it behind a feature, because `alloc` is always required. Other crates that need to support pure-`core` no-alloc builds MAY add an `alloc` feature and gate `extern crate alloc;` on it. +4. CI MUST include a `no-std-check` job that builds with `--no-default-features` (and any feature combinations that should stay no-std-clean, e.g. `--features hash`) and treats warnings as errors. The strongest variant uses a no-std-only target (e.g. `thumbv7em-none-eabihf`) to catch transitive `std` leaks at the linker; a `clippy -D warnings` run on the host target against `--no-default-features` is acceptable provided the crate has no transitive deps that smuggle `std::*` re-exports through unused-import lints. This crate uses the host-clippy variant; future crates SHOULD prefer the embedded target unless they have a documented reason to deviate. +5. Public API surface (traits, function signatures, returned types) MUST use only `core` / `alloc` types. `std::*` in implementation modules is allowed only when gated behind `#[cfg(feature = "std")]` AND only when the public surface remains alloc-friendly. +6. `std::collections::HashMap` / `HashSet` are FORBIDDEN. Use `hashbrown::HashMap` / `HashSet`, or `rustc_hash::FxHashMap` for internal-ID keys. +7. `std::sync::Mutex` / `RwLock` in new code requires explicit justification. Prefer `parking_lot::Mutex` / `RwLock` on hot paths in std-gated code. Use `spin::Mutex` for genuinely no-std code paths and only for very short critical sections. +8. `std::sync::OnceLock` is FORBIDDEN for fallible init. Use `once_cell::sync::OnceCell::get_or_try_init` or `once_cell::race::OnceBox`. +9. `thread_local!` is FORBIDDEN in library code. Replace with caller-managed scratch parameters or atomic-pointer patterns. +10. `std::io::Error` MUST NOT appear in public APIs. Define a crate-local error enum; `From` impls live behind `#[cfg(feature = "std")]`. +11. `std::time::Instant` and `std::time::SystemTime` MUST NOT appear in public APIs. Use a caller-provided clock trait or a `#[cfg(feature = "std")]`-gated convenience wrapper. +12. `std::thread::*` is FORBIDDEN in library code. Threading must be the caller's responsibility. +13. `core::*` MUST be used in source whenever the type is available there. `std::*` re-exports of `core` types (e.g. `std::sync::atomic::AtomicU64`) are forbidden — they break the build for no-std targets even when binary-identical for std targets. +14. Performance does NOT excuse picking `std::*` over a faster `no_std`-ready primitive. Community alternatives (`hashbrown`, `parking_lot`, `smallvec`, `rustc_hash`, `bytes`) are normally faster than their `std::*` counterparts on hot paths — pick them on merit, not as a fallback. +15. Adding a new unconditional `use std::*` to library code is a one-way ratchet. Reviewers MUST reject such additions unless the PR explicitly justifies why the std primitive cannot be replaced by a no-std-ready alternative or hidden behind `#[cfg(feature = "std")]`. +16. The `no-std-check` CI job MUST stay green (or, if in transition with `continue-on-error: true`, its compile-error count MUST be monotonically non-decreasing per PR — every PR either keeps the count flat or reduces it; raising it is a regression and blocks merge). +17. Test code (`#[cfg(test)]`), benches (`benches/`), and binaries (`src/bin/`) MAY use `std::*` freely. Only library code in `src/lib.rs` and its submodules is governed by these rules. +18. Doc comments and rustdoc `# Examples` blocks on `no_std`-capable APIs MUST NOT depend on `std::*` types if the API itself does not. Doctest examples requiring `std` MUST be gated `#[cfg(feature = "std")]`. +19. Adding a transitive dependency that pulls `std` into the otherwise no-std-clean build counts as a violation of rule 15 and requires the same approval path. diff --git a/zstd/src/decoding/errors.rs b/zstd/src/decoding/errors.rs index 1a029704e..d8b33eaee 100644 --- a/zstd/src/decoding/errors.rs +++ b/zstd/src/decoding/errors.rs @@ -1001,6 +1001,15 @@ impl From for FSETableError { pub enum FSEDecoderError { GetBitsError(GetBitsError), TableIsUninitialized, + /// Externally constructed `FSETable` violates the + /// `decode.len() == 1 << accuracy_log` shape invariant. Only + /// reachable under `feature = "fuzz_exports"`, where fuzz + /// harnesses can set the `FSETable.decode` / `accuracy_log` + /// fields directly and skip `build_decoding_table`. + InvalidTableShape { + decode_len: usize, + accuracy_log: u8, + }, } #[cfg(feature = "std")] @@ -1020,6 +1029,19 @@ impl core::fmt::Display for FSEDecoderError { FSEDecoderError::TableIsUninitialized => { write!(f, "Tried to use an uninitialized table!") } + FSEDecoderError::InvalidTableShape { + decode_len, + accuracy_log, + } => match 1usize.checked_shl((*accuracy_log).into()) { + Some(expected) => write!( + f, + "FSETable shape invariant violated: decode.len() = {decode_len}, expected 1 << accuracy_log = {expected} (accuracy_log = {accuracy_log})", + ), + None => write!( + f, + "FSETable shape invariant violated: decode.len() = {decode_len}, accuracy_log = {accuracy_log} overflows 1 << accuracy_log for usize", + ), + }, } } } diff --git a/zstd/src/fse/fse_decoder.rs b/zstd/src/fse/fse_decoder.rs index e76c0c2d8..abb3ee598 100644 --- a/zstd/src/fse/fse_decoder.rs +++ b/zstd/src/fse/fse_decoder.rs @@ -34,28 +34,153 @@ impl<'t> FSEDecoder<'t> { if self.table.accuracy_log == 0 { return Err(FSEDecoderError::TableIsUninitialized); } + // Externally-constructible-table guard: when + // `feature = "fuzz_exports"` is on, `FSETable.decode` / + // `FSETable.accuracy_log` are settable from outside the crate, + // so a fuzz harness can hand the decoder a mis-shaped table + // that skips `build_decoding_table`'s invariants. Validate the + // table-shape invariant `decode.len() == 1 << accuracy_log` + // up-front and surface as a typed `InvalidTableShape` error + // (distinct from `TableIsUninitialized` to keep fuzz triage + // unambiguous) — without this, `read_entry`'s bounds-checked + // indexing under the same cfg would panic on a malformed + // table, which fuzz harnesses cannot distinguish from a + // legitimate decoder failure. `checked_shl` covers the + // pathological case where `accuracy_log >= usize::BITS`. + #[cfg(feature = "fuzz_exports")] + { + let accuracy_log = self.table.accuracy_log; + let decode_len = self.table.decode.len(); + let expected = 1usize.checked_shl(accuracy_log.into()).ok_or( + FSEDecoderError::InvalidTableShape { + decode_len, + accuracy_log, + }, + )?; + if decode_len != expected { + return Err(FSEDecoderError::InvalidTableShape { + decode_len, + accuracy_log, + }); + } + } let new_state = bits.get_bits(self.table.accuracy_log); - self.state = self.table.decode[new_state as usize]; + // SAFETY: `accuracy_log` bits read from the bitstream produce + // `new_state < (1 << accuracy_log) = table_size = decode.len()`. + // `build_decoding_table` ensures the table is sized exactly + // `1 << accuracy_log` entries. The bounds check that the + // checked indexing would emit is provably redundant. Under + // `feature = "fuzz_exports"` `read_entry` falls back to the + // bounds-checked path — see comment on `read_entry`. + self.state = self.read_entry(new_state as usize); Ok(()) } /// Advance the internal state to decode the next symbol in the bitstream. + /// + /// # Panics + /// + /// Panics if called on an `FSEDecoder` whose backing `FSETable` has + /// not been built yet (empty `decode` vec). `FSEDecoder::new` + /// produces such a decoder with a zero-default `state`; the + /// well-behaved pipeline is `new` → `init_state` → `update_state*`, + /// and `init_state` returns `Err` on an uninitialized table. This + /// assertion converts what would otherwise be UB (from the + /// unchecked indexing in `read_entry`) into a clear fail-fast + /// panic that surfaces the API misuse immediately instead of + /// leaving the bitstream and decode state silently desynchronised. pub fn update_state(&mut self, bits: &mut BitReaderReversed<'_>) { + // Public-API safety guard: `FSEDecoder::new` builds a decoder + // with a zero-default `state` (Entry { new_state: 0, num_bits: + // 0, symbol: 0 }) regardless of whether the table was actually + // populated. A caller that constructs the decoder and then + // calls `update_state` BEFORE a successful `init_state` would + // hit `read_entry(0)` → `get_unchecked(0)` on an empty + // `decode` vec — UB in release mode, since `debug_assert!` is + // stripped. Fail-fast with `assert!` instead of silently + // returning so that misuse surfaces immediately rather than + // leaving the bitstream advanced by some bits but the decode + // state stuck at the zero-default Entry — a corruption mode + // that the caller has no way to diagnose. The well-behaved + // decode pipeline always pairs `new` → `init_state` → + // `update_state*`, so this branch is strongly biased "not + // taken" and the predictor amortises it to zero cost on the + // hot path. The corresponding `update_state_fast` is + // `pub(crate)` with controlled callers, so it relies on the + // documented precondition instead of paying for a per-call + // check. + assert!( + !self.table.decode.is_empty(), + concat!( + "FSEDecoder::update_state called on an uninitialized table; ", + "call init_state successfully before any update_state* call", + ), + ); let num_bits = self.state.num_bits; let add = bits.get_bits(num_bits); let next_state = usize::from(self.state.new_state) + add as usize; - self.state = self.table.decode[next_state]; + // SAFETY: same invariant as `update_state_fast` below — + // `new_state` and `num_bits` were paired by + // `calc_baseline_and_numbits` during table construction such + // that `new_state + (1 << num_bits) - 1 < table_size = + // decode.len()`. `add < 1 << num_bits` by definition of the + // `num_bits`-wide read, so `next_state < decode.len()`. + self.state = self.read_entry(next_state); + } + + /// Read `decode[idx]` — bounds-checked under `fuzz_exports`, unchecked + /// otherwise. The call sites all hold the FSE invariant `idx < + /// decode.len()` by construction (`init_state` reads + /// `accuracy_log` bits, `update_state*` derive `next_state` from + /// `Entry.new_state + add` where `calc_baseline_and_numbits` + /// guarantees `new_state + (1 << num_bits) - 1 < table_size`). + /// Under `fuzz_exports` external code can construct a mis-shaped + /// table that violates the invariant — fall back to checked + /// indexing so a fuzz harness sees a panic rather than UB, even + /// when the fuzz binary is built in release mode (which makes + /// `debug_assert!` a no-op and is the default for `cargo fuzz`). + #[inline(always)] + fn read_entry(&self, idx: usize) -> Entry { + #[cfg(feature = "fuzz_exports")] + { + self.table.decode[idx] + } + #[cfg(not(feature = "fuzz_exports"))] + // SAFETY: see comments at the individual call sites — `idx` is + // invariant-bounded by the FSE table-build / state-transition + // contract. LLVM cannot prove this on its own because the + // invariant spans `build_decoding_table` and decode call sites. + unsafe { + *self.table.decode.get_unchecked(idx) + } } /// Advance the internal state **without** an individual refill check. /// - /// The caller **must** guarantee that enough bits are available in the bit - /// reader (e.g. via [`BitReaderReversed::ensure_bits`] with a budget that - /// covers this and any other unchecked reads in the same batch). + /// # Preconditions (caller-enforced) + /// + /// 1. **Bit budget:** enough bits MUST be available in the bit + /// reader (e.g. via [`BitReaderReversed::ensure_bits`] with a + /// budget that covers this and any other unchecked reads in the + /// same batch). + /// 2. **State initialisation:** [`init_state`] MUST have returned + /// `Ok` on this decoder before any `update_state_fast` call. + /// Calling `update_state_fast` on a fresh `FSEDecoder::new` + /// output (which holds a zero-default `state` and may reference + /// an empty `decode` vec) would resolve to + /// `read_entry(0).get_unchecked(0)` on an empty slice — UB. + /// The empty-table guard in [`update_state`] is intentionally + /// omitted here to keep the per-sequence fast path branch-free; + /// the only call site (`decode_and_execute_sequences`) always + /// succeeds `init_state` before entering the per-sequence loop, + /// so the precondition holds by construction. /// /// This is the "fast path" used in the interleaved sequence decode loop /// where a single refill check covers all three FSE state updates. + /// + /// [`init_state`]: Self::init_state + /// [`update_state`]: Self::update_state #[inline(always)] pub(crate) fn update_state_fast(&mut self, bits: &mut BitReaderReversed<'_>) { let num_bits = self.state.num_bits; @@ -68,8 +193,10 @@ impl<'t> FSEDecoder<'t> { // `add < 2.pow(num_bits)` by construction of `BitReaderReversed::get_bits_unchecked`. // Therefore `next_state < self.table.decode.len()` and the indexed read // is in bounds; LLVM cannot prove this invariant on its own because it - // spans the table-build and decode call sites. - self.state = unsafe { *self.table.decode.get_unchecked(next_state) }; + // spans the table-build and decode call sites. Under + // `feature = "fuzz_exports"` `read_entry` falls back to bounds-checked + // indexing — see comment on `read_entry`. + self.state = self.read_entry(next_state); } }