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

fix unsoundness in Step::forward_unchecked for signed integers #122461

Merged
merged 2 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions library/core/src/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,25 @@ pub trait Step: Clone + PartialOrd + Sized {
}
}

// These are still macro-generated because the integer literals resolve to different types.
macro_rules! step_identical_methods {
// Separate impls for signed ranges because the distance within a signed range can be larger
// than the signed::MAX value. Therefore `as` casting to the signed type would be incorrect.
macro_rules! step_signed_methods {
($unsigned: ty) => {
#[inline]
unsafe fn forward_unchecked(start: Self, n: usize) -> Self {
// SAFETY: the caller has to guarantee that `start + n` doesn't overflow.
unsafe { start.checked_add_unsigned(n as $unsigned).unwrap_unchecked() }
}

#[inline]
unsafe fn backward_unchecked(start: Self, n: usize) -> Self {
// SAFETY: the caller has to guarantee that `start - n` doesn't overflow.
unsafe { start.checked_sub_unsigned(n as $unsigned).unwrap_unchecked() }
}
};
}

macro_rules! step_unsigned_methods {
() => {
#[inline]
unsafe fn forward_unchecked(start: Self, n: usize) -> Self {
Expand All @@ -198,7 +215,12 @@ macro_rules! step_identical_methods {
// SAFETY: the caller has to guarantee that `start - n` doesn't overflow.
unsafe { start.unchecked_sub(n as Self) }
}
};
}

// These are still macro-generated because the integer literals resolve to different types.
macro_rules! step_identical_methods {
() => {
#[inline]
#[allow(arithmetic_overflow)]
#[rustc_inherit_overflow_checks]
Expand Down Expand Up @@ -239,6 +261,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $u_narrower {
step_identical_methods!();
step_unsigned_methods!();

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand Down Expand Up @@ -271,6 +294,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $i_narrower {
step_identical_methods!();
step_signed_methods!($u_narrower);

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand Down Expand Up @@ -335,6 +359,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $u_wider {
step_identical_methods!();
step_unsigned_methods!();

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand All @@ -360,6 +385,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $i_wider {
step_identical_methods!();
step_signed_methods!($u_wider);

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand Down
5 changes: 5 additions & 0 deletions library/core/tests/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,11 @@ fn test_range_advance_by() {
assert_eq!(Ok(()), r.advance_back_by(usize::MAX));

assert_eq!((r.start, r.end), (0u128 + usize::MAX as u128, u128::MAX - usize::MAX as u128));

// issue 122420, Step::forward_unchecked was unsound for signed integers
let mut r = -128i8..127;
assert_eq!(Ok(()), r.advance_by(200));
assert_eq!(r.next(), Some(72));
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
The loop took around 7s
The loop took around 12s
Loading