Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Side effect handling in specialized zip implementation causes buffer overflow #82282

Closed
Qwaz opened this issue Feb 19, 2021 · 2 comments · Fixed by #82289
Closed

Side effect handling in specialized zip implementation causes buffer overflow #82282

Qwaz opened this issue Feb 19, 2021 · 2 comments · Fixed by #82289
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Qwaz
Copy link
Contributor

Qwaz commented Feb 19, 2021

} else if A::may_have_side_effect() && self.index < self.a.size() {
let i = self.index;
self.index += 1;
// match the base implementation's potential side effects
// SAFETY: we just checked that `i` < `self.a.len()`
unsafe {
self.a.__iterator_get_unchecked(i);
}
None

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
let len = self.len - self.index;
(len, Some(len))
}

self.index can be set to a value greater than self.len in this branch. This causes integer overflow in size_hint() and lead to a buffer overflow.

Playground Link that demonstrates segfault with safe Rust code.

@Qwaz Qwaz added the C-bug Category: This is a bug. label Feb 19, 2021
@Qwaz
Copy link
Contributor Author

Qwaz commented Feb 19, 2021

For the context, this causes a buffer overflow by violating the safety requirement of TrustedRandomAccess trait.

/// An iterator whose items are random-accessible efficiently
///
/// # Safety
///
/// The iterator's `size_hint` must be exact and cheap to call.
///
/// `size` may not be overridden.
///
/// `<Self as Iterator>::__iterator_get_unchecked` must be safe to call
/// provided the following conditions are met.
///
/// 1. `0 <= idx` and `idx < self.size()`.
/// 2. If `self: !Clone`, then `get_unchecked` is never called with the same
/// index on `self` more than once.
/// 3. After `self.get_unchecked(idx)` has been called then `next_back` will
/// only be called at most `self.size() - idx - 1` times.
/// 4. After `get_unchecked` is called, then only the following methods will be
/// called on `self`:
/// * `std::clone::Clone::clone()`
/// * `std::iter::Iterator::size_hint()`
/// * `std::iter::Iterator::next_back()`
/// * `std::iter::Iterator::__iterator_get_unchecked()`
/// * `std::iter::TrustedRandomAccess::size()`

@GuillaumeGomez GuillaumeGomez added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Feb 19, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 19, 2021
@jonas-schievink jonas-schievink added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 19, 2021
@hameerabbasi
Copy link
Contributor

Assigning P-critical as part of the WG-prioritization discussion on Zulip.

@hameerabbasi hameerabbasi added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 19, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this issue Mar 5, 2021
Fix underflow in specialized ZipImpl::size_hint

Fixes rust-lang#82282
@bors bors closed this as completed in ee796c6 Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants