Skip to content

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

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test contains a loop over a range of unconstrained integer types which defaults to i32.
Aiui in miri "time" ticks based on the number of mir statements executed or something like that. Since the Range impl changed this also affects rate of time.

Copy link
Member

@saethlin saethlin Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh this test again. We should probably do something a bit better here.

When isolation is enabled (the default), Miri doesn't have access to the system clock, so we use the end of a basic block as an extremely crude approximation of clock ticks, so that we can have a clock when isolation is enabled. This test asserts that the magic number for NANOSECONDS_PER_BASIC_BLOCK gets us an isolation clock that's remotely close to the real passage of time. Because when we first did this I think it was off by a factor of 100.

This diff is fine for this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a bit surprised that the number of basic blocks is almost doubling?