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

Integer overflow false positive with (0u8..).take(256) #25708

Closed
Kimundi opened this issue May 22, 2015 · 4 comments · Fixed by #72368
Closed

Integer overflow false positive with (0u8..).take(256) #25708

Kimundi opened this issue May 22, 2015 · 4 comments · Fixed by #72368
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Kimundi
Copy link
Member

Kimundi commented May 22, 2015

Half open ranges like 0u8.. are defined as counting upwards without doing bound checks until their last value.

However, due to the way they are implemented they will panic in debug mode before yielding the last element:

fn main() {
    for i in (0u8..).take(256).skip(250) {
        println!("{}", i);
    }
}
250
251
252
253
254
thread '<main>' panicked at 'arithmetic operation overflowed', /home/rustbuild/src/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libcore/ops.rs:203
playpen: application terminated with error code 101

(https://play.rust-lang.org/?gist=db5b640e1a6c015c03f5&version=stable)

The issue here is that while the yielded values do not depend on a integer overflow occurring, it does so as part for preparing the state for the next, invalid iteration.

It is therefore not possible to use them for counting through the whole range of an integer value in debug mode.

A possible solution would be to use conditional compilation to add a overflow flag to the struct in debug mode, have it use wrapping operation and panic on the next next() call.

See also #20249

@bluss
Copy link
Member

bluss commented May 22, 2015

See also #25696.

@tbu-
Copy link
Contributor

tbu- commented May 6, 2016

There are three ways to go about this:

  1. Return the sequence 0, 1, 2, ..., 255, 255, 255, 255, 255, ...
    This would make the (0u8..).take(256) work, but gives weird behavior after the first 255.
  2. Return the sequence 0, 1, 2, ..., 255, 0, 1, 2, ...
  3. Make a breaking change to extend the struct, either only in debug mode or always.

If (1) or (2) is wanted, I can implement it, but otherwise this issue can be closed with the label rust-2-breakage-wishlist, because the behavior of 0, 1, 2, ..., 255, panic is impossible to implement for the debug mode.

(This behavior is impossible, because you have 256 states for the struct, but want to yield 257 different behaviors: 0, ..., 255, panic.)

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 22, 2017
@steveklabnik
Copy link
Member

Triage: still panics

@CAD97
Copy link
Contributor

CAD97 commented May 20, 2020

For anyone falling on this issue: the correct way to iterate over all values is to use an inclusive range, 0..=u8::MAX.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 29, 2020
Resolve overflow behavior for RangeFrom

This specifies a documented unspecified implementation detail of `RangeFrom` and makes it consistently implement the specified behavior.

Specifically, `(u8::MAX).next()` is defined to cause an overflow, and resolve that overflow in the same manner as the `Step::forward` implementation.

The inconsistency that has existed is `<RangeFrom as Iterator>::nth`. The existing behavior should be plain to see after rust-lang#69659: the skipping part previously always panicked if it caused an overflow, but the final step (to set up the state for further iteration) has always been debug-checked.

The inconsistency, then, is that `RangeFrom::nth` does not implement the same behavior as the naive (and default) implementation of just calling `next` multiple times. This PR aligns `RangeFrom::nth` to have identical behavior to the naive implementation. It also lines up with the standard behavior of primitive math in Rust everywhere else in the language: debug checked overflow.

cc @Amanieu

---

Followup to rust-lang#69659. Closes rust-lang#25708 (by documenting the panic as intended).

The documentation wording is preliminary and can probably be improved.

This will probably need an FCP, as it changes observable stable behavior.
@bors bors closed this as completed in 93d45a0 May 30, 2020
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants