From 6bb98b752fa1dccb8f55bf97d38ff57f4af9b2e0 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 16:42:53 +0300 Subject: [PATCH 1/7] fix(fse): use spare_capacity_mut + MaybeUninit instead of premature set_len MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot flagged the prior P2 change as UB: `set_len(table_size)` before the write loop made `self.decode` logically contain `table_size` initialised entries, but they were uninitialised memory. The error path (`nb > accuracy_log`) returned with that broken state, and the outer `reset()`/Drop would have run on uninit entries — unsound per the Vec contract. Fix: write via `spare_capacity_mut()` (typed `&mut [MaybeUninit]`) and call `set_len(table_size)` only AFTER the loop completes. Error path returns with `decode.len() == 0` (set by the preceding `clear()`), so no uninitialised entry is observable. The hot-loop codegen is unchanged — `MaybeUninit::write` lowers to the same strided store sequence the raw pointer version was emitting; the soundness is now machine-checkable. `E: FseEntry` has no `Drop` so the dropped `MaybeUninit` slice on the error path is a no-op. --- zstd/src/fse/fse_decoder.rs | 53 ++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/zstd/src/fse/fse_decoder.rs b/zstd/src/fse/fse_decoder.rs index 8656b4cea..158e245c0 100644 --- a/zstd/src/fse/fse_decoder.rs +++ b/zstd/src/fse/fse_decoder.rs @@ -473,13 +473,15 @@ impl FSETableImpl { // table AND initialise `symbol_next` for them. Donor: // `tableDecode[highThreshold--].baseValue = s; symbolNext[s] // = 1;`. + // + // Index loop (not `iter().enumerate().take()`) — LLVM emits + // a tighter scalar loop without the Iterator::next state + // machine. The enumerate+take iterator chain was visible as + // ~1.8% combined self-time on the decode flamegraph. + let probs = self.symbol_probabilities.as_slice(); let mut negative_idx = table_size; - for (symbol, &prob) in self - .symbol_probabilities - .iter() - .enumerate() - .take(nb_symbols) - { + for symbol in 0..nb_symbols { + let prob = probs[symbol]; if prob == -1 { negative_idx -= 1; spread[negative_idx] = symbol as u8; @@ -493,12 +495,8 @@ impl FSETableImpl { // build loop's counter reaches `2*prob - 1` over `prob` // iterations (matching donor `symbolNext[s]++` semantics). let mut position = 0usize; - for (symbol, &prob) in self - .symbol_probabilities - .iter() - .enumerate() - .take(nb_symbols) - { + for symbol in 0..nb_symbols { + let prob = probs[symbol]; if prob <= 0 { continue; } @@ -532,9 +530,20 @@ impl FSETableImpl { // shape. let accuracy_log = self.accuracy_log; let table_size_u32 = table_size as u32; + // Write entries into `spare_capacity_mut()` (typed as + // `&mut [MaybeUninit]`) and only `set_len` AFTER all + // writes complete. This keeps the per-push bookkeeping out + // of the hot loop (the body becomes a flat strided + // `MaybeUninit::write` sequence) while staying sound: the + // `set_len` call only ever runs when every slot in + // 0..table_size is initialised. The error path returns Err + // with `decode.len() == 0` (the preceding `clear()`), + // exposing zero uninitialised entries. self.decode.clear(); self.decode.reserve(table_size); - for &symbol in spread.iter().take(table_size) { + let slots = &mut self.decode.spare_capacity_mut()[..table_size]; + + for (state_idx, &symbol) in spread.iter().take(table_size).enumerate() { let next_state = symbol_next[symbol as usize]; // `next_state >= 1` by construction: upstream // `read_probabilities` / `build_from_probabilities` @@ -567,6 +576,12 @@ impl FSETableImpl { // high_bit > accuracy_log + 1 and wrap `nb` to a large // u8. Reject so the unchecked indexing contract holds. if nb > accuracy_log { + // `decode.len()` is still 0 (set by `clear()` above) — + // no `set_len` ran, so no uninitialised entry is + // observable to the outer `build_decoding_table`'s + // `reset()` path. The partially-filled `slots` buffer + // is dropped here harmlessly (`MaybeUninit` has no + // Drop). return Err(FSETableError::TableInvariantViolation { prob: self.symbol_probabilities[symbol as usize], symbol, @@ -585,8 +600,16 @@ impl FSETableImpl { // formula bug instead of silently producing a // malformed entry. let new_state_u32 = (next_state << nb) - table_size_u32; - self.decode - .push(E::from_raw(new_state_u32 as u16, symbol, nb)); + slots[state_idx].write(E::from_raw(new_state_u32 as u16, symbol, nb)); + } + + // SAFETY: the loop above ran `table_size` iterations and + // wrote `slots[state_idx]` for every `state_idx ∈ + // [0, table_size)`. `reserve(table_size)` guaranteed capacity. + // Every Err exit happens BEFORE this `set_len`, so we only + // claim initialisation when every slot has been written. + unsafe { + self.decode.set_len(table_size); } Ok(()) From fafdad919e6d13066e703924a1b254c5f411a909 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 19:14:01 +0300 Subject: [PATCH 2/7] perf(decode): drop #[cold] on do_offset_history_repcode for hot workloads (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #279 round-1 mispredict diagnosis attributed 15.42% of decoder mispredicts to `do_offset_history_repcode`, with 27.80% landing on the `pushq %rax` function entry — call/ret BTB pressure from the never-inlined boundary. `#[inline(never)]` was dropped in earlier rounds; `#[cold]` was kept to preserve out-of-line layout for low-entropy blocks where the prior «drop both» variant regressed +15.9% on L14. The z000033 L-5 decode flamegraph surfaces this helper at 1.93% self-time despite the `#[cold]` label — the cold-bias attribute itself blocks LLVM from inlining even at hot call sites where the call/ret + BTB cost dominates the body work. The body is small (RULES lookup + 6 branchless cmov) and inlining duplicates a tight scalar sequence into the seq decoder's per-sequence loop. Drop `#[cold]` and let the inline cost-model see the full picture. Cold callers don't pay anything they weren't paying before — their total cost was already dominated by surrounding rare-path work, not this helper. --- zstd/src/decoding/sequence_execution.rs | 28 +++++++++++++++---------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/zstd/src/decoding/sequence_execution.rs b/zstd/src/decoding/sequence_execution.rs index 75cb0daeb..5f1a70422 100644 --- a/zstd/src/decoding/sequence_execution.rs +++ b/zstd/src/decoding/sequence_execution.rs @@ -84,17 +84,23 @@ pub(crate) fn do_offset_history(offset_value: u32, lit_len: u32, scratch: &mut [ do_offset_history_repcode(offset_value, lit_len, scratch) } -// `#[cold]` keeps the body out of caller hot layout and biases icache -// against pulling it unless hit. Earlier the helper was also -// `#[inline(never)]`; round-1 findings on issue #279 (branch-mispredict -// diagnostic) attributed 15.42% of decoder mispredicts to this function, -// with 27.80% landing on the `pushq %rax` fn entry — call/ret BTB -// pressure from the never-inlined boundary. Dropping `#[inline(never)]` -// while keeping `#[cold]` lets LLVM inline at hot call sites where the -// boundary cost outweighs body duplication; cold paths (low-entropy -// blocks where the previous "drop both" variant regressed +15.9% on -// L14) keep the out-of-line shape via the cold attribute. -#[cold] +// Previously `#[cold]+#[inline(never)]`; round-1 findings on issue +// #279 attributed 15.42% of decoder mispredicts to this function with +// 27.80% on the `pushq %rax` fn entry — call/ret BTB pressure from +// the never-inlined boundary. `#[inline(never)]` was dropped first, +// keeping `#[cold]` to preserve out-of-line layout for low-entropy +// blocks (the prior «drop both» variant regressed +15.9% on L14). +// +// For high-repcode workloads (z000033 L-5, decode_all flamegraph +// surfaces this helper at 1.93% self-time despite the `#[cold]` +// label), the cold-bias attribute itself blocks LLVM from inlining +// even at the hot call sites where the call/ret + BTB cost still +// dominates the body work. Drop `#[cold]` and let the inline +// cost-model see the full picture — body is small enough (RULES +// lookup + 6 branchless cmov), so duplication into hot callers is +// affordable, and cold callers don't pay anything they weren't +// paying before (their cost was already dominated by the surrounding +// rare-path work, not this helper). fn do_offset_history_repcode(offset_value: u32, lit_len: u32, scratch: &mut [u32; 3]) -> u32 { #[derive(Copy, Clone)] struct Rule { From 5b5e02d25c7799346911d59f5f66e2086a09ae1c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 19:17:40 +0300 Subject: [PATCH 3/7] fix(fse): index slice instead of take() before unsafe set_len MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spread.iter().take(table_size)` silently runs fewer iterations if `spread.len() < table_size` — which can only happen if a future refactor breaks the upstream `spread.resize(table_size, 0)` invariant, but the failure mode is severe: the loop body would leave the `[loop_count..table_size)` slots in `decode`'s spare capacity uninitialised, and the post-loop `set_len(table_size)` would then claim those uninitialised entries as initialised — UB. Switch to `spread[..table_size].iter()`. A length mismatch now panics with a clear bounds-check error BEFORE the unsafe set_len runs, surfacing the broken invariant immediately instead of turning it into silent memory unsafety. --- zstd/src/fse/fse_decoder.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/zstd/src/fse/fse_decoder.rs b/zstd/src/fse/fse_decoder.rs index 158e245c0..48089bdb2 100644 --- a/zstd/src/fse/fse_decoder.rs +++ b/zstd/src/fse/fse_decoder.rs @@ -543,7 +543,16 @@ impl FSETableImpl { self.decode.reserve(table_size); let slots = &mut self.decode.spare_capacity_mut()[..table_size]; - for (state_idx, &symbol) in spread.iter().take(table_size).enumerate() { + // Slice index instead of `spread.iter().take(table_size)`: + // if `spread.len() < table_size` (a future refactor breaking + // the upstream `spread.resize(table_size, 0)` invariant), the + // slice indexing panics here BEFORE the unsafe `set_len` + // below would claim uninitialised entries. `take()` would + // silently shorten the loop and leave `slots` half-written, + // which the post-loop `set_len(table_size)` would then expose + // as UB. Indexing surfaces the invariant violation as a + // bounds-check panic instead. + for (state_idx, &symbol) in spread[..table_size].iter().enumerate() { let next_state = symbol_next[symbol as usize]; // `next_state >= 1` by construction: upstream // `read_probabilities` / `build_from_probabilities` From bdcec869b9834d3a6540685807035a41be243144 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 15:46:18 +0300 Subject: [PATCH 4/7] =?UTF-8?q?perf(decode):=20skip=20zero-init=20of=20lit?= =?UTF-8?q?erals=20target=20=E2=80=94=20HUF=20overwrites=20everything?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 4-stream HUF burst path pre-sized `target` to `base + regen` via `target.resize(base + regen, 0)` so subsequent cursor-based index writes (`target[cursors[s]] = symbol`) could land in-bounds. The zero-init pass is pure wasted work: the burst loop + drain phase writes every byte in [base, base+regen) before returning Ok, and every Err path calls `target.truncate(base)` so callers never see the uninitialised tail. `__memset_avx2_unaligned_erms` ran at ~0.5% of decode self-time on the z000033 L-5 flamegraph, with the visible call chain going through `Vec::resize -> extend_with` from this exact site. Replace with `unsafe { target.set_len(base + regen) }`. Capacity is guaranteed by the `target.reserve(regen)` call at the top of `decompress_literals_impl`. u8 has no Drop, so exposing uninitialised bytes from set_len cannot cause UB from the Vec layer itself; the downstream contract (every byte written on Ok, truncate on Err) is unchanged. Targets the P5 mechanical cleanup item from the decoder gap-closure plan; expected wall-time win is small (~0.3-0.5% on z000033) but the change is mechanical and risk-free. --- zstd/src/decoding/literals_section_decoder.rs | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/zstd/src/decoding/literals_section_decoder.rs b/zstd/src/decoding/literals_section_decoder.rs index e5f70f8e8..02aec594d 100644 --- a/zstd/src/decoding/literals_section_decoder.rs +++ b/zstd/src/decoding/literals_section_decoder.rs @@ -280,7 +280,27 @@ fn decompress_literals_impl( // decode directly into slices — no temporary Vec allocations. let seg = regen.div_ceil(4); - target.resize(base + regen, 0); + // Expose the [base, base+regen) tail without zero-init. The + // burst loop + drain phase below writes every byte in that + // range before this function returns Ok; every error path + // calls `target.truncate(base)` before returning Err, so a + // caller never observes uninitialised bytes. + // + // Replaces a `resize(base + regen, 0)` whose `__memset_avx2` + // showed up at ~0.5% of decode self-time on the z000033 L-5 + // flamegraph — pure wasted work since HUF decode overwrites + // every byte it covers. + // + // SAFETY: `target.reserve(regen)` at the top of this function + // guarantees `capacity() >= base + regen`. `u8` has no Drop, so + // exposing uninitialised bytes via `set_len` cannot cause UB + // from the Vec itself. The downstream HUF burst writes every + // index in [base, base+regen) on the Ok path; on Err, target + // is truncated before return (truncating u8 is a `set_len` + // shrink, no destructors run, no uninit memory escapes). + unsafe { + target.set_len(base + regen); + } // Clamp every start/end into [base, base+regen] so cursors can // never index past the pre-allocated region, even with corrupted // frame headers that produce small regen (where N*seg > regen). From 2c231221f7c0a8b3528b3b16c1032eba15c684d7 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 16:49:25 +0300 Subject: [PATCH 5/7] fix(decode): use raw pointer for HUF burst writes, set_len only on success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot flagged the prior P5 change as UB: `set_len(base + regen)` made the [base, base+regen) tail logically initialised before the HUF burst wrote into it, then the code formed `&mut [u8]` views over that range (via `run_4stream_burst_loop`'s slice param and the drain loop's `target[cursors[i]] = ...`). Constructing a reference to uninitialised `u8` is UB per the Rust memory model (Miri-detectable), even though u8 has no niche. Fix: keep `target.len()` at `base` throughout the burst + drain phase. Pass `target_ptr: *mut u8 = target.as_mut_ptr()` to the burst loop (signature change: `target: &mut [u8]` → `target_ptr: *mut u8`) and the drain loop. Writes go through `target_ptr.add( cursors[s]).write(byte)` — raw stores that never form a reference to uninitialised memory. Only after all 4 cursors reach their `ends[]` AND the `decoded == regen` check passes do we commit via `set_len(base + regen)`. Error paths drop the per-byte truncate (`target.len()` was never grown, so a truncate would be a no-op). Holding the raw pointer without an intervening `&mut Vec` borrow keeps stacked-borrows happy. Burst body codegen is unchanged: raw `*mut u8` store lowers to the same `mov [rax+r8], r9b` the slice-indexed version was emitting. The win from skipping the `__memset_avx2` zero-init pass is preserved with proven soundness. --- zstd/src/decoding/literals_section_decoder.rs | 100 +++++++++++------- 1 file changed, 60 insertions(+), 40 deletions(-) diff --git a/zstd/src/decoding/literals_section_decoder.rs b/zstd/src/decoding/literals_section_decoder.rs index 02aec594d..4ee682697 100644 --- a/zstd/src/decoding/literals_section_decoder.rs +++ b/zstd/src/decoding/literals_section_decoder.rs @@ -280,27 +280,24 @@ fn decompress_literals_impl( // decode directly into slices — no temporary Vec allocations. let seg = regen.div_ceil(4); - // Expose the [base, base+regen) tail without zero-init. The - // burst loop + drain phase below writes every byte in that - // range before this function returns Ok; every error path - // calls `target.truncate(base)` before returning Err, so a - // caller never observes uninitialised bytes. - // - // Replaces a `resize(base + regen, 0)` whose `__memset_avx2` - // showed up at ~0.5% of decode self-time on the z000033 L-5 - // flamegraph — pure wasted work since HUF decode overwrites - // every byte it covers. + // Stream the burst + drain output through a raw `*mut u8` into + // `target`'s spare capacity, leaving `target.len()` at `base` + // until every byte in [base, base+regen) is written. Only then + // do we commit via `set_len(base + regen)`. This avoids the + // `__memset_avx2` zero-init pass that a `resize(base+regen, 0)` + // would emit (~0.5% of decode self-time on z000033 L-5) while + // staying sound: at no point does the code construct a + // `&mut [u8]` reference covering uninitialised bytes. // // SAFETY: `target.reserve(regen)` at the top of this function - // guarantees `capacity() >= base + regen`. `u8` has no Drop, so - // exposing uninitialised bytes via `set_len` cannot cause UB - // from the Vec itself. The downstream HUF burst writes every - // index in [base, base+regen) on the Ok path; on Err, target - // is truncated before return (truncating u8 is a `set_len` - // shrink, no destructors run, no uninit memory escapes). - unsafe { - target.set_len(base + regen); - } + // guarantees `capacity() >= base + regen`, so the raw pointer + // can safely address every index in [base, base+regen). Error + // paths exit BEFORE the final `set_len`, leaving `target.len()` + // at the pre-call `base` value — uninitialised bytes never + // become observable to any caller. `u8` has no Drop, so the + // raw uninitialised tail in spare capacity carries no + // teardown obligations. + let target_ptr: *mut u8 = target.as_mut_ptr(); // Clamp every start/end into [base, base+regen] so cursors can // never index past the pre-allocated region, even with corrupted // frame headers that produce small regen (where N*seg > regen). @@ -380,29 +377,42 @@ fn decompress_literals_impl( // no SIMD intrinsics needed). Single un-genericised call. // // SAFETY: caller guarantees `brs[s].source` is the same as the - // stream slice each decoder was initialised against; the - // upfront `target.resize(base + regen, 0)` covers all cursor - // writes; `packed` length matches `1 << max_num_bits` by - // `HuffmanTable::build_decoder`'s `resize`. + // stream slice each decoder was initialised against; `target_ptr` + // addresses an allocation of at least `base + regen` bytes (via + // the `target.reserve(regen)` above), so cursor writes in + // [base, base+regen) are in-bounds; `packed` length matches + // `1 << max_num_bits` by `HuffmanTable::build_decoder`'s `resize`. unsafe { run_4stream_burst_loop( &mut decoders, &mut brs, - target, + target_ptr, packed, &mut cursors, &bounds, ); } - // Drain remaining symbols from each stream, bounded by segment end + // Drain remaining symbols from each stream, bounded by segment end. + // SAFETY: cursors[i] ∈ [base, base+regen) by `starts`/`ends` clamping + // earlier; `target_ptr.add(cursors[i])` is therefore within the + // reserved allocation. Each write initialises one previously- + // uninitialised byte; `target.len()` remains at `base` so no + // `&mut [u8]` reference is constructed to that byte before it is + // written. The error exits below DO NOT call `target.truncate(base)` + // — `target.len()` is still `base` already, so a truncate would be + // a no-op; eliding it also lets us hold `target_ptr` without an + // intervening `&mut Vec` borrow that invalidates the pointer + // under stacked-borrows. for i in 0..4 { while brs[i].bits_remaining() > -max_bits && cursors[i] < ends[i] { - target[cursors[i]] = decoders[i].decode_symbol_and_advance(&mut brs[i]); + let byte = decoders[i].decode_symbol_and_advance(&mut brs[i]); + unsafe { + target_ptr.add(cursors[i]).write(byte); + } cursors[i] += 1; } if brs[i].bits_remaining() != -max_bits { - target.truncate(base); return Err(DecompressLiteralsError::BitstreamReadMismatch { read_til: brs[i].bits_remaining(), expected: -max_bits, @@ -411,18 +421,23 @@ fn decompress_literals_impl( } // Verify total decoded count matches expected regenerated size. - // Return error immediately rather than deferring to the downstream check. let decoded: usize = cursors.iter().zip(starts.iter()).map(|(c, s)| c - s).sum(); if decoded != regen { - // Truncate to base: segmented layout means partial decode left - // bytes scattered across segments, so only base is a clean boundary. - target.truncate(base); return Err(DecompressLiteralsError::DecodedLiteralCountMismatch { decoded, expected: regen, }); } + // Commit: every byte in [base, base+regen) was written above + // (cursors[s] reached ends[s] for all s, and the decoded total + // equals regen). `target.len()` was `base` until this point — + // exposing the freshly-initialised tail is now sound. + // SAFETY: see the `target_ptr` block above. + unsafe { + target.set_len(base + regen); + } + bytes_read += source.len() as u32; } else { //just decode the one stream @@ -506,7 +521,7 @@ struct LoopBounds { unsafe fn run_4stream_burst_loop( decoders: &mut [HuffmanDecoder<'_>; 4], brs: &mut [BitReaderReversed<'_, K>; 4], - target: &mut [u8], + target_ptr: *mut u8, packed: &[u16], cursors: &mut [usize; 4], bounds: &LoopBounds, @@ -617,38 +632,43 @@ unsafe fn run_4stream_burst_loop( // `[0, 1 << max_num_bits)`. `packed.len() == 1 << max_num_bits` // by `HuffmanTable::build_decoder`'s upfront `resize`. // - // SAFETY for `target.get_unchecked_mut(cursors[s])`: + // SAFETY for `target_ptr.add(cursors[s]).write(...)`: // The outer-loop gate `cursors[0] <= cursor_burst_ceil` // gives `cursors[0] + symbols_per_burst <= cursor_burst_ceil // + symbols_per_burst = starts[0] + min_seg_len`. By lockstep // advance, `cursors[s] - starts[s] == cursors[0] - starts[0]` // for all `s`, so `cursors[s] + symbols_per_burst - 1 < - // starts[s] + min_seg_len <= ends[s] <= target.len()` — - // every write in this iter (max index `cursors[s] + - // symbols_per_burst - 1`) is strictly in-bounds. + // starts[s] + min_seg_len <= ends[s] <= base + regen`. The + // caller passes `target_ptr = target.as_mut_ptr()` for a Vec + // whose `reserve(regen)` ensures `capacity >= base + regen`, + // so every write in this iter (max index `cursors[s] + + // symbols_per_burst - 1`) is strictly within the allocation. + // Using `.write()` (raw store) instead of `&mut [u8]` indexing + // means no Rust reference is ever formed to the uninitialised + // tail before its byte is written. debug_assert!(cursors[0] + symbols_per_burst <= cursor_burst_ceil + symbols_per_burst); for _ in 0..symbols_per_burst { let idx0 = (bits[0] >> table_shift) as usize; let entry0 = unsafe { *packed.get_unchecked(idx0) }; - unsafe { *target.get_unchecked_mut(cursors[0]) = (entry0 & 0xFF) as u8 }; + unsafe { target_ptr.add(cursors[0]).write((entry0 & 0xFF) as u8) }; cursors[0] += 1; bits[0] <<= (entry0 >> 8) & 0xFF; let idx1 = (bits[1] >> table_shift) as usize; let entry1 = unsafe { *packed.get_unchecked(idx1) }; - unsafe { *target.get_unchecked_mut(cursors[1]) = (entry1 & 0xFF) as u8 }; + unsafe { target_ptr.add(cursors[1]).write((entry1 & 0xFF) as u8) }; cursors[1] += 1; bits[1] <<= (entry1 >> 8) & 0xFF; let idx2 = (bits[2] >> table_shift) as usize; let entry2 = unsafe { *packed.get_unchecked(idx2) }; - unsafe { *target.get_unchecked_mut(cursors[2]) = (entry2 & 0xFF) as u8 }; + unsafe { target_ptr.add(cursors[2]).write((entry2 & 0xFF) as u8) }; cursors[2] += 1; bits[2] <<= (entry2 >> 8) & 0xFF; let idx3 = (bits[3] >> table_shift) as usize; let entry3 = unsafe { *packed.get_unchecked(idx3) }; - unsafe { *target.get_unchecked_mut(cursors[3]) = (entry3 & 0xFF) as u8 }; + unsafe { target_ptr.add(cursors[3]).write((entry3 & 0xFF) as u8) }; cursors[3] += 1; bits[3] <<= (entry3 >> 8) & 0xFF; } From bd22c20d845509a9f912e16798b8b43ed69ea8dc Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 19:24:11 +0300 Subject: [PATCH 6/7] docs(decode): update run_4stream_burst_loop SAFETY contract for raw pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function signature changed from `target: &mut [u8]` to `target_ptr: *mut u8` (commit fa50c464), but the `# Safety` block still claimed `target.len() >= base + regen` — which no longer holds because the caller now keeps `target.len()` at `base` until the post-loop `set_len` commits initialisation. Update the contract to document the actual current requirements: the pointer comes from a Vec whose allocation is at least `base + regen` bytes (via the caller's `target.reserve(regen)`), the Vec must not be reallocated while the pointer is in use, and writes use raw stores so no `&mut [u8]` view ever covers the uninitialised tail. --- zstd/src/decoding/literals_section_decoder.rs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/zstd/src/decoding/literals_section_decoder.rs b/zstd/src/decoding/literals_section_decoder.rs index 4ee682697..18b431428 100644 --- a/zstd/src/decoding/literals_section_decoder.rs +++ b/zstd/src/decoding/literals_section_decoder.rs @@ -514,9 +514,23 @@ struct LoopBounds { /// # Safety /// /// All four decoders must share the same table (holds by construction — -/// built from `&scratch.table`). `target.len() >= base + regen`. Each -/// `brs[s].source` must be the slice the corresponding decoder was -/// initialised against. +/// built from `&scratch.table`). +/// +/// `target_ptr` must come from a `Vec` whose allocation is at least +/// `base + regen` bytes (the caller guarantees this via +/// `target.reserve(regen)` before deriving the pointer). The Vec must +/// not be reallocated or moved while `target_ptr` is in use — the +/// caller holds no other references during the burst+drain phase so +/// this invariant is upheld trivially. +/// +/// Writes go through raw `target_ptr.add(cursors[s]).write(byte)` so +/// no Rust reference is ever constructed to the uninitialised tail +/// (`target.len()` stays at `base` until the post-loop `set_len` +/// commits initialisation). Cursor bounds `[base, base+regen)` are +/// enforced by the `starts`/`ends` clamping in the caller. +/// +/// Each `brs[s].source` must be the slice the corresponding decoder +/// was initialised against. #[inline(always)] unsafe fn run_4stream_burst_loop( decoders: &mut [HuffmanDecoder<'_>; 4], From 4d60f9b46bf8e19cb7fb320c7b26aeada8727e46 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Thu, 28 May 2026 21:17:53 +0300 Subject: [PATCH 7/7] docs(decode): rewrite run_4stream_burst_loop SAFETY against in-scope values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The in-loop SAFETY block referenced `starts[s]`, `ends[s]`, `min_seg_len`, and `base + regen` — none of which exist inside `run_4stream_burst_loop` (all live at the caller). That made the safety story hard to audit: a reader inside the function had no way to verify the bound it claimed. Rewrite the contract using only values visible in the function: `cursors`, `cursor_burst_ceil`, `symbols_per_burst`, and the new `alloc_upper_bound` field on `LoopBounds`. The caller fills `alloc_upper_bound = base + regen` (the upper bound on legal cursor values backed by `target.reserve(regen)`). A function-entry `debug_assert!` (after the `burst_eligible` early-return so the malformed-small-regen path still bails cleanly) ties `cursor_burst_ceil + symbols_per_burst` to `alloc_upper_bound`, turning the SAFETY chain into something a future maintainer can verify locally. Dropped the tautological `debug_assert!(cursors[0] + symbols_per_burst <= cursor_burst_ceil + symbols_per_burst)` — proved nothing. --- zstd/src/decoding/literals_section_decoder.rs | 58 ++++++++++++++----- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/zstd/src/decoding/literals_section_decoder.rs b/zstd/src/decoding/literals_section_decoder.rs index 18b431428..fbc2b1f5e 100644 --- a/zstd/src/decoding/literals_section_decoder.rs +++ b/zstd/src/decoding/literals_section_decoder.rs @@ -370,6 +370,7 @@ fn decompress_literals_impl( table_shift, cursor_burst_ceil, burst_eligible, + alloc_upper_bound: base + regen, }; // Burst is identical across all kernels (donor parity: reads @@ -498,6 +499,13 @@ struct LoopBounds { /// decodes ALL symbols via the single-symbol path. Setup-site /// safety rationale: adversarial / small-regen DoS guard. burst_eligible: bool, + /// Upper bound (exclusive) on cursor values written through + /// `target_ptr` — equals `base + regen` at the caller. Carried + /// here so the burst loop's SAFETY argument can be stated + /// purely in terms of values visible inside the function. The + /// caller guarantees the allocation behind `target_ptr` covers + /// `[0, alloc_upper_bound)` (via `target.reserve(regen)`). + alloc_upper_bound: usize, } /// Donor-parity 4-stream HUF decode burst loop. Single code path — @@ -546,16 +554,37 @@ unsafe fn run_4stream_burst_loop( table_shift, cursor_burst_ceil, burst_eligible, + alloc_upper_bound, } = *bounds; let max_num_bits = (64 - table_shift) as u8; // Skip burst entirely if min_seg_len < symbols_per_burst — drain // (the single-symbol tail outside this function) handles ALL - // symbols. See the `burst_eligible` doc on `LoopBounds`. + // symbols. See the `burst_eligible` doc on `LoopBounds`. Bailing + // here also covers the malformed-frame case where `regen` is + // smaller than a single burst's output (the caller's allocation + // is then too small to hold even one burst, but the drain path + // handles symbol-by-symbol decode within the segment ends). if !burst_eligible { return; } + // Caller-side cursor bound check, restated here so SAFETY reasoning + // below is self-contained against in-scope values only. The outer + // gate `cursors[0] <= cursor_burst_ceil` plus lockstep advance + // ensures every write index is `< cursor_burst_ceil + symbols_per_burst`; + // requiring that bound to fit inside the caller's allocation makes + // each `target_ptr.add(idx).write(_)` provably in-bounds. Only + // meaningful once `burst_eligible == true` (a malformed-frame + // small-regen path can leave `cursor_burst_ceil` saturated below + // `alloc_upper_bound - symbols_per_burst` — the burst skip above + // already bails on that case). + debug_assert!( + cursor_burst_ceil + symbols_per_burst <= alloc_upper_bound, + "caller must size the target allocation so the lockstep-advanced \ + cursors stay within bounds across a full burst", + ); + // Donor-parity burst loop. `bits[s]` is the unified u64 register // that fuses state + unconsumed stream + sentinel: // bits 63..(64-max_num_bits): current state (next index into `packed`) @@ -648,19 +677,20 @@ unsafe fn run_4stream_burst_loop( // // SAFETY for `target_ptr.add(cursors[s]).write(...)`: // The outer-loop gate `cursors[0] <= cursor_burst_ceil` - // gives `cursors[0] + symbols_per_burst <= cursor_burst_ceil - // + symbols_per_burst = starts[0] + min_seg_len`. By lockstep - // advance, `cursors[s] - starts[s] == cursors[0] - starts[0]` - // for all `s`, so `cursors[s] + symbols_per_burst - 1 < - // starts[s] + min_seg_len <= ends[s] <= base + regen`. The - // caller passes `target_ptr = target.as_mut_ptr()` for a Vec - // whose `reserve(regen)` ensures `capacity >= base + regen`, - // so every write in this iter (max index `cursors[s] + - // symbols_per_burst - 1`) is strictly within the allocation. - // Using `.write()` (raw store) instead of `&mut [u8]` indexing - // means no Rust reference is ever formed to the uninitialised - // tail before its byte is written. - debug_assert!(cursors[0] + symbols_per_burst <= cursor_burst_ceil + symbols_per_burst); + // bounds the burst's first cursor. All four cursors advance + // in lockstep (`cursors[s]` gains exactly 1 per inner iter + // along with `cursors[0]`), so the invariant + // `cursors[s] <= cursor_burst_ceil` holds across this burst's + // start for every `s`. The maximum write index during the + // `symbols_per_burst` inner iters is `cursors[s] + symbols_per_burst - 1`, + // strictly less than `cursor_burst_ceil + symbols_per_burst`. + // The function-entry `debug_assert!` ties that bound to + // `alloc_upper_bound`, which the caller guarantees to cover + // the allocation backing `target_ptr` (via + // `target.reserve(regen)` before deriving the pointer). + // `.write()` (raw store) is used instead of `&mut [u8]` + // indexing so no Rust reference is ever formed to the + // uninitialised tail before its byte is written. for _ in 0..symbols_per_burst { let idx0 = (bits[0] >> table_shift) as usize; let entry0 = unsafe { *packed.get_unchecked(idx0) };