Check for Integer Overflow by Default

[Apromixately]

It would be good to always check integers for overflow and thereby providing users with an integer type that actually behaves like an integer or at least fails completely instead of giving "wrong" results.

This was discussed on IRC last week and three distinct cases were identified:

1. An integer is desired and the implicit modular arithmetic is incorrect
2. Modular arithmetic is desired
3. An integer is desired but the user is sure that overflows are impossible and needs the extra speed of omitting the checks

My proposal is to make (1.) the default.

For (2.) there is already Wrapping but (3.) should also be annotated requiring people to assert that they have done their homework and
a) are sure that overflows cannot cause problems
b) the compiler cannot infer that the situation is safe and remove the checks
c) actually need the speed-up of omitting the checks.

I am aware that there are checked operations and compiler flags to keep overflow checks in release builds but the defaults are important and the defaults are wrong!

This issue is also discussed in the following two posts:
https://mail.mozilla.org/pipermail/rust-dev/2014-June/010363.html
https://huonw.github.io/blog/2016/04/myths-and-legends-about-integer-overflow-in-rust/

[andreytkachenko]

What about to create another compiler flag (if it is not yet created) for that. I am not sure that rust's embedded dev community will be happy with this idea.

[Apromixately]

I am not completely opposed to this idea. ;)

However, I think using unchecked_ operations with an Overflowing type (just like for checked_ and Wrapping) is sufficiently pleasant.

Having a compiler flag is dangerous because people will copy a list of "optimizations" from stackoverflow, disabling the checks again without knowing what they are doing. This may seem silly but as a security engineer I can only assure you that it is a real problem and I would like to avoid it.

I also don't see why embedded engineers would be less likely to care about overflow errors in their programs and want to blanket disable such checks. Sure, speed might be more important but probably not to the point of not caring about correctness anymore.

[hanna-kruppe]

We already have the flag -C overflow-checks=[on|off]. Is the proposal here to make it default to on in release mode, or to remove it (or rather, make it do nothing) altogether?

I'm leaning in favor of changing the default but this is probably contentious enough that we'd want an RFC that lays out the motivation and downsides (and ideally, hard benchmark data).

[Apromixately]

@rkruppe Yes, it should default to on and ideally be removed in favor of a more selective way of disabling it. The latter is probably more work and more controversial but imho the right thing to do™.

[leonardo-m]

Before introducing overflow checks on default I suggest to introduce a refined compiler pass like this one from Swift:

https://github.com/apple/swift/blob/master/lib/SILOptimizer/Transforms/RedundantOverflowCheckRemoval.cpp

[Apromixately]

@leonardo-m That is a great idea but I still think we should enable it first and then do that. The selling point of rust is that it is safer than C, so we should emphasize and prioritize that.

[hanna-kruppe]

Greater safety while also offering competitive performance and control is a selling point, too. Regressing the performance of the default configuration is sure to rustle some feathers, and making sure to quantify and reduce the slowdown will be very useful both for building goodwill and for convincing people to actually accept the new default.

Aside: You're using "safety" in a different sense here than most Rust material. I don't want to quibble about terminology, but I do want to be clear that lack of overflow checks does not jeopardize the guarantees that Rust offers (lack of use-after-free, type safety, data race freedom, etc.).

tl;dr: nevermind, ignore this post & don't read this - because it's also not in the proper place!
Actually maybe EDIT3 below has a point.ok nvm

Reading this from OP:

It would be good to always check integers for overflow and thereby providing users with an integer type that actually behaves like an integer or at least fails completely instead of giving "wrong" results.

made me believe that you (also?) meant that, in the following example(playground), Rust should have inferred the type of 'i64'(instead of 'i32') for 'b':

#![feature(core_intrinsics)] //must be crate-wide ie. #![]
fn print_type_of<T>(_: &T) { //src: https://stackoverflow.com/questions/21747136/how-do-i-print-the-type-of-a-variable-in-rust/29168659#29168659
    println!("{}", unsafe { std::intrinsics::type_name::<T>() });
}

fn main() {
    let a=1.0;// defaulting to f64
    print_type_of(&a);// f64
    println!("{}",a);// 1
    
    let b=-4000000000;// defaulting to i32 despite the 'literal out of range for i32'
    print_type_of(&b);// i32
    println!("{}",b);// 294967296
}

Btw is there an issue for this ^ ? I only stumbled upon PR #20189 so far.

