fix(store): remove aliasing UB in FusedIter::next#2993
Conversation
FusedIter::next created a second `&mut Self` via `&mut *ptr::from_mut(self)` and kept it (and a borrow derived from it) live across a `replace(self, ...)` call, producing two aliasing mutable references — undefined behavior. The reborrow existed only to work around the borrow checker's inability to prove that the exhaustion transition in the `None` branch doesn't conflict with the key returned in the `Some` branch (NLL problem case #3). Replace the `Active`/`Interregnum`/`Expended` enum with a struct holding the iterator and an `expended` flag. The transition then becomes a write to the disjoint `expended` field, which the borrow checker accepts without any reborrowing — the same split-borrow pattern already used by the sibling `Iter::next`. This drops the unsafe block entirely along with the now-unused `ptr`, `replace`, and `unreachable_unchecked` imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ENkAQmget6Yusiefe9TUXx
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 1 agents | Quality score: 31% | Review time: 38.9s
🟡 1 warnings. See inline comments.
🤖 Generated by MeroReviewer | Review ID: review-c20891d0
| Self::Active(iter) => *self = Self::Expended(iter), | ||
| Self::Expended(_) | Self::Interregnum => unsafe { unreachable_unchecked() }, | ||
| } | ||
| self.expended = true; |
There was a problem hiding this comment.
🟡 seek() does not reset expended flag after re-seek
The old Active/Expended enum preserved the iterator in the Expended variant, which made it impossible to re-use after exhaustion. The new struct keeps the same iterator and sets expended = true on exhaustion, but seek() now returns Ok(None) when expended is true without ever clearing the flag. If a caller calls seek() after the iterator is exhausted (which is a valid use-case for DBIter — seek can rewind), the call is silently swallowed. This is a behavioral change from the old code where Active(iter).seek(key) would always forward to the underlying iterator regardless of prior exhaustion. Whether this matters depends on whether callers ever re-seek an exhausted FusedIter, but the DBIter contract does not prohibit it and IterPair::seek calls self.0.seek(...) unconditionally.
Suggested fix:
If re-seeking should reset the iterator, clear `expended` in `seek`: `self.expended = false; self.iter.seek(key)`. If re-seeking after exhaustion is intentionally unsupported, document the invariant with a comment.
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 299.8s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-89f70031
2 participants
FusedIter::next created a second
&mut Selfvia&mut *ptr::from_mut(self)and kept it (and a borrow derived from it)live across a
replace(self, ...)call, producing two aliasing mutablereferences — undefined behavior. The reborrow existed only to work around
the borrow checker's inability to prove that the exhaustion transition in
the
Nonebranch doesn't conflict with the key returned in theSomebranch (NLL problem case #3).
Replace the
Active/Interregnum/Expendedenum with a struct holdingthe iterator and an
expendedflag. The transition then becomes a writeto the disjoint
expendedfield, which the borrow checker accepts withoutany reborrowing — the same split-borrow pattern already used by the
sibling
Iter::next. This drops the unsafe block entirely along with thenow-unused
ptr,replace, andunreachable_uncheckedimports.Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01ENkAQmget6Yusiefe9TUXx