Fix soundness, correctness, and panic bugs found in review#41
Merged
Conversation
What: Peekable::peek_mut and FalliblePeekable::peek_mut returned a mutable
reference to the cached lend with the lender's full storage lifetime ('this),
not the &mut self borrow, so entirely safe code could copy out a lend that
outlives the borrow (e.g. a Peekable<'static, _> yields a dangling &'static
reference).
Why: use-after-free reachable from 100% safe code -- a soundness hole.
How: no sound safe signature exists (shrinking the lend lifetime re-enables an
unsound write-back of a shorter-lived lend), so gate both methods behind
`unsafe fn` with a documented `# Safety` contract; wrap the doc examples and
the three test call sites in `unsafe {}`, and add a trybuild case proving the
safe call is rejected (E0133).
…ocation What: into_inner/count/into_parts on Peekable and Flatten/FlatMap moved the AliasableBox<L> out (freeing the L allocation) while the sibling MaybeDangling cache field still held a lend or sub-lender borrowing into that allocation; the cache was then dropped afterward. Why: a covariant lend or sub-lender with drop glue is dropped after its backing allocation is freed -- use-after-free on drop, reachable from the safe API. How: clear the cache (dropping any held lend/sub-lender while the box is still alive) before extracting the box, and in count inspect the cache before freeing it; fold/rfold already extract the box only in the empty-cache arm and are left untouched. Adds behavioral regression tests (the UAF itself is a Miri check).
What: StepBy::nth (and the fallible twin), on a fresh iterator (first_take) with n > 0, decremented n without consuming the first element, so it returned an element one step too early -- e.g. step_by(2).nth(1) yielded index 1 instead of index 2. Why: wrong result, disagreeing with std::iter::StepBy and with the crate's own next() path (which consumes the first element). How: consume and drop the first element (self.lender.next()) before the n -= 1 in the first_take branch, mirroring std. Adds regression tests asserting parity with std::iter::StepBy.
What: Fuse::last (and the fallible twin) returned Some without setting the fused flag; since last() drives the inner lender to None, a non-fused inner could then be re-polled by Fuse::next and resurrect, and size_hint stayed non-zero. Why: violates Fuse's guarantee that once the inner yields None, the Fuse yields None forever. How: set flag = true unconditionally before delegating to the inner last(), so a subsequent next() short-circuits to None. Adds regression tests with a resurrecting inner lender.
What: TakeWhile::next (and the fallible twin) returned None on inner-None via `self.lender.next()?` / an early `return Ok(None)` without setting the done flag, yet TakeWhile unconditionally implements FusedLender / FusedFallibleLender, so a non-fused inner could resurrect on a later next(). Why: the advertised fused guarantee (None is terminal) was broken. How: set flag = true on the inner-None path so subsequent next() calls short-circuit to None. Adds regression tests with a resurrecting inner lender.
What: Take::advance_back_by(0) trimmed the untaken tail of the inner lender instead of being a no-op, and Take::nth_back on an empty take(0) drained the whole inner lender, whereas Take::next_back on take(0) correctly returns None untouched. Both apply to the fallible twin. Why: advance_back_by(0) must be a no-op, and an empty Take must not consume the inner lender; both corrupt the lender later returned by into_inner. How: early-return Ok on advance_back_by(0), and guard the nth_back tail-drain with `self.n != 0` so an empty Take leaves the inner untouched (mirroring the next_back guard). Adds regression tests inspecting into_inner after each op.
…low panic
What: Chunky::nth's exhaust branch (and the fallible twin) computed
`self.len.checked_mul(self.chunk_size).expect("overflow in Chunky::nth")`;
with a valid ExactSizeLender reporting len() near usize::MAX, the chunk count
(len().div_ceil(chunk_size)) times chunk_size overflows and the expect panics.
Why: a public method panics on a valid (if extreme) lender; the exhaust branch
only needs to drive the inner lender to its end.
How: use saturating_mul in the exhaust branch -- the product only feeds
advance_by, whose overshoot is harmless. The in-range branch keeps checked_mul
(there n * chunk_size < total <= usize::MAX and never overflows). Adds a
regression test with a usize::MAX-length lender.
What: FromIterRef::{next,nth,next_back,nth_back} (and the fallible twin's
next/nth/next_back) ran `self.current = self.iter.next()` with the previous
item still owned in self.current while the right-hand side executed, so an
iterator whose items hold an exclusive borrow or guard (RefMut, MutexGuard,
...) re-acquired the same resource and panicked or deadlocked.
Why: a valid iterator makes FromIterRef panic (e.g. RefCell "already mutably
borrowed").
How: assign self.current = None before fetching, dropping the previous item
first -- exactly as the existing last() already does. Adds regression tests
with an iterator yielding RefCell RefMut guards.
What: Lender::next_chunk and FallibleLender::next_chunk documented "This is equivalent to cloning the lender and calling take(chunk_size) on it", but next_chunk borrows &mut self (Chunk::new(self, ...)) and advances the original lender -- it neither clones nor consumes it. Why: the documented ownership/state model is wrong and misleads callers about the lender's state after taking a chunk. How: replace the false sentence with an accurate description -- the returned Chunk borrows the lender mutably and advances it as consumed, and unconsumed elements remain for later calls (Chunk has no Drop that skips them).
zommiommy
force-pushed
the
review-bugfixes
branch
from
July 9, 2026 14:45
f01f09d to
db2dc79
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a set of soundness, correctness, panic, and documentation bugs found while reviewing the crate. Each fix is its own commit and ships with a regression test in the same commit; every fix is applied to both the
adapters/sourcesversion and itsfallible_*twin.Soundness
peekable: makepeek_mutunsafe.Peekable::peek_mut/FalliblePeekable::peek_mutreturnedOption<&mut Lend<'this, L>>with the lender's full storage lifetime, so safe code could copy out a lend that outlives the&mut selfborrow (aPeekable<'static, _>yields a dangling reference). There is no sound safe signature, so both are nowunsafe fnwith a# Safetycontract; a trybuild case proves the safe call is rejected (E0133).peekable/flatten: drop the cached lend before freeing the lender allocation.into_inner/count/into_partsmoved theAliasableBox<L>out (freeingL) while the siblingMaybeDanglingcache still held a lend/sub-lender borrowing into it, dropping that cache afterward — a use-after-free on drop. The cache is now cleared while the box is alive.Correctness
step_by:nthoff-by-one on a freshStepBy.step_by(2).nth(1)returned index 1 instead of 2; the first element is now consumed before decrementingn, matchingstd::iter::StepBy.fuse:last()did not set the fused flag. Sincelast()drives the inner lender toNone, a non-fused inner could resurrect on a laternext(); the flag is now set.take_while: inner-Nonewas not terminal despite the unconditionalFusedLender/FusedFallibleLenderimpl; the done flag is now set on the inner-Nonepath.take: zero-width back operations corrupted the inner lender.advance_back_by(0)trimmed the tail andtake(0).nth_back(_)drained the inner lender (unlikenext_back);advance_back_by(0)is now a no-op andnth_backis guarded withself.n != 0.Panics
chunky:nthoverflow panic. The exhaust branch didlen.checked_mul(chunk_size).expect(...), panicking for anExactSizeLenderwithlen()nearusize::MAX; it now usessaturating_mul(the product only feedsadvance_by).from_iter_ref: double-borrow panic/deadlock.next/nth/next_back/nth_backranself.current = self.iter.next()with the previous item still owned, so an iterator whose items hold an exclusive borrow/guard (RefMut,MutexGuard) re-acquired the same resource;self.currentis now cleared before fetching, aslast()already did.Docs
next_chunk: corrected the doc that claimed it "is equivalent to cloning the lender and callingtake"; it borrows&mut selfand advances the original lender.Testing
cargo test -p lenderpasses: 34 lib unit tests, all integration suites, the trybuildfailharness, and 214 doctests.cargo buildandcargo clippy -p lender --all-targets -- -D warningsare clean. The two soundness fixes are correct by construction; Miri validation of them is left for CI.