EDIT: I guess I found one #8337 (comment)
EDIT2: Actually, nevermind, since this behaviour is documented as such: "Rust defaults to an i32, which is the type of secret_number unless you add type information elsewhere that would cause Rust to infer a different numerical type."
EDIT3: actually, now that I think about it, I think that the integer literal itself ( -4000000000 that is) should be the one that gets its type inferred to be i64, and thus it would be equivalent to -4000000000_i64 which would in fact ensure that b infers to i64 correctly!
EDIT4: ok, inferring type of integer literals may only work in this case but not in others, so, bad idea; it could maybe infer a minimal type that would be required in order to hold the value, and have b coerced to at least that; but this likely introduces too much complexity internally.

[andrewchambers]

Just anecdotally, lots of permission check bypasses in the linux kernel have been from integer overflows.

[daxpedda]

Before I start writing I just want to point out that I'm completly new to Rust (2 days in) and I may be wrong about everything that I'm going to write here.

I noticed some inconsistencies on the default state of overflow checks:

• (2.) there is already Wrapping but (3.) should also be annotated requiring people to assert that they have done their homework

  This was not talked about in this issue, but I completly agree that it needs a different annotation. Wrapping indicates that the number is supposed to overflow and that it is intended, no overflow checks should be used in either debug or release builds. But numbers that are marked as unchecked should only be used for performance reasons and it would be extremly useful to have them checked in debug builds but unchecked in release builds. This creates somehow of a mess where you end up with three different "types" (or however this is implemented) of numbers:

  • Default numbers, as proposed by this issue, they are overflow checked in both debug and release builds.
  • Wrapping numbers, which are intentionally overflowing and are not checked in either builds.
  • Unchecked numbers, which are checked in debug builds and unchecked in release builds.

  A new question would then arise: what exactly is the point of overflow-checks in rustc then?
  EDIT: I guess a good answer to that is that people who don't care about overflow checks don't have to deal with a new type and can just have the arithmetic performance they are used to from other languages.
  Also checked_add and all the other corresponding operations would become kind of useless except for annotation purposes.

• The division and remainder operator also do overflow checks, but they do it in both debug and release builds, even if you set overflow-checks=off. As far as I can see the only way to disable the checks is to use wrapping_div, wrapping_rem, unchecked_div or unchecked_rem. I don't know if that is intentional, but it looks like a bug to me.
  On a sidenote here, this has nothing to do with overflows, but there is currently no way to disable the division-by-zero check except with unchecked_div and unchecked_rem which are both hidden behind nightly, I find this very odd, it should just be available through unsafe, right?

• as does not perform overflow checks, neither in debug or release mode. I understand that the nomicon doesn't say that it performs these checks, but currently I can't find a different way to cast between integers. Looking at Tracking issue for TryFrom/TryInto traits #33417. I also read in a couple of places that it was changed but reverted later on.

• It's not properly and clearly stated when overflow checks are enabled and when they are not.

  • The rust reference would be the place where it should be clearly stated in my opinion. But it only says:

    Other kinds of builds may result in panics or silently wrapped values on overflow, at the implementation's discretion."

    Which is not clear enough in my opinion. But it looks intentional to me, and if it is then this point is moot I guess, because cargo clearly states it.

  • The only place where its definitive is in the cargo documentation with overflow-checks in the profile sections.

  • A 2nd mention in the rust reference talks about how overflow checks are enabled in debug, but not that they are disabled in release.

  • It's clearly stated in the 2018 edition.

---

With that said, in my opinion overflow checks should be enabled by default, a new type std::num::Unchecked corresponding to std::num::Wrapping should be introduced, a unchecked_add corresponding to wrapping_add for all operators should be added and both things should be marked as unsafe.

[pnkfelix]

This >3 year old comment from Niko rust-lang/rfcs#560 (comment) seems particularly relevant to this discussion.

In particular this sentence:

Of course the plan is to turn on checking by default if we can get the performance hit low enough!

[leonardo-m]

Even if we don't turn on checking by default, I think it's still a good idea to add some math optimizations like in Swift language for debug builds.

[OnlyOneCannolo]

FWIW, Ada has a concept of modular and non-modular types, where by default signed types have checked overflow resulting in an exception, and unsigned types have modular semantics.

I'm not advocating for any particular approach, or even weighing in on previous discussion or existing options in rust. I'm just calling attention to a similar situation in another language.