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.).

[ghost]

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.

[hanna-kruppe]

We have Wrapping<T> corresponding to "modular semantics" (edit: modular, not non-modular).

[ghost]

@rkruppe Shouldn't Wrapping be the one corresponding to modular semantics? In any case I think the best behavior is what @daxpedda and I suggested but it is only really possible if the checks can be made fast enough.

[najamelan]

Apologies if this is completely not applicable here, but Capnproto author Kenton Varda claims to do static out of integer overflow checking. I don't entirely understand how that's possible, but if it is, I reckon rust should probably have it.

[ts826848]

@najamelan At least based on a quick reading, that particular kind of overflow checking is done by embedding the maximum possible logical value for an integral type into the type and tracking that maximum value across operations. It would work fairly well if you use a relatively small portion of an integer type's possible range, but isn't really a general solution.

I think const generics and const fns would be needed for a straight translation of the C++ code to Rust. I'm not sure whether what has landed so far would be enough, but in principle it's possible.

[matu3ba]

Without unwinding we get something like 1.status flag clear, 2.operation, 3.status flag read,4.mask flag, 5.status flag mask, 6.compare to nullregister, 7.njump. That should be still 2x-3x the cost of multiplication/addition per check.

It would be nice, if the issue could be updated to reflect the status quo or closed after the overflow RFC
and overflow checks.
Specifically uncompromised speed was taken instead of safety, since the tradeoff in debug mode was seen as acceptable.

[njor]

Hi,
I am currently learning rust, and found this issue while looking up the overflow behavior, but not for arithmetic operations, but for casts, and was surprised that the most « simple » casts (ie. using the as keyword) did not check for overflow at all, even in debug mode, and even with the compiler option overflow-checks explicitly set to true.
I am not the only one stumbling on this it seems, as there are for example questions like this one (from 2015) on stack overflow.

Is there a compiler option to add overflow checks by default when casting an integer with as ? I did not find any.
If not, maybe such an option could be added ?

Personally, I would really prefer to have checks by default every time an integer can overflow, even for « simple » casts (those that everyone will use most of the time), and then have some conversion function that can overflow / truncate without panicking if you need it, but with an explicit name like u8::unchecked_from(), or something like that.

Even if this compiler check never becomes the default (as I understand there are backwards compatibility requirements here), at least having a compiler option to turn it on automatics checks for that as well ?

As for the general discussion of whether to have systematic overflow checks on by default or not, I am in favor of it as well.
I really care about performance, but even if systematic checks were to significantly impact performance of mathematical operations, I was under the impression that for most programs, 99 % of the time is spent doing other things than maths (like waiting for IO operations, allocating / freeing memory, etc.) so most programs might not see any perceptible slowdown because of systematic overflow checks.
And even for those that do, it would probably be limited to a small number of very specific loops where the CPU spends most of its time, so these loops could easily be identified and fixed by replacing the checked number types by a Wrapping type, just for those specific loops.
(And if it is not possible, it would still be possible to disable the runtime checks globally for a whole project, as a last resort.)

I think this way (all overflow checks activated by default, explicitly opt-out if you don’t want it) is much more logical, because if you want performance for number operations and / or want to rely on the overflowing behavior, you will immediately notice the problem (because it always panics), and you can fix it immediately. But the other way around can create bugs that are much more complicated to find and fix.

[matu3ba]

Is there a compiler option to add overflow checks by default when casting an integer with as ? I did not find any.
If not, maybe such an option could be added ?

Use TryFrom.

Even if this compiler check never becomes the default (as I understand there are backwards compatibility requirements here), at least having a compiler option to turn it on automatics checks for that as well ?

as cuts away the bits not needed after transformation. The idea behind is, that the semantics should be equivalent to normal arithmetic for performance. Thus no checking is done and you need to opt-in for further arithmetic/bitpattern checks.

Can you ask on reddit or another forum, why as is not deprecated in favor of From or anything more explicit?