Integer overflow #560

Merged
merged 18 commits into from Feb 6, 2015

Projects

None yet
@nikomatsakis
Contributor
@nikomatsakis nikomatsakis self-assigned this Jan 7, 2015
@sfackler sfackler commented on an outdated diff Jan 7, 2015
text/0000-integer-overflow.md
+ impl WrappingOps for u16
+ impl WrappingOps for i32
+ impl WrappingOps for u32
+ impl WrappingOps for i64
+ impl WrappingOps for u64
+
+These are implemented to wrap around on overflow unconditionally.
+
+### `Wrapping<T>` type for convenience
+
+For convenience, also provide a `Wrapping<T>` newtype for which the operator
+overloads are implemented using the `WrappingOps` trait:
+
+ pub struct Wrapping<T>(pub T);
+
+ impl<T: WrappingOps> Add<Wrapping<T>, Wrapping<T>> for Wrapping<T> {
@sfackler
sfackler Jan 7, 2015 Member

Nit: this is the old definition of Add

@thestinger

If it's ever going to use hardware overflow instructions and cause an error only on-demand, then it can't panic or report precise errors. Having it use debug_assert! means it's going to add a huge amount of overhead to unoptimized builds regardless of any future optimizations. Anyway, it will never be free because operations with potential side effects are clearly going to block code motion optimizations in many cases where pure ones would not.

@thestinger

Slowing down unoptimized Rust even further is certainly relevant. Optimized builds take a long time to compile and there is absolutely no support for debugging optimized code with LLVM. It's inappropriate to claim that the performance hit from this isn't relevant.

@bill-myers

This is an improvement of the current state (not sure if it's the best overall since solutions that don't fail might be better, but much better that silently overflowing).

I think the "wrapping"/modular types should be separate types (which could be called "m8", "m16", etc.), just like signed and unsigned integers are separate types.

This makes sense since they are mathematically different objects, with s32/u32 representing subsets of the infinite ring of integers ("Z"), while the "wrapping" types represent the finite rings of integers modulo 2^k ("Z/2^kZ").

In practice, this means that things like ordered comparisons or integer-like division make no sense on modular types (if needed, the programmer would have to cast to either the corresponding unsigned or signed type, which of course will in general yield different results)

[I'd also be tempted to remove the arithmetic operations from the current non-modular types since they aren't closed under them, but I guess that might be too extreme (the user would be forced to choose between manually calling "checked_add", cast to the modular types and back, or cast to bigint depending on what they want).]

@glaebhoerl
Contributor

@bill-myers Thanks for explaining this more thoroughly. What would the difference be between m32 and u32 using wrapping operations (i.e. the current u32)?

(Of course I'm +1 on the RFC as it is.)

@thestinger I agree that performance and not foreclosing on imprecise exceptions are important. But it's not clear to me that the present RFC would create any constraints in this area. I see this text:

However, when running in debug mode (in other words, whenever a debug_assert! would be compiled in) they are required to emit code to perform dynamic checks on all arithmetic operations that may potentially overflow.

I interpret this to mean "whenever debug_assert!s are enabled, overflow must also be checked (somehow)", as opposed to "overflow must be checked using debug_assert!s".

@arielb1
Contributor
arielb1 commented Jan 8, 2015

By the way, wraparound operations are also useful for "odometer comparison" (which is (a - b) as iKK < 0)

@bill-myers

@glaebhoerl

m32 would be the same as current u32 except it would not support the <, >, <=, >=, /, % and >> operators (and any other where casting to s32 or u32 and back yields a different result). It might also be printed differently, for instance the value 42 as "42 mod 2^32".

@thestinger

@glaebhoerl: So does let _x = int::MAX + 1 report an error? If it must, then it can't ever perform well in the future.

@Aatch
Contributor
Aatch commented Jan 9, 2015

@thestinger it does report an error. No it "must" not do so. If you'd read the RFC, you'd know that overflow produces an unspecified value. In builds where a debug_assert! is not removed (currently the absense of a --cfg ndebug flag), it will report an error.

@Aatch Aatch referenced this pull request in rust-lang/rust Jan 9, 2015
Closed

Implement arithmetic overflow changes #20795

@thestinger

If you'd read the RFC, you'd know that overflow produces an unspecified value.

I've read the RFC and I'm responding to it.

In builds where a debug_assert! is not removed (currently the absense of a --cfg ndebug flag), it will report an error.

So it strictly reports an error on overflow by default which is the only case that really matters. Even Rust's own nightly releases aren't build with ndebug. You don't get to hand wave away significant performance issues because there's an obscure way to turn it off where the chosen semantics simply aren't enforced properly.

@huonw
Member
huonw commented Jan 9, 2015

So it strictly reports an error on overflow by default which is the only case that really matters. Even Rust's own nightly releases aren't build with ndebug. You don't get to hand wave away significant performance issues because there's an obscure way to turn it off where the chosen semantics simply aren't enforced properly.

The current build system defaults shouldn't influence language design; in any case, the defaults should be changed, and somewhat coincidentally there was actually some discussion along these lines in #rust-internals in the past few days (it's no-one's problem if they missed that discussion, just some extra commentary :) ).

If it's ever going to use hardware overflow instructions and cause an error only on-demand, then it can't panic or report precise errors. Having it use debug_assert! means it's going to add a huge amount of overhead to unoptimized builds regardless of any future optimizations. Anyway, it will never be free because operations with potential side effects are clearly going to block code motion optimizations in many cases where pure ones would not.

I wonder if we could tweak this part of the detailed design

However, when running in debug mode (in other words, whenever a debug_assert! would be compiled in), they are required to emit code to perform dynamic checks on all arithmetic operations that may potentially overflow.

slightly, to more explicitly allow e.g. coalescing multiple checks so that each possibly-overflowing operation may not be checked immediately, while still guaranteeing that any overflow will detected at some point (in any case, this doesn't seem to be disallowed AFAICT). Also, I'll note that, as written, the RFC does not require the use of debug_assert itself, just that whatever mechanism is chosen is active when debug_asserts are.

@thestinger

The current build system defaults shouldn't influence language design

The ridiculously poor performance of Rust's debug builds is a major problem, and this is an enormous step in the wrong direction. The RFC _is_ heavily influenced by the current compiler implementation because it's based on the assumption that debug builds are already slow. However, it's going to severely aggravate that problem and the inability to mix debugging with optimizations is a fixable LLVM issue which is not there for other compilers like GCC. It's the norm to use an optimized debug build with GCC (-Og -g or even -O2 -g) because it doesn't lose any of the information when it's doing stuff like inlining.

You can't claim that language design shouldn't be influenced by the build system defaults while supporting an RFC that's explicitly designed around a concept of debug and release builds.

@thestinger

It's absurd to draw a line between language and implementation in order to ignore the fact that out-of-the-box performance will be severely impacted. The practical implications of an RFC are very relevant and cannot be ignored by refusing to talk about anything but pure programming language design. Rust's optimization process takes a very long time so the performance of unoptimized builds is not unimportant considering the productivity loss from optimizing.

I can't see it as a step in the right direct unless it's an explicitly an opt-in feature in all cases. It would be fine to tie it to debug_assert! as long as that's never enabled by default. Otherwise, the language is just becoming even more unusable without gaining a significant level of robustness in return.

@thestinger

How many real overflow bugs were found while converting the compiler? That's a 200k+ line code so if it has any value at all there should be dozens of bug fixes to point at. Each case of intentional overflow is a case where this feature had a negative impact. The ratio between those is what determines the value of the feature vs. the cost of the feature. An acceptable ratio is a subjective issue, but the ratio itself is simply an important number to discuss here. You may be willing to accept hundreds or thousands of cases of "friction" in order to catch one bug, but it will drive away people would find the language too abrasive.

@huonw
Member
huonw commented Jan 9, 2015

the inability to mix debugging with optimizations is a fixable LLVM issue which is not there for other compilers like GCC. It's the norm to use an optimized debug build with GCC (-Og -g or even -O2 -g) because it doesn't lose any of the information when it's doing stuff like inlining.

I'm afraid I don't understand why this current failing of LLVM is important. Could you expand on why integer overflow detection would be influenced by LLVM being able to build optimised debuggable code?

You can't claim that language design shouldn't be influence by the build system defaults while supporting an RFC that's explicitly designed around a concept of debug and release builds.

I was specifically talking about Rust's makefiles currently defaulting to passing --cfg ndebug (as, I think, you were above), not the more abstract concept of a debug/release build.

Phrasing it the latter terms: Rust current defaults to a sort-of debug build, we could/should make it default to a release build. This seems mostly independent of detecting integer overflow in debug builds, to me?

It's absurd to draw a line between language and implementation in order to ignore the fact that out-of-the-box performance will be severely impacted.

I'm not doing that, I personally haven't said anything about performance or otherwise, just correcting/clarifying/commenting on some subpoints made earlier in this discussion, trying to keep everything as true (given what I personally understand) as possible. Whether this leads us to doing overflow detection or not is another matter.

It would be fine to tie it to debug_assert! as long as that's never enabled by default.

Are you saying that you would be OK with the RFC if it was adjusted to explicitly require using the debug_assert! macro, in an opt-in manner? (NB. it currently doesn't require using debug_assert! at all, it doesn't even require unwinding.)

@thestinger

I'm afraid I don't understand why this current failing of LLVM is important. Could you expand on why integer overflow detection would be influenced by LLVM being able to build optimised debuggable code?

I'm not the one tying it to debug vs. release builds.

I was specifically talking about Rust's makefiles currently defaulting to passing --cfg ndebug (as, I think, you were above), not the more abstract concept of a debug/release build.

Phrasing it the latter terms: Rust current defaults to a sort-of debug build, we could/should make it default to a release build. This seems mostly independent of detecting integer overflow in debug builds, to me?

I'm not talking about Rust's build system. I'm talking about rustc. I don't think it's acceptable to have this performance hit by default with a flag for opting out of it. I'm strongly against doing anything like this unless it's clear that the performance hit is opt-in.

I'm not doing that, I personally haven't said anything about performance or otherwise, just correcting/clarifying/commenting on some subpoints made earlier in this discussion, trying to keep everything as true (given what I personally understand) as possible. Whether this leads us to doing overflow detection or not is another matter.

You claimed that the defaults used by rustc are not relevant... I think it's very relevant if there's going to be a 20-100% performance hit unless people remember to pass an extra flag.

Are you saying that you would be OK with the RFC if it was adjusted to explicitly require using the debug_assert! macro, in an opt-in manner? (NB. it currently doesn't require using debug_assert! at all, it doesn't even require unwinding.)

I'm saying that I would be fine with it if it was made clear that this isn't going to be enabled when you use rustc foo.rs or rustc -O foo.rs and it's shown that it catches enough bugs to make up for the extra friction. If it hasn't caught a significant number of bugs in rustc, that's an indication that the added friction didn't come with any benefits.

@huonw
Member
huonw commented Jan 9, 2015

I'm not talking about Rust's build system. I'm talking about rustc. I don't think it's acceptable to have this performance hit by default with a flag for opting out of it. I'm strongly against doing anything like this unless it's clear that the performance hit is opt-in.

You said above:

Even Rust's own nightly releases aren't build with ndebug.

I was refuting the relevance of this.

You claimed that the defaults used by rustc are not relevant... I think it's very relevant if there's going to be a 20-100% performance hit unless people remember to pass an extra flag.

I specifically claimed that:

The current build system defaults shouldn't influence language design

We should evaluate this feature on its own merits, not on what we happen to do in some Makefiles right this minute. Once the feature is accepted, we then chose the defaults for rustc appropriately. (I agree that the current choice is probably not what we want---with or without integer overflow detection---but fortunately it's easy to change.)

@thestinger

I was refuting the relevance of this.

I don't see how. It was an example of why the rustc default is what people will use. You're misrepresenting it by taking it out of context and pretending it was an argument about the Rust build system.

We should evaluate this feature on its own merits, not on what we happen to do in some Makefiles right this minute.

I was never talking about build system defaults, so I don't know why you keep coming back to this.

Once the feature is accepted, we then chose the defaults for rustc appropriately. (I agree that the current choice is probably not what we want---with or without integer overflow detection---but fortunately it's easy to change.)

The merits of the feature certainly depend on the interaction with the rest of the language and tooling. There is no indication that the compiler's default for ndebug is going to be changed so you can't hand wave the problem away. It's clear misdirection.

If you don't want to talk about the issue of a significant out-of-the-box performance hit then the RFC should make it clear that the feature is going to be opt-in with the reference implementation. Otherwise, the fact that it's significantly hurting performance by default is very relevant.

@bill-myers

Correctness is more important than performance.

The current version of Rust silently fails to provide fundamental properties of integers such as that a < b implies a + 1 < b + 1.

Also, this must be on by default at least for debug builds since otherwise there will be code that relies on overflow checking being off, which means that overflow checking becomes unusable since turning it on will break things.

I think it should be on by default for production builds too, since performance doesn't matter for most software (consider how much software is written in languages with horrible performance due to bad design such as Python or Ruby), and anyway integer arithmetic should actually be rare outside integer-based algorithms (which can be written with modular integers or with scoped overflow checking attributes).

@thestinger

I think it should be on by default for production builds too, since performance doesn't matter for most software

In that case there's little reason to write the software in Rust. You should be using big integers if you don't want to spend the time carefully thinking about bounds.

The current version of Rust silently fails to provide fundamental properties of integers such as that a < b implies a + 1 < b + 1.

Replacing the return value with an unspecified value or a panic is strictly a step backwards in this regard. It will have strictly fewer guarantees than it does today... The unsigned types already provide all of the fundamental properties that would be expected of them for addition, subtraction and multiplication. Division is always going to be weird because we pretend it's a total function when it's not.

and anyway integer arithmetic should actually be rare outside integer-based algorithms

Integer arithmetic is anything but rare... and there are way more issues with floats in addition to this. Rust doesn't provide any of the high-level, sane number types like big integers or rationals.

@thestinger

Correctness is more important than performance.

Clearly Rust should be a purely functional language with dependent typing. This is just bullshit rhetoric and is not contributing to the discussion at all. Rust has no purpose without good performance, and integer overflow checking pushes it behind C# and Java.

Code with an overflow bug is incorrect whether this feature is in place or not. It is an improvement in a situation where ending the execution of the program is better than continuing on with an unintended overflow. Since using it for the compiler (200k+ LOC) does not appear to have uncovered any bugs, I think the value of the feature has been greatly exaggerated.

@glaebhoerl
Contributor

@thestinger

So does let _x = int::MAX + 1 report an error? If it must, then it can't ever perform well in the future.

I think it should, but not necessarily at that exact point. (As far as I can tell the RFC doesn't talk about this directly, i.e. doesn't rule it out. More explicit text might be beneficial as @huonw notes.)

@thestinger

I think it should, but not necessarily at that exact point.

Okay, so it can't be a fast implementation of overflow checking. I'm not sure that such a thing can exist anyway considering the every increasing importance of vectorization, etc.

@thestinger

An issue that this proposal leaves out is the divergence in semantics for scalar and vector types. Why does it make sense for integer arithmetic to have different semantics when it's done with vectors? Either way, there is no hardware support for it and it's slow. If it's really going to do checking, then it should be doing it across the board. At the moment, it's adding friction, hurting performance and making the language inconsistent with itself.

@thestinger thestinger commented on an outdated diff Jan 9, 2015
text/0000-integer-overflow.md
+they are *required* to emit code to perform dynamic checks on all
+arithmetic operations that may potentially overflow.
+
+The goal of this rule is to ensure that, during debugging and normal
+development, overflow detection is on, so that users can be alerted to
+potential overflow (and, in particular, for code where overflow is
+expected and normal, they will be immediately guided to use the
+`WrappingOps` traits introduced below). However, because these checks
+will be compiled out whenever an optimized build is produced, final
+code wilil not pay a performance penalty.
+
+In the future, we may add additional means to control when overflow is
+checked, such as scoped attributes or a global, independent
+compile-time switch.
+
+## `WrappingOps` trait for explicit wrapping arithmetic
@thestinger
thestinger Jan 9, 2015

Swift's way of doing this is both more explicit and easier to use. This adds a lot of friction to using wrapping arithmetic, and Rust already suffers a lot from that issue.

@thestinger thestinger commented on an outdated diff Jan 9, 2015
text/0000-integer-overflow.md
+
+Reasons this was not pursued: The proposed changes are relatively well-contained.
+Doing this after 1.0 would require either breaking existing programs which rely
+on wraparound semantics, or introducing an entirely new set of integer types and
+porting all code to use those types, whereas doing it now lets us avoid
+needlessly proliferating types. Given the paucity of circumstances where
+wraparound semantics is appropriate, having it be the default is defensible only
+if better options aren't available.
+
+## Checks off means wrapping on
+
+Where overflow checks are turned off, instead of defining overflow as returning
+an unspecified result, define it to wrap around. This would allow us to do
+without the `WrappingOps` trait and to avoid having unspecified results. See:
+
+ * [Daniel Micay on June 24][DM24_2]
@thestinger
thestinger Jan 9, 2015

I don't want this, it was just an idea thrown into a discussion. I do a lot of that.

@thestinger thestinger commented on the diff Jan 9, 2015
text/0000-integer-overflow.md
+ ...
+ }
+
+ ...
+
+[40]: https://github.com/rust-lang/rfcs/blob/master/active/0040-more-attributes.md
+
+## Different operators
+
+Have the usual arithmetic operators check for overflow, and introduce a new set
+of operators with wraparound semantics, as done by Swift. Alternately, do the
+reverse: make the normal operators wrap around, and introduce new ones which
+check.
+
+Reasons this was not pursued: New, strange operators would pose an entrance
+barrier to the language. The use cases for wraparound semantics are not common
@thestinger
thestinger Jan 9, 2015

Wraparound semantics are commonly desired in systems programming. I think this claim is complete bullshit, and the people who came up it are application programmers without an understanding of this niche. It's incredibly common to rely on wraparound semantics for things like sane counters.

I think it would be far more accurate to say that there is little use for fixed-size integers at an application programming level. It's almost always the case that a big integer is desired at that level, and Rust's widespread misuse of fixed-size integers is misguided.

@thestinger
thestinger Jan 9, 2015

Swift doesn't have these operators simply for the sake of having them. I think it's a bit extraordinary to make the claim that they were entirely misguided by adding them, considering that it's not even intended to be a low-level systems language.

@glaebhoerl
glaebhoerl Jan 9, 2015 Contributor

I fully admit to having only a superficial acquaintance with systems programming, or with use cases for wrapping arithmetic in particular, which is why I chose to ask other people about it. My attitude, way back last summer when I started the mailing list thread, was, "the only use case for wrapping arithmetic I can think of off the top of my head is for hashing, but surely there must be many more?". That was followed by a really long discussion with a wide range of participants, in which the only other use cases anyone managed to identify were checksums and emulation. This frankly was a surprise to me, and informed the RFC. Half a year later, someone did eventually bring up counters and deltas in the other PR (which albeit still doesn't give a good idea of prevalence). Apparently "an understanding of this niche" is not so easy to come by.

Either way, this doesn't make me change my belief that "I expect this to use wraparound semantics on overflow" should (somehow) be distinguished from "I expect this not to overflow" in the source. If the former turns out to be more commonly desired than I had assumed, then more convenient syntax for it can still be added backwards compatibly at any point.

I think it's a bit extraordinary to make the claim that they were entirely misguided by adding them

Be kind to point out where I made that claim.

I think it's a judgment call and possibly a question of different priorities. Rust has very studiously avoided introducing new symbolic operators on top of the ones C/C++ have (apart from the old built-in smart pointers). Swift, by contrast, allows user-defined operators.

@mahkoh
mahkoh Jan 10, 2015 Contributor

That was followed by a really long discussion with a wide range of participants, in which the only other use cases anyone managed to identify were checksums and emulation.

The current RingBuf implementation could be faster (10% iirc) if it used wrapping arithmetic. In some cases, e.g. popping n elements, the current implementation is O(n) while the wrapping implementation is O(1) after optimization.

@bill-myers

The unsigned types already provide all of the fundamental properties that would be expected of them for addition, subtraction and multiplication.

They do if you want to work in Z/2^kZ, but they definitely don't if you want to work in Z, which is typically the case.

Abstractly, you get a finite ring with non-zero characteristic with an order not consistent with the ring operations rather than the expected ordered countable ring with zero characteristic and no zero divisors.

In "common" terms, this has very undesirable implications such as that a < b doesn't imply a + 1 < b + 1 , that adding a number to itself repeatedly eventually results in 0 and that ab = 0 doesn't imply that a = 0 or b = 0.

A typical security catastrophe that happens as a result of this is checking that a + b < limit and assuming that this also checks that a < limit and b < limit (since of course that's true with non-negative integers), while this is not the case with C-like unsigned integer types.

Replacing the return value with an unspecified value or a panic is strictly a step backwards in this regard. It will have strictly fewer guarantees than it does today

Why? It guarantees that, as long as some code doesn't trigger a panic due to overflow error, then all computations behave as if they were done with mathematical integers / bigints.

That's a step forward from giving meaningless results.

There are still issues like that operations are no longer associative if you don't exclude panics, but at least either everything is correct or it fails visibly.

FWIW I think it's theoretically much better to just not provide addition and multiplication at all for fixed-size integer types or having the result be a type large enough to hold all values, but that requires massive code changes unlike overflow checking.

It is an improvement in a situation where ending the execution of the program is better than continuing on with an unintended overflow

Aborting the execution of a program is in general better than continuing with wrong results, since the former is detectable and does not cause harm beyond unavailability.

In security terms, it means that a bug leads to a denial of service, completely reversible by fixing the program, as opposed to an irreversible compromise, where secret data is acquired by unauthorized exploiters and physical systems are actuated to cause damage and nothing can be done to reverse that.

@Aatch
Contributor
Aatch commented Jan 9, 2015

The merits of the feature certainly depend on the interaction with the rest of the language and tooling. There is no indication that the compiler's default for ndebug is going to be changed so you can't hand wave the problem away. It's clear misdirection.

I have filed #563 to address the use of ndebug since I consider it a wart anyway. The proposed change would remove the overflow checks for optimised builds, while retaining them for unoptimised builds.

@thestinger

A typical security catastrophe that happens as a result of this is checking that a + b < limit and assuming that this also checks that a < limit and b < limit (since of course that's true with non-negative integers), while this is not the case with C-like unsigned integer types.

You've provided no evidence that this is a typical security bug. Rust is already immune from nearly all of the typical integer overflow bugs. Can you point to any security-relevant Rust or Servo bug fix where this would have helped? I don't think so, because 99% of the cases where it will matter are low-level, performance critical unsafe code used to build safe abstractions and the checks are never going to be used there.

Why? It guarantees that, as long as some code doesn't trigger a panic due to overflow error, then all computations behave as if they were done with mathematical integers / bigints.

That's quite misleading, considering that the universe doesn't simply end when a panic happens. It is a side effect and even does I/O by default... good luck fitting that into a set of mathematical laws.

Aborting the execution of a program is in general better than continuing with wrong results, since the former is detectable and does not cause harm beyond unavailability.

That's true in some domains and not in many others. A gamer is not going to be happy if the game aborts because of one of many minor overflow bugs, and there is no way that the software is going to be tested enough to identify them all.

In security terms, it means that a bug leads to a denial of service, completely reversible by fixing the program, as opposed to an irreversible compromise, where secret data is acquired by unauthorized exploiters and physical systems are actuated to cause damage and nothing can be done to reverse that.

I think that's a pretty big stretch. Integer overflow bugs resulting in memory unsafety are very common but Rust is already immune to that. These automatically inserted checks are always going to be disabled in any of the low-level code that's using unsafe for performance. You're not providing any evidence that this is going to eliminate a large class of security bugs from Rust programs. It's one of many possible correctness issues and I don't see what makes it compelling enough to pay a huge performance cost.

@tomjakubowski
Contributor

They do if you want to work in Z/2^kZ, but they definitely don't if you want to work in Z, which is typically the case.

I think the majority of C and C++ programmers are already accustomed to this characteristic of unsigned integers.

FWIW I think it's theoretically much better to just not provide addition and multiplication at all for fixed-size integer types or having the result be a type large enough to hold all values, but that requires massive code changes unlike overflow checking.

Forbidding addition and multiplication completely on fixed-sized integer types seems a bit extreme.

@thestinger

@Aatch: I strongly agree with that RFC but I still think that it's a problem if unoptimized builds are significantly slower and more bloated than they already are without passing an extra flag.

@thestinger

I think the majority of C and C++ programmers are already accustomed to this characteristic of unsigned integers.

It's also how the types work in languages like C#. Java and C# have well-defined signed overflow, as Rust does at the moment. I'm not aware of any precedent for what Rust is going to do...

Forbidding addition and multiplication completely on fixed-sized integer types seems a bit extreme.

Wouldn't it be nice if Rust had usable and fast big integers? Then we could simply limit fixed-size integers to cases where the bounds are clearly in place, which is the only sane way forwards. Instead, the core developers have done everything they can to make big integers harder to use by ripping out support for them from the standard library and destroying the ability to write generic numeric code - even at a language level via the operator traits. At the same time, they're pushing for adding more friction to the language to tackle the overflow problem - which they have exasperated by forcing widespread abuse of fixed-size integers. I have no doubt that this is just a step towards ruining performance by default down the road.

@bill-myers

A gamer is not going to be happy if the game aborts because of one of many minor overflow bugs, and there is no way that the software is going to be tested enough to identify them all.

It's perfectly possible to cause catastrophes in games with integer overflow, for instance by having a currency amount wrap down from 0 to a huge unsigned value, causing massive economical effects in an online game.

I can think of the "Black Sunday" bug in Kingdom of Loathing from memory as an example (http://kol.coldfront.net/thekolwiki/index.php/Black_Sunday).

It's also how the types work in languages like C#. Java and C# have well-defined signed overflow, as Rust does at the moment. I'm not aware of any precedent for what Rust is going to do...

I think Ada has overflow checking, Python has bigints by default, JavaScript has floats by default.

Instead, the core developers have done everything they can to make big integers harder to use by ripping out support for them from the standard library and destroying the ability to write generic numeric code.

This needs to be fixed.

Overall, the thing is that the approach in this RFC is the only to way to:

  1. Not have silently broken "integers"
  2. Not require massive code changes
  3. Not require using bigints everywere

If one is willing to accept massive code changes, then there are other possibly better options such as not having addition/multiplication at all on signed/unsigned fixed-size integers (and then the user either uses bigints, or checked_add or casts to modular types and back), or extending the return type, i.e. having "u32 + u32" give an u33 result (or more correctly, an [0, 2^33 - 2] range integer type result).

But maybe it's better to start with the thing that doesn't require massive changes and once overflow checking is accepted, perhaps considering the more drastic moves.

@thestinger

It's perfectly possible to cause catastrophes in games with integer overflow, for instance by having a currency amount wrap down from 0 to a huge unsigned value, causing massive economical effects in an online game.

It's far more common for a crash to cause more harm than the overflow. Cherry picking an example or two showing that overflow can be a problem doesn't demonstrate that it's a smaller issue than the added crashes.

I think Ada has overflow checking, Python has bigints by default, JavaScript has floats by default.

All of those are examples of languages not doing what is proposed in this RFC. I said that there's no precedent for the proposal, not that there aren't languages doing other things.

This needs to be fixed.

It's not going to be fixed - this stuff has been stabilized and a rushed 1.0 release is right around the corner. Big integers lost and aren't going to be supported well. Instead, Rust encourages widespread abuse of fixed-size integers when the bounds aren't known, which is clearly idiotic.

@bill-myers

It's far more common for a crash to cause more harm than the overflow

Well, the generally accepted principle is that crashes are better than incorrect operation for generic programs.

If this principle is rejected, then I don't see why Rust should keep bounds checking on reads, for instance ("It's far more common for a crash to cause more harm than reading from a random place in memory"), and overall such a stance does not seem compatible with the design principles of Rust.

Big integers lost and aren't going to be supported well.

Are there issues with bigints that cannot be fixed without breaking backward compatibility? Looks like those should get an RFC to fix them before it's too late.

@nikomatsakis
Contributor

@thestinger

Clearly Rust should be a purely functional language with dependent typing. This is just bullshit rhetoric and is not contributing to the discussion at all. Rust has no purpose without good performance, and integer overflow checking pushes it behind C# and Java.

This is clearly over the top. Keep in mind that we're debating the semantics of unoptimized debug builds. I think @bill-myers's point is solid that overflow bugs are subtle and non-obvious, and we want to catch them, and I think we can make it much easier to do without costing us performance. More on this in a second.

@thestinger

This is clearly over the top.

What is over the top is claiming that the language is not correct if it does not always perform these checks and that fixed-size arithmetic should be removed.

@thestinger

that overflow bugs are subtle and non-obvious, and we want to catch them

If it doesn't catch any in 200k+ lines of the Rust codebase while self-hosting and running tests, then it doesn't seem to have much value. What is the scale of the code before this is expected to find one bug? If it's enabled in production, then it will have an impact - but how common are silent overflows that occur in testing without any obvious side effects?

@thestinger

I think 99% of the cases of overflow being a security bug are already covered by Rust's memory safety or are in low-level unsafe code where these checks are never going to be used, and the remaining issues generally occur in high-level application logic where it makes no sense to use fixed-size integers because there are no clear bounds.

@nikomatsakis
Contributor

@thestinger I've read all of your comments and would like to respond to some of your points. As usual, you've raised some interesting concerns. I think I can summarize a couple of major points that you made as follows:

  1. There are (or may be in the future) efficient means of detecting overflow that do not necessarily give precise results. [cite]
  2. Vectorization semantics and normal addition semantics will diverge. [cite]
  3. Having a large performance hit on simple workflows is a problem (put another way, users should always opt-in to overflow checks). [cite]
  4. Overflow bugs are not worth taking a performance hit because we still give strong memory safety guarantees. [cite]

Let me respond to each point in turn, then I'll add a few questions at the end.

Point 1: There are (or may be in the future) efficient means of detecting overflow that do not necessarily give precise results.

You are correct that the RFC did not address the question of precision, and I will update it to address that. I think that as far as the normative spec is concerned, precise results should not be guaranteed. This leaves us the most options going forward.

Non-normatively, I am not yet sure if I think that giving imprecise errors is a good idea. To the extent that it permits more widespread checking (particularly, checking in optimized builds), it's obviously good. But then it will make errors harder to debug: so I guess it depends on the performance win that results. I'm happy to just let compilers (and users) set their policy on this point as they choose. As far as I know, this is largely a speculative point anyway, right? Or do architectures expose the kind of features you're talking about today? (If so, cool!)

Note also that in an optimized build, where assertions are not guaranteed, compilers could still choose to use checks that only panic if the overflowing result is actually used. I believe this is within the proposed rules. In particular, if checks are enabled, all overflows must panic, but if not, the compiler may either panic or substitute an undefined value as the result. I will make it clear than (1) whenever an error results in panic, the precise source location of the panic is not defined, though compilers are encouraged to give precise results when possible and (2) that if assertions are not enabled, it is not required that the choice between error or undefined result be the same for all executions, and hence we can have branch-dependent errors (only if the result is used).

(Regarding the use of debug_assert!, I did not say nor intend to say that inserting overflow checks would be done by literally adding a debug_assert! into the code, but simply that the two ought to be enabled/disabled (by default) under similar circumstances.)

Point 2. Vectorization semantics and normal addition semantics will diverge.

It seems to me that this only affects the ability to auto-vectorize code. If you use explicit vectorization intrinsics, they come along with a specific semantics, and I think it is ok for this to be different that the equivalent for loop in subtle cases. So I assume we're talking about non-vectorized code that is being optimized to use vectorized instructions under the hood.

In that case, we are already assuming that optimizations are enabled, and hence that the semantics of overflow are undefined, so vectorization should not be a problem. It is of course true that if the user explicitly requests overflow detection, then our ability to optimize will be hindered, but then the user did ask for more safety, so I don't see the problem there.

Point 3. Having a large performance hit on simple workflows is a problem (put another way, users should not have to opt-in to overflow checks).

I think here we just disagree. I think that of course we want debug builds to be fast, but even more we want them to test and catch errors. In all major projects I've worked on, the workflow has included three large categories of builds:

  1. "debug" -- no optimizations, all assertions enabled, debug-info included. This is what people use most of the time when testing and debugging their code locally. It is also generally tested by the CI infrastructure, but may not be for all platforms or configurations.
  2. "optimized" -- all optimization enabled, all assertions disabled, debug-info possibly stripped. This is what ships to the end user. It is definitely tested by the CI infrastructure.
  3. "opt-debug" -- all optimizations enabled, debug assertions enabled, debug-info included. This is what you use when you have a problem that doesn't seem to show up in a debug build for whatever reason. Hopefully it occurs here.

Now, depending on the bug, there may be a reason to do something else. For example, it can be useful to turn on assertions only within a specific portion of the code, or to enable debug-info but not debug assertions etc, but those are uncommon cases.

I expect that we will basically align cargo with these modes. Currently cargo lacks an opt-debug mode, and I'm not entirely sure of the behavior of cargo build --release, but anyway. Those are cargo issues as far as I'm concerned.

Beyond helping users debug their code, there is another reason that I think it's very important to have overflow checks in debug builds by default. Basically, I want to make it very, very likely that all cases of intentional overflow will be redirected to the appropriate wrapping intrinsics. This in turns means we can safely ratchet up the overflow checks in the future without breaking pre-existing Rust code. I think that if you cannot use your library in debug mode without causing panics, you will quickly notice this, and adjust it appropriately. If we fail to guarantee overflow checks, however, we will quickly accrue a de facto dependency on overflow semantics (as we indeed already have).

Point 4. Overflow semantics are not worth taking a performace hit because we still give strong memory safety guarantees.

I don't think the premise of this point is correct. There is no performance hit in an optimized build, and unoptimized builds are already giving up some performance, so where is the problem? As for the general importance of catching overflow, I agree that we still give strong memory safety guarantees even in the face of oveflow (modulo unsafe code), and this is precisely why I think it's acceptable for optimized builds to forego the dynamic checks. Integer overflow is simply a weaker class of bug than a data race or access to freed memory. However, such bugs are still important. As you yourself once wrote (when arguing against my proposal to remove the mut keyword), "Memory safety isn't the only thing that matters". What can I say, you made a good point!

Finally, a question. I also have a question for you. What is the basis for your figure of a performance hit of 20-100%? On of the things I've been looking forward to as part of @aatch's work is the ability for us to get more realistic performance numbers for large programs. Personally, I think even a 20-100% hit is worth it, since it doesn't apply to optimized builds, but I suspect the hit may be smaller. I also suspect that it will improve in time, particularly with Swift's emphasis on checked overflow.

@thestinger

Non-normatively, I am not yet sure if I think that giving imprecise errors is a good idea. To the extent that it permits more widespread checking (particularly, checking in optimized builds), it's obviously good. But then it will make errors harder to debug: so I guess it depends on the performance win that results. I'm happy to just let compilers (and users) set their policy on this point as they choose. As far as I know, this is largely a speculative point anyway, right? Or do architectures expose the kind of features you're talking about today? (If so, cool!)

Architectures essentially expose support for this by having an overflow flag. Instead of branching on the overflow flag, they can be propagated through loops and then checked only when the compiler loses track of the "pure" code. This is still going to have a significant cost - especially if it could have been vectorized - but it's a lot better than a branch on every arithmetic operation.

I also suspect that it will improve in time, particularly with Swift's emphasis on checked overflow.

Swift has a high-level IR above the LLVM layer where it will do these optimizations. This is where the lazy overflow checking would likely be implemented. It can consider the arithmetic operations as returning a result and and overflow flag, and OR these together as much as possible - only performing a branch where the values escape to somewhere that they can be observed. It then maps down to the LLVM overflow intrinsics, which map down to efficient hardware instructions.

It's unrealistic for LLVM to do much optimize here itself, because it has no idea what the language wants. A panic with a unique error is pretty much impossible to optimize - it can do anything as far as LLVM is concerned and passing it stuff like the row and column makes it unique.

Finally, a question. I also have a question for you. What is the basis for your figure of a performance hit of 20-100%? On of the things I've been looking forward to as part of @aatch's work is the ability for us to get more realistic performance numbers for large programs. Personally, I think even a 20-100% hit is worth it, since it doesn't apply to optimized builds, but I suspect the hit may be smaller. I also suspect that it will improve in time, particularly with Swift's emphasis on checked overflow.

It's the typical performance hit from the naive implementation of adding a branch with a unique side effect to every arithmetic operation. The performance hit will get larger in a complex number crunching program because the branches won't be predicted correctly and the bloat will cause a severe impact on instruction caching.

There are various papers looking into optimized (lazy) overflow implementations and the performance hit for various types of programs. I think 3-5% is a fairly realistic number for a very good lazy implementation, simply because programs spend a lot of their time memory bound, etc.

@pcwalton
Contributor
pcwalton commented Jan 9, 2015

It's unrealistic for LLVM to do much optimize here itself, because it has no idea what the language wants. A panic with a unique error is pretty much impossible to optimize - it can do anything as far as LLVM is concerned and passing it stuff like the row and column makes it unique.

You keep bringing this up, but I've never understood this as an objection to doing overflow checking at all. If we're making implementation decisions like passing line number information to LLVM that makes it unable to fuse panics, then that seems like a straightforward bug to fix at the implementation level. LLVM already has function annotations that allow it to express invariants that allow it to do constant propagation and the like, and it even goes beyond that to special-case functions like operator new and whatnot. We already have custom optimization passes for Rust in LLVM. I don't see how making LLVM optimizations work better with panic is out of the question (or even particularly difficult).

@thestinger

In optimized builds, it should be permitted but not required to abort the process when a overflowed value would have been observed. This would permit an efficient implementation taking advantage of hardware and compiler support. Ada deals with integer overflow like this.

@pcwalton
Contributor
pcwalton commented Jan 9, 2015

In optimized builds, it should be permitted but not required to abort the process when a overflowed value would have been observed. This would permit an efficient implementation taking advantage of hardware and compiler support. Ada deals with integer overflow like this.

I don't think anyone would disagree with this—at least, I don't. (Though of course I would prefer to throw an exception instead of aborting.)

@thestinger

I don't think anyone would disagree with this—at least, I don't. (Though of course I would prefer to throw an exception instead of aborting.)

Exceptions means no support for using hardware overflow support on *nix. An implementation could be allowed to throw an exception but it shouldn't be required to do it. It's not safe to throw from a signal handler and Rust aims to avoid a runtime which means it can't just set up arbitrary signal handlers. It's totally not kosher for a library to do that, and an application may want to do that itself for other reasons.

@dgrunwald
Contributor

Rust has no purpose without good performance, and integer overflow checking pushes it behind C# and Java.

You're wrong here: In C#, overflow checking is active by default in debug builds.
Release builds have well-defined wrapping semantics by default.

C# allows overriding the default setting within a function by using checked or unchecked blocks, which change the semantics of integer arithmetic within that block. So that's pretty much the same as the 'scoped attributes' RFC.

@thestinger

You're wrong here: In C#, overflow checking is active by default in debug builds.

I don't think that's true. It has a /checked switch but it's always disabled by default.

I was also talking about release builds, considering that unoptimized Rust is already much slower than C# and Java. I'm not operating under the assumption that this is going to be limited to debug builds forever. By making overflow abort / throw in debug builds, it's now incorrect (but practical) to remove these in optimized builds. At some point, it's going to be turned on by default everywhere at some performance hit because it has been defined as wrong to not enable it. Even a 5% performance hit by using some level of hardware support is going to be deemed acceptable considering that the language has already been designed this to use it. It's not fair to pretend that this isn't the plan.

@dgrunwald
Contributor

I don't think that's true. It has a /checked switch but it's always disabled by default.

Maybe if you're using the command-line compiler. Most C# developers use Visual Studio, and that creates projects with overflow checking enabled by default.

@thestinger

An untouched Visual Studio installation doesn't seem to do that. It's hidden away in Properties -> Build -> Advanced and disabled by default. It remembers if you want it enabled though...

@dgrunwald
Contributor

Huh, you're right. I wonder when this changed, because I think earlier VS versions (around 2003/2005) did this.

@nikomatsakis
Contributor

@thestinger thanks for the clarification. So, if I understood you correctly, you're saying that if we put some effort into optimizing our implementation intelligently (to coalesce checks and so forth), you believe we could get the overhead down to 3-5%. And this overhead itself would be restricted to debug builds (unless user opts in). So I confess I don't entirely see the problem -- certainly our initial impl is going to be naive, but it will improve with time (and you suggested some good strategies for doing so). In the meantime, optimized builds will suffer no performance penalty, and debug builds will help users identify overflow problems. Maybe if the overheads are low enough, we can just turn overflow checking on by default even with optimizations.

@nikomatsakis
Contributor

@thestinger

At some point, it's going to be turned on by default everywhere at some performance hit because it has been defined as wrong to not enable it.

This is not true. It is has been defined as wrong to have programs that overflow without using the wrapping or checked intrinsics. This does not imply it is wrong to disable checking any more than it is wrong to disable debug assertions.

Even a 5% performance hit by using some level of hardware support is going to be deemed acceptable considering that the language has already been designed this to use it. It's not fair to pretend that this isn't the plan.

Of course the plan is to turn on checking by default if we can get the performance hit low enough! You say this like it's a bad thing, but I don't understand why that would be so.

As for your specific number of 5%, I don't have an opinion on whether that's a good threshold. Seems like a fairly large hit to me but I guess I'd want to wait and see what people do -- if people are enabling overflow checks locally, even with a 5% perf hit, that might be a good reason to suspect we should turn them on universally (by default).

@thestinger

you believe we could get the overhead down to 3-5%. And this overhead itself would be restricted to debug builds (unless user opts in).

I expect it would be a lot higher for debug builds. It could get down to a number like that with extensive optimizations where usable error reporting isn't provided. It will always prevent stuff like auto-vectorization though... but that isn't yet that pervasive.

@nikomatsakis
Contributor

I expect it would be a lot higher for debug builds. It could get down to a number like that with extensive optimizations where usable error reporting isn't provided.

Ah, because (you believe) the cost of a panic is prohibitive?

EDIT: This sounds a bit rude, sorry. I just meant that while unwinding may well be very expensive today, this is not obviously a given, and it seems like it can be optimized as well over time.

@thestinger

Ah, because (you believe) the cost of a panic is prohibitive?

Leaving out precise errors makes it possible to coalesce more branches and making the error checking lazy rather than strict wipes out an even greater number but completely loses track of the origin.

@thestinger

(but debug builds won't have any optimization at all, so you're paying for a branch on each one)

@thestinger

Detailed error reporting of the origin is essentially debug information that the optimizer is forced to preserve. Making it fast also means lazy error checking where optimizations are implemented that will cause code that would have aborted to not abort as long as an overflowed value cannot be observed. Giving each arithmetic operation an effect is a problem, especially if each operation has a unique effect.

@thestinger

For example, with a fast overflow checking system this will abort if x + y overflows:

println!("{}", x + y);

However, this does not have to abort, because there is no way to observe the overflow:

let z = x + y;
for i in range(0, z) {}

This has to abort because otherwise it would have an effect (non-termination):

let z = x + y;
while z < x || z < y { ... }
@rkjnsn
Contributor
rkjnsn commented Jan 9, 2015

I very much like the idea of having operations expecting wrapping distinguished from those for which overflow would be an error. I also agree that overflow checking should occur in debug builds, both to catch overflow errors as soon as possible and to prevent code from accidentally relying a certain overflow behavior.

However, I think this default overflow checking should be made as fast as is reasonable, including potentially doing imprecise, lazy checking and aborting instead of panicking. I think it would be sufficient to report that an overflow occurred somewhere in a given region and then terminate execution. There should then be an opt-in way to enable precise checking (either globally or for a given crate, module, or function) if needed to track down the precise cause of the overflow.

I think overflow checking should be off by default for release builds, but with options for enabling it. It would be useful to be able to selectively enable overflow checks (e.g., via scoped attributes) so that one could have them enabled for security-critical code, but disabled elsewhere.

@bill-myers

It doesn't seem obvious without looking in detail at a specific CPU microarchitecture that a branch that is predicted to not be taken or even better an instruction with hardware support needs to be particularly slow or slower than conditionally setting an overflow flag (assuming that the architecture doesn't have an overflow flag that is not reset after each instruction, which e.g. x86-64 doesn't).

In fact, it might be faster because a correctly predicted untaken branch just does nothing as opposed to either consuming a register for the lazy-checked overflow flag or issuing a write to L1 cache if it's stored on the stack.

That said, the RFC should definitely try to allow implementations to be as lazy as possible without compromising correctness.

@nikomatsakis nikomatsakis Clarify that overflow panics are not required to be precise,
and that panics may occur only on some branches.
630dd70
@nikomatsakis
Contributor

Added a new commit clarifying that precision is not required and so forth: 630dd70

@bill-myers

I don't think this makes any sense for right shifts.

Right shifting by N or more should just yield 0 so that it is equivalent to division by 2^N.

Left shifts should trap on overflow just like multiplication by 2^k. If the shift amount is N or greater, then the left shift will always trap unless the value shifted is 0.

If the programmer desires x86-like semantics, they can shift by (s & 31) and the optimizer will hopefully remove the extra checks since it knows the shift amount is less than 32.

Owner
@samlh
samlh commented Jan 10, 2015

I like this proposal, but I don't like the concept of "unspecified behavior" that isn't really unspecified. To avoid confusion, I'd prefer to say that if the overflow is not diagnosed, the value is guaranteed to wrap.

Alternatively, you could write that it truly is unspecified, and optimize based on that, but I think that's non-ideal.

@glaebhoerl
Contributor

I'd prefer to say that if the overflow is not diagnosed, the value is guaranteed to wrap.

I'd be fine if rustc did that as long as it's not advertised, to avoid people relying on it. (In fact I'm not sure if there's any other reasonable plan...)

@glaebhoerl
Contributor

As per this discussion with @petrochenkov, I think we should remove unary negation for unsigned types as part of this. (cc also rust-lang/rust#5477)

@samlh
samlh commented Jan 10, 2015

Rustc, and therefore the language, work one way or the other. If you want to choose the behavior of rustc, do so, but then don't call the behavior unspecified. Alternately, say that the behavior may be one of a few options (to correspond to hardware).

As a user I want there to be a certain level of sanity in compiler behavior. The trapping behavior is definitely good, but I want to make sure that not force-enabling trapping in release builds isn't shooting myself in the foot.

to avoid people relying on it.

I want to rely on the compiler not doing really weird things when an overflow bug slips through testing. If it isn't specified, I'll likely do so regardless, and try to watch the release announcements to know when to change my compiler flags (or switch to shipping debug builds).

@glaebhoerl
Contributor

Rustc, and therefore the language, work one way or the other.

The fact that there is only a single implementation of Rust may not always be one in the future.

I want to rely on the compiler not doing really weird things when an overflow bug slips through testing.

Me too! But I don't want people to turn off overflow checks as a way to get wrapping semantics. So this is a fine line to walk on.

@nikomatsakis
Contributor

On Sat, Jan 10, 2015 at 04:09:38PM -0800, Gábor Lehel wrote:

Me too! But I don't want people to turn off overflow checks as a way to get wrapping semantics. So this is a fine line to walk on.

The fact that people cannot turn the checks off in debug mode should
resolve this problem, however.

@glaebhoerl
Contributor

It definitely helps. Maybe I'm paranoid - but I've worked with people who were only willing to test their software "exactly as it's going to be shipped", and only ever compiled in released mode. (And this was with Qt where that meant all assertions got compiled out!)

@Valloric

Maybe I'm paranoid - but I've worked with people who were only willing to test their software "exactly as it's going to be shipped", and only ever compiled in released mode.

While those people are certainly crazy in my book, there's the other side of that coin: I've seen production issues caused by compiler bugs in optimization passes that would have been trivially caught had the tests been run on an optimized server binary as well, but tests were only run on debug binaries.

So tests should run in both configurations; possibly in opt mode only right before a release as a sanity check.

@petrochenkov
Contributor

@samlh @glaebhoerl
Stable unspecified values (when ∀ x, y x + y gives the same result on every evaluation) would likely prevent all realistic optimizations.
Although I still don't think this word play is more useful than just saying "yes, it wraps".

@petrochenkov
Contributor

A short recap of what the major C++ compilers do with overflows:
Clang and gcc have a set of sanitizers, inserting runtime checks for various kinds of undesired behaviour.
http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation
https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html
In particular, -fsanitize=signed-integer-overflow checks for integer overflows.
Clang even has -fsanitize=unsigned-integer-overflow despite the fact that unsigned overflows are well-defined and the checks can give false positives.
These two are just a small part of the set; some other sanitizers implemented in clang and gcc may be less relevant for Rust, but they still can be useful due to presence of unsafe blocks and Rust should be ready for them.
In general, sanitizers can be controlled in a fine-grained fashion by compiler flags and not tied to something like debug_assert!, furthermore sanitizers can be disabled for particular pieces of code.
VC++ has a rudimentary form of sanitizers represented by the /RTC flags
http://msdn.microsoft.com/en-us/library/8wtf2dfz.aspx
None of these sanitizers in all three compilers is enabled by default even in debug builds.

The analogous approach in Rust would be:

  1. Implement the overflow sanitizer in rustc and require it to be a mandatory part of any Rust implementation
    • The sanitizer can be enabled by a dedicated compiler flag/attribute and isn't directly tied to Debug or Release.
    • The sanitizer is disabled by default.
  2. Promote the sanitizer, advertise it, mention it in the guide and everywhere else
    • Maybe even turn it on by default in Debug builds - a purely socially motivated move
  3. Provide a way to opt out of the sanitizer locally by marking code with attributes or using wrapping_add, Wrapping<T>, Swift-like wrapping operators or whatever
    • The marked operations can be easily found by simple text search
    • The sanitizer can actually find two kinds of errors - legit overflows and unmarked wrapping operations
  4. Future: implement more sanitizers in the same fashion

In general, I'm relatively sceptical about finding overflow bugs in casual testing in Debug builds.
What I think would be useful is emitting checks in Release builds to the code running in production in something like "Profile Guided Check Insertion" build pass (by analogy with Profile Guided Optimization).
The checks would be inserted into cold code and would not affect performance.
In this scenario I would expect the overflow handlers to write something to log and proceed with 2's complement results rather than panic, but for it user defined handlers should be allowed in the first place.

@klutzy klutzy referenced this pull request in rust-lang/rust Jan 13, 2015
Closed

Numeric traits inconsistent with integer type system #21069

@nikomatsakis
Contributor

On Mon, Jan 12, 2015 at 01:46:25PM -0800, Vadim Petrochenkov wrote:

In general, I'm relatively sceptical about finding overflow bugs in
casual testing in Debug builds.

I don't expect overflow checks to find all bugs either. However, I
do expect mandatory overflow checks to ensure that the overflow
semantics are well-known and that code which intentionally uses
overflow breaks fast (and I think the PR shows that this
works
). This means that when I run my sanitizer and see
violations, I can be reasonably sure that they are not false
positives, even in libraries I am using (where people may not
otherwise have been careful to identify wrapping operations).

A secondary benefit is that re-running a problematic test case with
assertions enabled (usually my first step to diagosing a problem...)
will quickly uncover an overflow violation, but I agree that if you
want to be thorough some kind of secondary sanitizer is a good idea.

@mahkoh
Contributor
mahkoh commented Jan 13, 2015

Having the result of overflows unspecified makes the language strictly less useful and means that rust can no longer be considered a language for writing secure programs.

The author starts by saying

The semantics of the basic arithmetic operators and casts on the built-in fixed-size integer types are currently defined to wrap around on overflow. This is is well-defined behavior, so it is already an improvement over C.

But then suggests to make the result unspecified which will invert this improvement. Consider the following code:

fn get_user_credentials(vec: &[Credentials], a: usize, b: usize) -> &Credentials {
    assert!(a > b);
    let idx = vec.len() - (a - b);
    &vec[idx]
}

Now consider the following "equivalent" transformation:

fn get_user_credentials(vec: &[Credentials], a: usize, b: usize) -> &Credentials {
    assert!(a > b);
    let idx = vec.len() - a + b;
    &vec[idx]
}

With the new semantics, this innocent transformation has introduces a major security risk. If an attacker manages to have this function called with a > vec.len(), the function will return an arbitrary set of credentials. Naturally this will never be caught in debug builds because the attacker has to send a specially crafted packet to get a > vec.len().

If the author thinks that this makes Rust safer, then he is clearly mistaken.

@mahkoh
Contributor
mahkoh commented Jan 13, 2015

This RFC is seriously flawed in many ways. Because the text is way too long,
I've paraphrased the quotes below, which is indicated by [ and ].

Motivation

[We are better than C but that's not where we should set the bar.]

The current specification is on par with C in practice. GCC and Clang have flags
to make signed integer overflow wrap around. We will see below that this RFC
sets the bar far below this behavior.

[Overflow is a harmful logic error in most cases. Wrapping around can cause
security bugs.]

This seems to be an axiom this RFC is based on. If integer overflow is as
harmful as the author wants us to believe, then there surely is research which
shows that this is true? The author repeats multiple times that he is not aware
of code that profits from integer overflow except for hash algorithms. This
issue already contains multiple examples where integer overflow is the desired
behavior.

Furthermore, accidental and harmless unsigned integer underflow happens easily.
E.g. a - b + c and a - (b - c). Also let x: u8 = -1.

It hasn't been shown that the new semantics will make code safer. We'll see
below that this RFC introduces serious new security bugs.

[Wrapping around can only ever be a memory safety issue in unsafe code. If the
built-in integer types are defined as wrapping around, it's hard to detect if
wrapping around is the intended behavior during code review.]

Absolutely. Unfortunately this RFC goes way beyond fixing this.

[It should be easy and convenient for the user to decide where exactly he wants
overflow checks.]

[Wrapping around is almost never desired. Therefore it doesn't have to be
convenient.]

Again, it hasn't been shown that this is true. Given the lack of bugs caused by
overflow in current code, maybe the opposite is true.

Goals of this proposal

[Don't break code that currently works if possible.]

It didn't work.

Non-goals of this proposal

[We don't care about the performance in checked code.]

Then the detailed design below must have been written by someone else. It
contains lots of stuff, e.g. postponing error reporting, that only makes sense
if you care about performance.

[We don't care about compatibility with checked arithmetic in future processors.]

Fun.

Acknowledgements and further reading

On the limited use cases for wrapping arithmetic:

  • [Jerry Morrison on June 20][JM20]

Maybe this person could point us to the data confirming the claims in this RFC.

Detailed design

Let's look what the template says:

This is the bulk of the RFC. Explain the design in enough detail for somebody
familiar with the language to understand, and for somebody familiar with the
compiler to implement. This should get into specifics and corner-cases, and
include examples of how the feature is used.

Unfortunately this RFC doesn't do any of this. Given that this RFC is about
changing the specification of the language, one would expect the detailed design
to contain the concrete text that is supposed to be added to or removed from the
spec. Not only doesn't it do this, but the informal explanation below
contradicts itself and makes writing unsafe code impossible.

Semantics of overflow with the built-in types

In the following, the "built-in integer types" are i8, u8, ..., u64,
isize, and usize.

[If overflow is checked for the built-in integer types, panic on overflow.
Otherwise return an unspecified result on overflow.]

[If overflow is checked for the built-in integer types, out of bounds shifts
panic. Otherwise out of bounds shifts return an unspecified result.]

It's unclear what is meant by "overflow is checked." This paragraph seems to
make a clear distinction between this state and the "unchecking mode." If
overflow is checked, panic, otherwise don't panic but return whatever you want.
In contrast to that, the paragraphs below state that in "unchecking mode" the
compiler is free to insert whatever code it wants and to panic if it feels like
it.

[Overflow is considered a logic error.]

What is this doing in the detailed design? This tells us absolutely nothing
about how the compiler is to behave. Even after paraphrasing everything it's
still hard to see what the author wants to tell us.

[The compiler must not change the semantics of code based on the assumption that
no overflow happens.]

This contradicts the statements below.

Notes:

So is this still part of the changes the author wants to make to the spec or
just some informal explanations of the text above? Apparently it's changes to
the spec given that it gives the compiler the freedom to cause memory unsafety
when it encounters an overflow.

[The compiler should wrap around on unchecked overflow of the built-in integer
types.]

Absolutely useless. First you write in the spec that the compiler is free to
return whatever value it wants but now you ask it nicely to not do that. What
kind of spec is this? Given that you don't even require the compiler to document
what it does exactly (i.e. implementation defined behavior), this is not better
than "read the source if you want to see what happens." Who is supposed to use
this information? The user certainly cannot rely on it.

[Unchecked overflow of the built-in integer types produces an unspecified value
but does not cause undefined behavior in the sense of the C standard.]

Wrong. The next paragraph makes UB possible.

[If overflow is checked and detected for the built-in integer types, the
compiler has to panic after a statically determined number of instructions.]

What kind of systems programming language is this that allow the compiler to
introduce function calls (panic) just about everywhere in the code without my
explicit approval? Consider the following code:

fn totally_safe_function(vec: &mut Vec<T>, a: usize, b: usize) -> T {
    assert!(vec.len() > 0);
    not_so_safe_function(vec, a - b)
}

fn not_so_safe_function(vec: &mut Vec<T>, c: usize) -> T {
    unsafe {
        let ptr = vec.as_mut_ptr();
        let len = vec.len();
        let ret = ptr::read(ptr.offset(len - 1));
        ret.val = c;
        /// XXX
        vec.set_len(len - 1);
        ret
    }
}

So, a - b underflows and the compiler inserts a panic at XXX and the
destructor of ret runs twice.

Even if you remove this "panic whenever you feel like after the overflow".
Panicking in unsafe code just because you got an overflow is ridiculously
unsafe.

[If overflow checking is disabled, the compiler is free to insert code which, on
overflow of a built-in integer type, panics after an arbitrary number of
instructions.]

I don't even want to know who came up with this and why they think this is
necessary and what the consequences are.

Clearly there are also cases where the compiler will introduce serious security
bugs in completely safe code:

fn credentials(vec: &[Cred], lo: usize, hi: usize) -> &Cred {
    &vec[lo - hi]
}

becomes in unchecked mode

fn credentials(vec: &[Cred], lo: usize, hi: usize) -> &Cred {
    if lo < hi {
        &vec[0] // root credentials
    } else {
        &vec[lo - hi]
    }
}

because the compiler is allowed to do this as per the spec.

The default

[The compiler has to insert code that checks overflow of the built-in integer
types in debug mode. The compiler is allowed to insert such code at any time.]

And what kind of bug will this catch if it's not enabled in release mode? Would
youtube have caught their view counter overflow bug with this? Na, in fact, with
this RFC, the bug would have been discovered much later. Given that the result
of overflow is undefined in release mode, view_counter + 1 == view_counter is
a valid possibility.

[This is to ensure that overflow of the built-in integer types is detected
during debug mode. Overflow checks will be compiled out in optimizing builds.]

The "detailed design" above clearly states that the compiler is allowed to do
whatever it wants in optimizing builds. Why does it now say that there are no
checks in optimizing builds? Again: What kind of spec is this?

Alternatives and possible future directions

Checks off means wrapping on

Where overflow checks are turned off, instead of defining overflow as returning
an unspecified result, define it to wrap around. This would allow us to do
without the WrappingOps trait and to avoid having unspecified results.

This doesn't follow. You could easily say that, in optimizing builds, the
compiler can freely choose between wrapping and panicking. This way the user
still cannot depend on wrapping and the user still has to use explicitly
wrapping functions if he wants this behavior.


Anyway, this RFC isn't seriously up for discussion. The author has
already stated that this RFC will be accepted.

We (the core team) have been reading these threads and have also done a lot of
internal experimentation, and we believe we've come to a final decision on the
fate of integers in Rust.

@brson
Contributor
brson commented Jan 13, 2015

@mahkoh Your comments are unnecessarily combative.

Everybody please remember to keep your tone civil.

@mahkoh
Contributor
mahkoh commented Jan 14, 2015

Another thing that cannot be expressed properly after this RFC:

if a <= b + c {
    b += c - a;
}

The right side of both b += c - a and b -= a - c can overflow. Therefore you have to write

b += c;
b -= a;

or

b = b + c - a;

Edit: More precisely: In the example above neither a, b, nor c are allowed to overflow. But it's fine if the temporary value on the RHS overflows.

@nikomatsakis
Contributor

@mahkoh I was working on a more detailed response to your points, which I'll post soon, but I did want to point out a quick note on this example that you gave. There is no loss of expressiveness here. You can use wrapped_add and wrapped_sub calls to do all the things you are showing. This also highlights the (somewhat subtle) logic that implies overflow is permissible here. Note also that overflow of intermediates is only harmless in sums; something like (a + b)/2 does not behave as expected, as famously reported here.

@mahkoh
Contributor
mahkoh commented Jan 14, 2015

It's true that one can use the wrapping operations, but it's questionable if this makes the code clearer.

b += c;
b -= a;

b += c.wrapping_sub(a);

Or from the code I was working on when I noticed this:

                self.text.end -= cur_grapheme_len;
                self.text.end += right_len;
                self.term_unused_width += cur_grapheme_width;
                self.term_unused_width -= right_width;

                self.text.end += right_len.wrapping_sub(cur_grapheme_len);
                self.term_unused_width += cur_grapheme_width.wrapping_sub(right_width);

I'd rather use the first version which is much easier to read.

@mahkoh
Contributor
mahkoh commented Jan 14, 2015

Oh, that's wrong too. If the wrapping_sub actually wraps, then the produced value will be so large that the += will also wrap. Therefore you'd have to write

self.text.end = self.text.end.wrapping_add(right_len.wrapping_sub(cur_grapheme_len));

At that point you can just use

self.text.end = self.text.end + right_len - cur_grapheme_len;
@mkaufmann

#1)
First of thanks to Niko for providing the intial thoughts on this interesting discussions! As there are some drawbacks to the approach outlined in the RFC (performance for debugging, additional code complexity for algorithms relying on wrapping semantics and different semantics between programs in debug and release builds!, more on this in section 4) I think an assessment of the actual securtiy benefits of this RFC is very important. The combination of theoretic improvements, actual observable improvements in real code and the drawbacks of this change together should be considered to decide whether the RFC brings a net benefit or not. One additional concern should also be whether this is actually the best solution for that class of bugs. I don't think the whole discussion is yet well represented in the RFC document. Following are my thoughts.

2) What are the theoretic improvments, which types of bugs are removed?

Here I borrow a part from the motivation in the RFC:

In the large majority of cases, wrapping around on overflow is not a useful or appropriate semantics: programs will generally not work correctly with the wrapped-around value. Much of the time, programs are merely optimistic that overflow won't happen, but this frequently turns out to be mistaken. When overflow does happen, unexpected behavior and potentially even security bugs can result.

It should be emphasized that wrapping around on overflow is not a memory safety issue in the safe subset of Rust, which greatly mitigates, but does not eliminate, the harm that overflow bugs can cause. However, in unsafe blocks and when interfacing with existing C libraries, it can be a memory safety issue; and furthermore, not all security bugs are memory safety bugs. 

My understanding of this is that there are two distinct error classes covered by this proposal:

  • 2a) Fix potential memory safety issues in unsafe code and when interfacing C libraries
  • 2b) Fix logical bugs through overflow which could result in unintended behaviour with potential security implications

3) What are the actual observable improvements through the proposed change?

The theoretic improvements are discussed in the motivation, but how do these affect real code? Issue #20795 which converts rust to use these new wrapping semantics can provide some insights. I analyzed the change set and could not find any bug that was fixed in the conversion. Given that this is a very large code base this seems to be a disappointing result, especially in comparison to the very large number of other bugs in the code base (not due to quality but due to the sheer size). This seems to indicate to me that the class of errors adressed by this RFC is at least by numbers very insignificant compared to the total number of bugs in the code base. Please correct me if this is an misinterpretation.

Another way to double check the effect of the proposed changes would be to look at (closed) bug reports and check which were caused by overflows, there were not detected anyways (e.g. through bound checks). I did not do this. Instead of looking at the past bugs one could look at all incoming bugs for a timeframe and check whether they would be solved by this change. This would give a ratio of fixed bugs divided by total bugs which could also hint at the importance of this change. In the last week 235 bugs were created. So by just observing one week one could determine if the impact would be in the percent range. Again the ratio is only one number, but once the issues are identified where overflows lead to problems, we could also look at the badness of the error.

4) What are the drawbacks of the RFC?

This is in part a subjective matter. The safest argument would be that certainly all code with checks enabled would be slower (the proposal limits this behaviour to debug builds). The argument was made that as the branches for overflow checking should be fully predictable they should be negligible. This will probably not be true. Branch predictors have a very limited number of slots, these can be easily oversaturated when there are a lot of branches, resulting in the loss of information for some branches and thus the ability to predict them.

Other than the performance problems the changes also result in added complexity when writing code which depends on wrapping semantics. This will affect the learning curve. The severity of this problem should be checked by looking at code that depends on wrapping semantics. In Issue #20795 mostly only hashing code and cryptographic code was affected . Additionally the following idiom had to be written in the more verbose way with wrapping add:

(id - self.from_id_range.min + self.to_id_range.min)

So in total three changes. In my own code I also use overflows to e.g. detect the available elements in a circular buffer without branches (for a circula buffer head can be larger or smaller than tail):

((head - tail) & size_mask)

this would also require conversion to the wrapping syntax or use of branching code (slower).

One further (arguably more subjective) point is that the change as proposed creates a difference in behaviour for the same code between debug and release. A line of code that is executed both in debug and release mode results in differing behaviour (this is different from a debug_assert or similar mechanisms where the code is either executed or not). The severity of this point should be measured based on whether most overflow bugs will occur in release or debug builds! IF most of the overflow bugs are only triggered in release builds where the code is often subject to a wider range of input than during testing/debuggin scenarios, the benefit of the proposed RFC would be either greatly reduced or the performance implications of this RFC would be much more severe as the checks would also need to be activated in release builds for a positive impact.

#5) Is the proposed solution complete?
The motivation lists to my understanding two types of error classes that are addressed by the proposed changes. Are the changes sufficient to detect these or will it only detect a subclass? Would there be a solution which also could cover a wider class of errors?

After the proposed changes there will still be plenty of arithmetical operations that will lead to bugs in unsafe blocks and interactions with C code. Overflow and underflow often result in conditions where the number is extremely out of bounds of the desired number range (e.g. <= size, leading to memory safety issues) or due wrap arounds point to a wrong field (leading in logical errors in the program). Same for the second class of errors, even by avoiding over and underflows there will be still many situations where a number can go out of the intended range by the developer (e.g. in a game u64 would allow plenty of room to create overly high money/attack/... values through bugs). In my opinion overflow/underflow checking is only halfway to a proper solution for these types of errors. My feeling is that often the desired value range does not correspond to the value range that is desired by a developer. A system which could would allow to encode and enforce information in a numeric type about the desired range with overflow checking would potentially allow to cover a much larger class of errors. If the range is statically known, the underlying numeric datatype could be adjusted from u8 up to bigint and the corresponding checks could be inserted if necessary. If this type would be ergonimic to use I think many developers would naturally use it in their applications. In this case overflow/underflow semantics might not even be necessary on the primitive numeric types as developers would freely choose the advanced numeric types with which they could express their intend better. This is just a dream of mine and I did not think about all the implications about this approach, but I think it might be worthwile. Currently in the RFC there is only discussion about why overflow/underflow might be better as the current state, but there is no discussion why only the smaller class of overflow/underflow bugs should be detected instead of numeric ranges which can be specified by users of the language. If overflow checks are inserted anyway maybe going the whole way is not that much more expensive.

Final thoughts

My two biggest wishes before proceeding with this RFC would be to evaluate the impact on real code bases by monitoring bugs over a sufficient timeframe and checking if they could be fixed by this change (as described in Section 3), evaluating if it is really sufficient to only activate overflow checks in debug as my feeling is that mostly these overflow bugs would be triggered in real usage scenarios where release code would be run. Also I would really see more discussion on the thoughts in section 5 from someone with a deeper background in language theory.

@nikomatsakis
Contributor

@mahkoh thanks for the feedback. I appreciate some of your critiques of general flow of the RFC, and I'll adjust it. The RFC has undergone some editing over time so perhaps the emphasis in some parts disagrees with others, but I think the proposed rules are fairly clear in their intent (indeed, you seem to have divined it fairly well, based on your comments).

If I can summarize your points:

  1. Having the result of overflow be undefined (if no panic occurs) makes it hard to reason about your programs.
  2. Unsafe code needs to know precisely what will happen and when.
  3. Some cases of overflow are harmless.
  4. This result is pre-determined anyway.

Let me address each in turn.

Point 1. Having the result of overflow be undefined (if no panic occurs) makes it hard to reason about your programs.

I am sympathetic to this argument. I think at this point I'm inclined to define the result of overflow as either wrapping or triggering an error. This should help people, I imagine, to reason about the behavior of their code in the face of accidental overflow. The fear that people will disable overflow checks in order to get wrapping semantics is mitigated by the fact that (1) the RFC proposes no means to control overflow checks within one part of the code and (2) the RFC requires that overflow checks be performed in debug builds. Even if we introduce scoped overflow controls in the future, I would personally be opposed to adding any means to turn overflow checks unilaterally off.

Point 2. Unsafe code needs to know precisely what will happen and when.

In general, unsafe code will have to reason carefully about exception safety and all manner of unusual situations. I don't see why this case is terribly different. To write robust unsafe code, I would say it is recommended practice to use wrapping or checked operations, and not the default operators. In addition, there is a very real danger that unsafe code which has unintentional overflow will result in out-of-bounds array accesses or invalid pointer arithmetic. This danger is mitigated by this RFC (though not completely resolved).

Point 3. Some cases of overflow are harmless.

As I wrote here, it is true that some cases are harmless, but others are not. I personally think that calling out the harmless cases is not a bad thing -- and it is often possible to restructure additions and subtractions so as to avoid overflow. Your responses (1, 2) to that case seem to suggest that using the methods is overly painful. Clearly, this is a matter of taste; I prefer to see any kind of non-obvious restructing that is there to ensure overflow is not a problem called out. I also think that the use of Wrapped might help significantly.

Point 4. This result is pre-determined anyway.

It's true that we did announce an intention to accept this RFC. However, we also announced an intention not to change the name of int and uint, and look at how that turned out! I would say that this RFC is under true discussion, though I feel that the majority of comments I have read have been in favor.

@nikomatsakis
Contributor

An open question:

Do you think it would be a good idea to have Wrapping overload the operators to permit "uplifting" normal integers to wrapped ones?

I envision an impl like this:

impl Add<Wrapped<i32>> for i32 {
    type Output = Wrapped<i32>;
    fn add(self, other: i32) {
        Wrapped(self.0.wrapping_add(other))
    }
}

The plus side is that one can write x = x + 1 and so forth when x is wrapped. The downside is that there may be surprises like:

x = x + (y + z)

where x is wrapped but y and z are not. On the other hand, all this means is that a panic might ensure if y + z overflows -- and presuming we did specify that adds wrap if no panic occurs, then the result would be as expected otherwise.

@nikomatsakis
Contributor

@mkaufmann thanks for the thoughtful comment. I'll have to read it over again to make sure I've extracted everything. My first reaction is that the goal of the RFC, however, is not to catch all bugs, or even close to that, but to make clear that overflow is not something that we think is commonly expected. I'm not sure that the PR proves much one way or the other: I do not doubt that there are many potential overflow bugs that are not currently exploited by the tests. However, I do think that it is plausible to run more thorough sanitizers and analysis against the codebase after the PR has landed, since intentional use of overflow has been clearly highlighted.

To put it another way, the goal of the assertions introduced by this RFC is not primarily to catch accidental overflows, but rather to catch intentional overflow and ensures it is appropriately declared. Now, I would be very happy if, in the future, we could enable checking all the time, but that is not currently believed to be feasible. Nonetheless, if we do nothing, it will never be possible.

I guess the key question then is whether you believe the majority of math operations out there will cause strange behavior if an overflow occurs. If you believe the answer is yes, then it makes sense (I think) to adopt this RFC, as it leaves us a path to enable stricter checking in the future. If we decline the RFC, then there doesn't seem to be a plausible path to me.

UPDATE: I decided I was underselling the RFC a bit here. See also this rephrasing below.

@rkjnsn
Contributor
rkjnsn commented Jan 14, 2015

@mkaufmann

I analyzed the change set and could not find any bug that was fixed in the conversion.

For me, one of the main benefits of this is to quickly find overflow bugs during development. It may well be that testing eventually uncovers them, anyway, but it can take a lot more time to track down and correct random weird behavior than it would to fix an explicit overflow error.

Thus, I am less worried about how many bugs were immediately caught in an already published piece of software that fairly widely used and tested, and more about the ability to quickly identify and fix overflow bugs during development, and to be able to quickly diagnose an overflow bug if a problematic test case is later identified.

In addition, there are other advantages to enforcing that uses of wrapping be explicit, such as the ability to run certain analyses and sanitizers without false positives, and the ability to enable overflow checking in production (for situations where correctness is more important than speed or availability) without having to worry about whether any of your libraries implicitly depend on certain overflow behavior.

@glaebhoerl
Contributor

Some small points:

(Just in case I'd given that impression) I'm not necessarily opposed to defining overflow more strictly as "either wraparound or report an error". I very much sympathize with the motivation of having a more precise specification for greater predictability and to definitively rule out certain compiler shenanigans. There are tradeoffs, and it's a judgment call. I only have concerns. One is "what if people start disabling checks when they want wrapping semantics". I think @nikomatsakis's case here is fairly convincing, provided that the mentioned assumptions are actually upheld in practice (debug builds always check for overflow, and we don't add a way to locally and unconditionally disable checks). The other is: would this rule out the option of moving to more sophisticated designs like Ada's and AIR (see links in RFC) in the future? A definite answer to this question should exist, but I don't personally have the expertise to determine what it is.

I also have to agree that the combination of the compiler having the liberty to report overflow errors imprecisely, and checks being present or not depending on the build, and checks being reported via panics (unwinding), seems like it would be completely untenable. Our goal is to improve robustness, but if in pursuit of that goal we turn out to initiate unwinding at unpredictable points inside of unsafe blocks, then it would seem that maybe we have gone somewhat astray. Maybe overflow panics plus unsafe blocks is already untenable, even without the added unpredictability. (I've come to lean towards the position that having unwinding at all is not worth all of the complications that arise from it, but it's probably too late for that now.) So, anyways: what if we specified that overflow is always reported by terminating the program outright (abort rather than panic)? Who thinks that that would be a horrible idea? (Perhaps we could have report-using-panic! be opt-in, say on a per-crate level. Function calls must already be assumed to potentially panic, so this is unlikely to cause problems cross-crate.)

Finally I just want to emphasize, once more with feeling, that the biggest impact of the RFC may not actually be for overflow, but for underflow of unsigned integers. I think this alone is sufficient to motivate it: with the RFC, it makes sense to follow "common sense intuitions" and use unsigned types for variables where negative values don't make sense. The compiler will insert assertions to guard against going below zero in all of the relevant places, automatically (at least in debug builds). Without that, I think the recommendation would have to be something like only ever use unsigned integers if you really, really need the extra positive range. (Google's C++ conventions say "only if you need the defined-wrapping overflow semantics", but that's not relevant for us.) For unsigned types with wrapping semantics (i.e. the current ones), underflow is silent and undetectable. With signed types, at least you can insert assert!(n >= 0) in some places. (I think this deserves a mention in the RFC, it just hadn't yet occurred to me when I wrote it.)

Do you think it would be a good idea to have Wrapping overload the operators to permit "uplifting" normal integers to wrapped ones?

I'm not sure. I thought of this, and due to not knowing whether or not it would be a good idea, left those impls out as the conservative and forwards-compatible choice. This feels like it might also be closely related to the implicit widening, promotion, etc. discussion (discourse thread) - your example of where this might go wrong is structurally very similar to the examples people have cited against implicit widening.

@mahkoh
Contributor
mahkoh commented Jan 16, 2015

ith the RFC, it makes sense to follow "common sense intuitions" and use unsigned types for variables where negative values don't make sense.

(a + b) - c != a + (b - c)

This RFC goes against common sense because it removes additive associativity from unsigned integers.

@mahkoh
Contributor
mahkoh commented Jan 16, 2015

"what if people start disabling checks when they want wrapping semantics"

Yes, what if? I'd really like to see this question answered.

Function calls must already be assumed to potentially panic, so this is unlikely to cause problems cross-crate.

No they don't. Lots of unsafe functions depend on the documentation precisely stating when the functions they call can panic.

@thestinger

I don't think it makes sense for debug assertions to throw exceptions, even in a language primarily using exceptions for error handling. It means programs have different behaviour based on whether it's a release build or not...

@mahkoh
Contributor
mahkoh commented Jan 16, 2015

If integer overflow is considered a bug then a / b should also be considered a bug if a != 0 (mod b). Unlike overflow, this has actually caused crashes in Chromium's webm decoder. (IIRC)

@Diggsey
Contributor
Diggsey commented Jan 17, 2015

@mahkoh In the vast majority of cases, programmers use "a/b" (where a and b are integers) to mean integer division, wanting exact division for which a remainder is an error, is the exception, so at best you're arguing for a helper method on Int to do that (and maybe that's a good idea).

@mahkoh
Contributor
mahkoh commented Jan 17, 2015

If the programmer really wants to lose the precision, then he can use a function that shows his intent. For example:

let c = b.lossy_div(a);
let c = LossyDiv(b) / LossyDiv(a);

depending on if you want to use a helper method or a helper type. As you said yourself, accidentally or deliberately losing precision via division is much more common than integer overflow. Without dedicated syntax, static analyzers and code reviewers cannot know which one it is.

Obviously lossy division has to be significantly more inconvenient to write because we want the right "default" to be used most of the time. The right default is where, unless the programmer has made his intent clear, loss of precision is an error.

I've already given you an example where not checking a % b == 0 can and has lead to catastrophic errors in image/video decoding.

@huonw
Member
huonw commented Jan 17, 2015

I've already given you an example where not checking a % b == 0 can and has lead to catastrophic errors in image/video decoding.

Which example? You mentioned Chromium's webm but no concrete example and I cannot find anything via google.

@huonw
Member
huonw commented Jan 17, 2015

As you said yourself, accidentally or deliberately losing precision via division is much more common than integer overflow

Also, @Diggsey didn't say actually anything about the relative frequency of loosing precision via division vs. integer overflow, just the relative frequency of "division operations that are OK to lose precision" and "division operations that need to be exact".

@mkaufmann

The right default is where, unless the programmer has made his intent clear, loss of precision is an error.

@mahkoh I disagree with this statement. Loss of precision in integer division is a well known behaviour in most languages. I just tested "5/4" in C, C++, Java, C#, Python, Ruby, Javascript, Go. Of all these languages only javascript returned 1.25, all the rest returned 1. Your proposal of treating loss of the remainder as an error is as far as I know unprecedented.

One assumption of the this RFC is that most integer arithmetic does not expect wrapping behaviour to behave correctly. This assumption was proven when doing a test implementation of this RFC in rust-lang/rust#20795 , only very few changes were necessary to adapt the codebase to the new behaviour. Thus it seemed reasonable to adapt the default to better reflect the intend of the developer that most of the arithmetic does not overflow.

If remainder after division would be treated as error I would suspect that much much more existing code would actually break. So supporters of the change would need to prove that it makes sense as a default. Currently I have the feeling that due to the different characteristics remainder after division should be discussed in a seperate RFC. I don't think it makes sense to adopt it as default behaviour. On function or block it certainly would have some value for auditing.

@nagisa
Contributor
nagisa commented Jan 17, 2015

Python … only javascript returned 1.25, all the rest returned 1.

$ python -c "print(5/4)" # your python interpreter is old
1.25
@mkaufmann

Oh sorry you are right. python 2.7 returns 1 and python3.X returns 1.25. Still I think my argument holds, that many developers will know that integer arithmetic will lose the remainder and probably expect this behaviour from a statically typed language even when they come from a dynamically typed language with automatic type conversion like python or javascript.

@mahkoh
Contributor
mahkoh commented Jan 17, 2015

@mkaufmann: I believe those languages don't check integer overflow either. Like the RFC says: Rust should not fall into the trap of low expectations. Instead it should pioneer checked division as an additional safety feature. I believe Pascal distinguishes between integer and floating point division.

If remainder after division would be treated as error I would suspect that much much more existing code would actually break.

Of course it breaks code. Rust breaks code all the time since it's not stable yet. Pre-1.0 changes should be free to break code for the purpose of creating a better language.

One should remember that division is hardly ever used when compared to addition, subtraction, or multiplication:

~/rust/rust/src$ grep ' [-+*] ' $(find -name "*.rs") | wc -l
4590
~/rust/rust/src$ grep ' / ' $(find -name "*.rs") | wc -l
417
~/rust/rust/src$ wc -l $(find -name "*.rs") | tail -1
  480447 total

Checking 400 lines in a 500k lines codebase seems more than reasonable.

@vojtechkral

I have a question: Has it been established how much of a problem integer overflows actually are?

I can't find this in this in the motivation paragraph where I'd expect a mention... Are there maybe some serious CVEs featuring integer overflow? Are there some common use cases where integer overflow is a major concern?

EDIT: Apparently I missed nikomatsakis' post about testing overflows in SpiderMonkey, sorry!

One such case I can think of just of the top of my head is parsing integers from string, since a string of numerals can be arbitrarily long. However, while parsing these strings, overflow checking probably has to be done always, ie. in optimized builds too, so I'm not sure the RFC applies to that case.

Trap of low expectations is one thing, but trying to cover every corner case just to be on the safe side is another (cf. Rootless Root) ...

@mkaufmann

@mahkoh maybe that statement of mine was distracting from the main message. My argument was error on remainder after integer division does not make sense as a default because loss of remainder is actually a desired, well understood, inherent propery of integer division. You probably disagree. Both our opinions are based on intution, but I think the burden of proof is on the one who proposes the change (as said in my previous comment):

So supporters of the change would need to prove that it makes sense as a default.

I am very much interested in your report of the analysis of these 400 lines of rust code using integer division. That will certainly provide a good base for future discussions.

@mahkoh
Contributor
mahkoh commented Jan 17, 2015

After removing comments and floating point division, there are at most 255 cases of integer division in the rust repo. Looking at the first 24 of them I see

  • 6 cases where lossy division is clearly the desired behavior
  • 3 cases where is is unclear if the code is correct without knowing the algorithm
  • 1 case where a hack is used to get (a/b).ceil(), making it harder to follow the code.
  • 5 cases where the programmer should have used a function that explicitly returns / and % because he uses both
  • 8 cases where the checked division is the intended behavior
  • 1 case where the current implementation seems broken because it might expect the remainder to be 0

https://gist.github.com/mahkoh/69a6a7fbbc7b0f1c9160

I only spent 15 minutes on this so please check the results.

@glaebhoerl
Contributor

This RFC goes against common sense because it removes additive associativity from unsigned integers.

Yes, I agree that this is a problem. I'm hopeful that we could restore this guarantee later on once we can figure out a good way to specify it. (But maybe it's not actually that hard and I'm just being overly cautious? @nikomatsakis what do you think?)

@nielsle
nielsle commented Jan 18, 2015

I like the defaults in this RFC. As I see it they are mainly aimed at new users. On the other hand experienced users should eventually be allowed to configure as much as possible.

Perhaps cargo benchmark --all-configurations could cause cargo to benchmark a series configurations, and generate a nice report. This report could include the cost of: overflow checks, number of threads, choice of libraries, linking options, inlining behavior, various buffer sizes, and perhaps even the cost of indexing overflow checks.

EDIT: I have been bitten by lossy division. It would be nice to have a lint against it.

@nikomatsakis
Contributor

I just want to apologize for being absent from this thread. I came down with a nasty fever last week and haven't really been in a state to read comments or compose articulate responses. I see that there has been a lot of discussion; I hope to be feeling better next week and to read and digest it.

@petrochenkov
Contributor

One more possible approach - overflow handlers:

All the arithmetic operations return wrapped around result, but if overflow happens they execute an overflow handler before returning.
An overflow handler can:

  • panic
  • abort
  • write to log
  • do nothing (everyone's favorite variant)
  • anything else

Overflow handlers are installed at compile time at any scope level.
The defaults can be chosen to match the RFC.
Any downsides? (Except for downsides inherent to all checking approaches.)

@nikomatsakis
Contributor

@glaebhoerl regarding additive associativity, I agree it would be nice to retain that, but I think it's kind of subtle and any rule that attempted to do so could well wind up more confusing. (Basically, the success then of your expression would be a function not only of what it does operationally but also the sensitivity and intelligence of the analysis.)

@nikomatsakis
Contributor

@mahkoh I actually find your case about "integer division" interesting, though I think you're making it as a kind of "trojan horse" proposal. It is true that it's one more case where integers (and computers) defy math "as taught in school" -- though even languages like Python still offer floating point, which can equally egregious (none of the identities we would like hold there either).

One place where integer division differs from overflow is that it occurs in such everyday, non-extreme cases, like 3/2. I think this is part of what @mkaufmann was getting at --- rounding down during division is something you experience right away when dividing integers, so hopefully it quickly becomes something you expect.

I don't recall any bugs that I have personally encountered by integral division but I do know that others have complained to me about programs in C where they call a function foo(float) like so foo(3/2) and 3/2 evaluated to 1. But that seems to be more the "implicit widening" problem. Certainly those people were satisfied with the notion that Rust had a stricter separation of floating point and integers, so that expectations were set appropriately. But this is not to say by any means that no bugs will/do occur.

@Aatch
Contributor
Aatch commented Jan 21, 2015

I'd like to mention that in implementing part of this RFC I discovered that the most common case was actually underfow and not overflow. Which makes more sense, when working with unsigned integers, it's far easier to forget a check for 0 when doing a -1 than it is to overflow a usize when used as an index, for example.

@quantheory
Contributor

I think that these pieces seem like no-brainers:

  • We should never ever ever have C-style undefined behavior for the compiler to optimize on, only undefined results. It's philosophically just about the opposite of what Rust is even for.
  • It should be possible to use wrapped arithmetic where useful.
  • It should be possible to decide that overflow and underflow are errors, and have the compiler insert checks for integer arithmetic.
  • Optimized builds should not contain such checks (except maybe if we provide some non-recommended #![slow_down_even_my_optimized_builds_I_hate_overflow_that_much] attribute).

These seem to me to be not entirely obvious, but probably for the best:

  • Wrapped arithmetic is opt-in, via special traits, while undefined results are the default.
  • Overflow checks and user asserts should usually be turned on by one flag (but should probably be independently switchable as well?).
  • The default (just running rustc) should probably have these debug options off by default. The potential cost for some cases is just so high that I suspect that it could make some applications unusable. Also, the cynical side of me says that if you really never test with debug assertions enabled, you shouldn't really expect to be free of certain kinds of bugs.

I agree that integer division is a separate issue (except that division by zero counts as overflow). Most applications I've seen that heavily use integer arithmetic either expect to get a truncated value, or want a "divmod" function, so those two cases seem like they should be ergonomic. (For that reason, I really, really think that a divmod method should be added to the Int trait, or an bare function should be added to std::num, since this seems to really be the most common kind of division needed for e.g. converting scalar IDs into indices into multidimensional or distributed data. But that's even more off-topic.)

@nikomatsakis
Contributor

@mkaufmann I've been pondering your long and detailed comments. I've decided not to try and address the entirety of those comments in one go but rather a point at a time. I would probably state my goals as (in no particular order):

  1. To give users the ability to identify accidental overflow.
  2. To ensure intentional use of wrapping semantics is distinguished from accidental use.
  3. To enable a path for stricter checking in the future without breaking the language.

I agree that there is a risk that point 3 may not be tenable. If there is a large enough body of code, de facto dependencies grow up on the strangest of details, and it can be a hard call whether it's worth going forward with a change that is technically permissible (it only breaks programs that are buggy "by definition") but which in fact will convert working code into not working code. This applies equally to bug fixes and language changes. Nonetheless, other projects have gone forward with changes of similar or greater magnitude successfully (one important feature of such transitions, I think, is making the proposed change in a slow, deliberate fashion so as to give people time to notice and respond to the change in behavior). Still, as you say, it may be that even if we can make overflow checking cheap, we are not able to put that into production as a universal default, and people have to opt in.

Which brings us to the question of scoped defaults. I certainly have nothing against the idea of enabling overflow checking in a scoped fashion. What concerns me most is what happens when no attributes are in scope. Basically, for all intents and purposes, I think the default cause is the important case -- it is what sets the baseline behavior that all packages will adhere to. As you point out, de facto dependencies will occur. Therefore, I thought that the goal of identifying intentional use of wrapping would not be met. At the same time, defaulting to checks being enabled all the time felt like it would be too high a price to pay and would lead to people just turning the checks off in practice.

The idea of using a scoped attribute to disable checking in a region I have mixed feelings about. On the one hand, it's a convenient form of declaration. On the other hand, it is not clear to me that whether an operation should wrap or not is something that typically is correlated with a module of code so much as with a set of values. The current RFC gives you both options, in the form of methods on normal types and newtypes, but admittedly the ergonomics leave something to be desired.

I am, frankly, not so concerned about the ergonomics. Not because I don't think it's imporant, but because I think we can iterate on that fairly easily. I don't think it even has to happen in the standard library. People can build libraries of their own. There's no magic here. I'd rather not do a bunch of work ahead of time; I'd rather do something minimal and then adopt what others do.

@pornel
pornel commented Jan 22, 2015

Have you considered adding overflow check only to explicit casts between types? (at least as MVP)

I'm most concerned about x as usize cast, since that's the place where AFAIK size isn't guaranteed and will change between platforms. I can ensure I use large-enough types in parts of my program that I control, but I have no choice about ubiquitous usize APIs and I feel like I'm casting to usize all the time. I think if that was the only place where Rust ever checks for overflow it'd already be an improvement.

@mkaufmann

@nikomatsakis can you please restate the underlying motivation. I previously argued that the motivation as currently written in the RFC is not really coherent. Without that it is really hard to judge why exactly those goals are desirable. This is specially important with respect to goal 3 and also my next point.

Additionally I am also missing arguments why overflow checking should ever be desirable as a default in release mode. Because of my arguments against this in my previous comments I think it is a must that this RFC specifies that overflow checking with panic will be disabled per default in release mode. Thus I would advocate that integer arithmetic is defined as wrapping in release mode, except when overriden by the user or the user uses a .checked_add method (though I would also be open to other solutions which also do not panic on overflow)

@nikomatsakis nikomatsakis Substantial rework of the text:
1. Clarify the aims of the RFC and generally streamline the text.
2. In the event that no panic occurs, define the results of overflow.
3. Be less generous about the compiler's ability to defer panics.
4. Move the wrapping operations out of the prelude and into `std::num`.
5. Elaborate on the objections raised to the previous draft
   and respond to them.
6dbbc70
@nikomatsakis
Contributor

I just pushed a large rework of the text. The high-level ideas remain the same but I changed a number of the smaller details in response to feedback. I also tried to cover the objections raised and list my take on them. From the commit message, a summary of the changes:

  1. Clarify the aims of the RFC and generally streamline the text.
  2. In the event that no panic occurs, define the results of overflow.
  3. Be less generous about the compiler's ability to defer panics.
  4. Move the wrapping operations out of the prelude and into std::num.
  5. Elaborate on the objections raised to the previous draft
    and respond to them.
@mahkoh

Hilarious. Instead of saying that the value must be representable in the new type you chose a description that is machine dependent and implies that -1i64 as i8 panics.

Hilarious.

😞 This has been a hard discussion to read, and the snark doesn't help.

implies that -1i64 as i8 panics

I don't think that that was the intention. My reading is that this was written with only unsigned integers in mind, and discussion of casts to/from signed integers was omitted by accident. (Note for instance that there's no explanation of what happens for casting between isize and usize, or from i32 to u64, either.)

I do agree that it would make more sense to just say that the value should be representable in the new type. I still don't really understand the reasoning behind keeping wrapping semantics if the overflow checks are turned off. The compiler should be free to produce such values if it's convenient, but why make a promise for users to rely on if we're asserting that overflow is a program error?

It is also inprecise in that "/"can't underflow as far as I know and "%" can't overflow?

@mahkoh
Contributor
mahkoh commented Jan 29, 2015

The new version is just a rehash of the old one and provides no new arguments. So I'm just going to highlight the deceiving style it's written in:

Numeric overflow prevents a difficult situation. On the one hand,
overflow (and [underflow]) is known to be a common source of error in
other languages.
[...]
[underflow]: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types

Let me reproduce your source here:

On Unsigned Integers

Some people, including some textbook authors, recommend using unsigned types to represent numbers that are never negative. This is intended as a form of self-documentation. However, in C, the advantages of such documentation are outweighed by the real bugs it can introduce. Consider:

for (unsigned int i = foo.Length()-1; i >= 0; --i) ...
This code will never terminate! Sometimes gcc will notice this bug and warn you, but often it will not. Equally bad bugs can occur when comparing signed and unsigned variables. Basically, C's type-promotion scheme causes unsigned types to behave differently than one might expect.

So, document that a variable is non-negative using assertions. Don't use an unsigned type.

Good example, except that Rust doesn't have for loops and uses iterators instead. In this particular example one would most likely use an iterator and the problem doesn't even arise. So why did you choose this example? Oh, right, the url has google in it.

Following Google's infinite wisdom, Rust should add implicit coercions between integer types so that we can use signed types everywhere. That's not even a joke. If Rust only had signed types, then the a + b - c problem wouldn't arise.

The RFC has several major goals:

  1. Help users to [...] and help those who wish [...]
  2. Ensure that users who wish [...] can safely [...]

Who would disagree with this kind of language? Let's reformulate a bit:

help those who agree with my opinions on integer overflow
punish those who don't

Because that's how it is if you don't allow users who wish to disable overflow checking to do so.

Ensure that users who wish to detect overflow can safely enable
overflow checks and dynamic analysis, both on their code and on
libraries they use

So there will never be a way for crate owners to disable overflow checking in their libraries. Thanks for letting us know.

Further down you just start repeating your old claims that these overflow checks are important and that they outweigh the negative consequences. But you still don't tell us what measures you've used to come to this result even though you've been asked in this issue to cite your sources.

Nobody should forget that not a single bug has been found thanks to these checks.

@mkaufmann

like signed underflow leading to an infinite loop.

You probably meant unsigned underflow?

@mkaufmann

So "optional" is just another word for undefined behaviour? I don't see a valid reason to allow undefined behaviour!

... may result in a panic, but the ... operation may also simply produce a value

This is very imprecise. At minimum it should be defined as it either panics or produces a value following wrapping semantics.

@mkaufmann

These are implemented to wrap around on overflow unconditionally.

To be precise "unconditional overflow" assumes that signed integer division is well defined.

@mkaufmann

However, wraparound arithmetic is almost never appropriate.

Wraparound is appropriate when a panic from overflow would result in worse behaviour. Thus I would probably formulate it rather as less controversial " wraparound arithmetic is alsomost never intended behaviour" and save the discussion about appropriateness for a different place.

@mkaufmann

since optimized code will not include overflow checking

This is different from what you said before that overflow checking is "optional". Better write "might not include"

@mkaufmann

Furthermore, the danger of an unexpected panic occurring in unsafe code must be weighed against the danger of a (silent) overflow, which can also lead to unsafety.

I agree it must be weighted. I think the proof is on you that overflow in unsafe code leads to more unsafety than unexpected panic. For me that sentence is currently an argument against enabling integer overflows in unsafe code!

@mkaufmann

This part was not changed in this commit, still I want to comment on it:

Should Rust consider these to be instances of overflow, or should it guarantee that they return the correct mathematical result, at the cost of a runtime branch? Division is already slow, so a branch here may be an affordable cost.

The LLVM sdiv operation results undefined behaviour with respect to those operations. To get any defined behaviour you need a branch, even if you just want to guarantee an overflow. On ARM the division by -1 won't trigger any overflow, so if you define it as overflowing you have to insert branches in ARM code. Given this the only sane behaviour is to define it as returning the correct mathematical result if it fits into the data type and otherwise use wrapping semantics. If integer overflow checking is enabled, of course the corresponding overflow should be triggered. Also this only affects signed integer division which is even less important than unsigned integer division.

This is also how it is handled in go and java

@mkaufmann

I went through the changes and it is definitely an improvement. There are some nitpicks with respect to wording, but I think those are probeably not controversial.

Big points I see missing are:

  • Explicit checked operations should be defined I just noticed this is defined on the Int trait. It still might be worthwile to export this as a trait on its own analougus to the Wrapped trait to be consistent.
  • I still don't see a good reason why to allow undefined behaviour when overflow checking is "optional"
  • The RFC still assumes that overflow checking in production builds brings a benefit. As previously argued by me it actually leads to more unsafety. Overflow checks are only helpful during development.
  • Overflow checking in unsafe code makes me still unhappy. I think the best solution would be to force the user to explicitely state what is desired. So if a datatype supports both checking and wrapping arithmetic she can't use the normal "+","-","/",... operators but has to explicitely write checked_add or wrapping_add. This would probably require some changes in the compiler but I think semantically this is a clear winner.
@nikomatsakis
Contributor

@mahkoh I have to be honest, I am not sure why you are using such a hostile tone. I think it's natural for the RFC to advocate for itself and present itself in the best light possible. I realize we don't seem to agree on the best course of action here, but this style of argumentation is not particularly helpful to anyone.

Your claim regarding there "never being a way for crate owners to disable overflow checking" doesn't accurately reflect the text of the RFC. For one thing, the RFC includes wrapping primitives that can be used (perhaps as part of a macro, if you prefer). As far as the normal operators go, the RFC does not include any current means to disable checks, that is true. However, it does say, "In the future, we may add some way to explicit disable overflow checking (probably in a scoped fashion). In that case, the result of each error condition would simply be the same as the optional state when no panic occurs." I suppose it is not written explicitly, but I think it is implied that if users explicitly disabled checks in some local fashion, then this would take precedence . I can make that more explicit. That said, I think it is preferable for wrapping semantics to always be expressed using the wrapped methods and the Wrapped types, just to reduce the number of ways to express the same thing. In general, part of the idea of this RFC is to retain future options to the extent possible. This includes backing off from overflow checking if we find it is infeasible or unhelpful, though I think that's unlikely.

Regarding the question of data, I agree that I haven't provided hard statistics on overflow bugs. I would be very interested in seeing such statistics (I'll note that I haven't seen any statistics the other way, however). The experience with porting Rust was, while not a slam dunk, more suggestive than you think. For one thing, a number of...suspicious oddities were uncovered that were not anticipated, even if they don't necessarily qualify as bugs. For example, the fact that the enum discriminant counter can overflow, potentially leading to duplicate discriminants (or at the very least, not particularly well-defined discriminants). Let's call those near misses. I suspect there are plenty more overflow bugs to be found; dynamic testing is limited to reporting problems that actually occur, and I don't think we do extensive testing that targets overflow. (That said, I know that @eddyb has recently encountered various oddities and undefined behavior concerning overflow in static constant evaluation, though I think the specifics there are complicated and I don't want to claim that this RFC would help in diagnosing such problems necessarily.) (Note also that the standard library already takes some amount of care to use checked operations, particularly in the containers libraries.)

In any case, my own opinion on overflow is based more on word of mouth, but word of mouth from several authoratative sources. For example, Robert O'Callahan has stated on numerous occasions that integer overflow complicates the gfx module in Firefox. Here is one such example: "Overflowing integer coordinates can lead to infinite loops and incorrect layout or rendering, the latter of which can occasionally have security implications. Task failure is better than both of those. Generally, the sooner we detect bugs and fail the more robust we will be against malicious input. Being able to harden the code against a common class of bugs without making the language any more complicated is very attractive to me." The RFC includes further citations which are absolutely worth reading.

One of my biggest concerns involves unsafe code. While I don't expect the checks themselves to detect many bugs in such cases, I do expect the awareness of overflow to lead people to use the explicit checked/wrapped variants (and I would support a lint warning against using the normal operators in functions that contain unsafe blocks). This in turn will make code more robust. The RFC already cites this phrack article on the dangers of overflow, but more sources are readily available that indicate that overflow can easily be a contributing factor to security hazards.

I agree incidentally that the Google guidelines are only tangentially relevant to Rust; but then I only cited them as evidence that unsigned underflow is considered by some to be a more serious problem than overflow. It is certainly a more common problem in my experience. The dangers in Rust are less, since we rarely write naked loops and we have lints for useless comparisons, but it's still possible to make a mistake when counting downwards. I know because I've done it on occasion. ;) Underflow is kind of interesting because (again, in my experience) it tends to occur so much more readily, which means that the debug checks will probably be a quite useful diagnostic even without writing special tests.

@nikomatsakis
Contributor

@mkaufmann I'll go through your points and try to incorporate them, but let me respond to your overall summary here:

Explicit checked operations should be defined

These are not included because they are already present in the standard library. It's possible that these interfaces should be improved in some way, but they do exist.

I still don't see a good reason why to allow undefined behaviour when overflow checking is "optional"

I think you are correct and my wording was unnecessarily "undefined". I don't actually expect the compiler to randomly inject checks, after all. I primarily wanted to convey that authors should not use + when overflow is a possibly expected outcome, because it is possible that others will build their code with overflow checking always enabled (or analyze their code with static analysis tools). The whole idea of having overflow checking be mandatory in debug builds was to drive this point home. I very much believe that whatever a spec says about defined vs undefined, the actual behavior is what matters the most, which is why I want to ensure that actual overflow checks are being performed at some point during regular development.

I also am very amenable to tweaking the defined outcomes for various mathematical operations. I think our general intention here has been that, in the absence of overflow checking, we will try to map as directly as possible to builtin operations. In some cases this leads to undefined values being produced due to differences between architectures and so forth (shifting being a natural example, but perhaps division falls into that category as well). In any case I consider this a semi-orthogonal question.

The RFC still assumes that overflow checking in production builds brings a benefit. As previously argued by me it actually leads to more unsafety. Overflow checks are only helpful during development.

First, the RFC itself only enables overflow checks during development. I do believe that it would be helpful to have more overflow checking, but I think that any move to make that a default would require a further RFC. This would occur only when we have more experience to go on.

I just re-read your arguments concerning classes of bugs as well as those of others. I tried to address them in the RFC but I'll give it another go. I think the two biggest points are the divergence of debug/unsafe and the fact that there are new points where panic/unwinding can occur.

  1. Differences between debug/optimized builds don't worry me. Differences already exist. It's universal practice to litter safety-conscious code with assertions that get compiled out. There is no question that this makes code safer overall (and no question that it'd be better on average to run with those assertions enabled permanently, if possible).
  2. The necessity of writing exception-safe code in unsafe blocks doesn't bother me. This is already basically a requirement, since panics can occur in various scenarios. Moreover, a lint against using naked + and - in unsafe fns or fns that contain unsafe blocks would basically make this a non-issue. Explicit use of wrapping and checked primitives would bring the possibility of overflow to the attention of readers. I think I should add some text to the RFC explicitly calling for a lint; the only reason I didn't is because it seems like a small detail that could be done independent of an RFC. (In my view, this RFC is primarily about changing the direction of the language away from defined overflow semantics, we can litigate some of the fine details as we go.)

Am I missing an important point here?

@nikomatsakis
Contributor

Ah, @glaebhoerl I just saw your comment regarding aborting the process on overflow. I am pretty opposed to that for a various reasons.

  1. Uncontrolled abort is a very clumsy tool that is not suitable for a number of applications. For example, Skylight, Servo, and even rustc try to have some form of controlled shutdown. This is made much harder by the use of abort.
  2. Division by zero already panics and that seems like a reasonable precedent.
  3. I believe we already have a means for disabling unwinding overall and simply using abort instead; certainly I have no objection to making that an option. It seems better to me to make this an orthogonal choice.
@mkaufmann

@nikomatsakis thanks for your remarks. I think our opinions are mostly aligned, the disagreement is probably mostly over the text in the RFC.

Explicit checked operations should be defined

This was clearly an oversight be me, so this point is adressed.

I still don't see a good reason why to allow undefined behaviour when overflow checking is "optional"

Your comments clarify it somewhat, but I still had a hardtime understanding the different uses of "optional" in the RFC (it seems to be used in two contexts one is that overflow checking might not always be activate and the other beeing that when debug_asserts are disabled that the compiler is free to omit the overflow check or throw a panic). Especially the second meaning looks like undefined behaviour to me. I don't know if this is a misunderstanding on my side or intended meaning? I would rather have a more clear state description like

  • debug_asserts enabled: Panic on overflows ( I also argue there should not be an option to disable)
  • debug_asserts disabled: Use wrapping semantics, division by zero is the only operation that panics.
  • debug_asserts disabled + User activates overflow checking: Panic on overflows.

This specified the behaviour for all scenarios and there is no undefined behaviour or values left (if we agree to defining the result of INT_MIN/-1 as INT_MAX).

The RFC still assumes that overflow checking in production builds brings a benefit.

For me "the fact that there are new points where panic/unwinding can occur" is not just limited to unsafe code, but also when putting code into production. If overflow checking would ever be pervasive (as stated as a future goal in the RFC) the range of bugs would change from [incorrect value -- crashing -- hanging -- security breach] in order of severity to [crashing]. For me >99% of the bugs would probably fall into the category incorrect value, especially because rust provides memory safety and sane loop iterators. For all these bugs the result would detoriate and as a user crashing software is really frustrating or can also have security implication.

Because the benefit of having overflow checking in non debug builds is dubious to me I am arguing that:

  • The future goal having universal overflow checking should be removed
  • Non debug builds should per default be defined as wrapping (removing the complicated "optional" / undefined behaviour part
  • Provide a flag which activates overflow checking if desired by the user.

I think this makes a much more clear cut story.

@mkaufmann

In my comment above I forgot shifts. So it should read "only division by zero or shifts with more bits than the size of the datatype should panic"

@nikomatsakis
Contributor

@mkaufmann I basically agree with you and will up date the RFC accordingly. I am not sure whether I consider "universal overflow checking by default" to be a good idea or not, but regardless I consider it a future option that we may or may not want, and I think we can debate that in the future when there is much more data to consider.

@eddyb
Member
eddyb commented Jan 30, 2015

What @nikomatsakis is alluding to is my rvalue promotion work, which needed const_eval to enforce constant division and remainder are not UB.
Outside of that, const_eval is used for the lengths of fixed-length arrays and C-like enum values (apart from some other stuff in pattern-matching, but that isn't really relevant).
As such, it ends up being called before type information is available, so it ends up "approximating".
Internally, it uses i64 and u64 and it needs to perform wrapping arithmetic to implement the correct rules, and do custom overflow checking (eventually) - it just can't do that very well without any type information.

In other words, checked overflow would do nothing for the implementation, even though stricter rules could let us error as early as possible instead of having to keep around overflowing literals to show some warnings.

@eddyb
Member
eddyb commented Jan 30, 2015

Personally, I want integer literals to evaluate to wrapping arithmetic types, so I don't have to write W(123) everywhere (anything longer is even less acceptable).

And there's this issue that I haven't seen being brought up: there is no obvious way to add iN and uN given overflow rules, the shortest method I could find is:

// what used to be (u + (i as uN))
if i < 0 {
    u - ((-i) as uN)
} else {
    u + (i as uN)
}
@nikomatsakis
Contributor

@eddyb I think the obvious way would be u.wrapping_add(i.wrapped_as_uN()). That said, I recently noticed that the RFC doesn't include any wrapped_as_uN() methods, which is an oversight imo that should be corrected.

@nikomatsakis
Contributor

In other words, checked overflow would do nothing for the implementation, even though stricter rules could let us error as early as possible instead of having to keep around overflowing literals to show some warnings.

Thanks for the clarification. That said, even just making it more clear where the error lies feels like an improvement in my book. (Also, I suspect we would catch some strange cases when downcasting from u64 to the eventual target type, but I agree it wouldn't be a thorough check by any means -- perhaps enough thought to alert a fuzzer that something was broken.)

@nikomatsakis
Contributor

Also, I suspect we would catch some strange cases when downcasting from u64 to the eventual target type, but I agree it wouldn't be a thorough check by any means.

But perhaps we never downcast, because we just convert into a constant for the LLVM representation. Not sure.

@nikomatsakis
Contributor

@mkaufmann to be clear, I was agreeing that I will amend the RFC to be clearer about when overflow checks occur and not leave it as vague and undefined. However, I don't really want to change the existing target-dependent semantics of shifts or other operations -- I am quite sympathetic to the idea of making that better defined, but I think it belongs in a separate RFC and is worthy of a separate discussion (perhaps with some measurements).

@mkaufmann

Ok makes sense. Just a minor detail, division is defined as:

If no panic occurs, the result of such an operation is defined to be the same as wrapping.

While shift is defined as

If no panic occurs, the result of such an operation is an undefined value (note that this is not the same as undefined behavior in C).

Can't both be described as same behaviour as wrapping (which would implicitely allow undefined behaviour for shift as it is currently with divsion)? That way the semantics of wrapping integer operations can than be cleared up in a seperate RFC and won't touch this RFC. If the behaviour of shift is defined in this RFC, it would need to be amended if we change the shift semantics in the wrapping RFC. Again just minor nitpick ;)

@nikomatsakis
Contributor

@mkaufmann Sure. That's roughly what I did.

@eddyb
Member
eddyb commented Jan 30, 2015

@nikomatsakis This is what I was talking about, wrapping is not the solution (if you want any checked arithmetic at all) - my version still checks for overflow, but only does so for the cases that are semantically relevant to adding a signed integer to an unsigned one.

@mkaufmann

@nikomatsakis Awesome work! Thanks for your patience and greate idea with the lint for unsafe code :). I really like this RFC now +1

@eddyb Wrapping != checked arithmetic. The wrapping semantics is what is currently used when you use the arithmetic operators. So while wrapping_X might look a little bit more verbose, there will not be any difference in the result of operations compared to the operators today and also not in performance.

@nikomatsakis
Contributor

@eddyb if I understand you, you are saying "it would be useful to allow a signed offset to be added to an unsigned value, and to then do overflow checking in an intelligent way". In other words, if you have A + B, where A is unsigned and B is signed, you error if (B < 0 && abs(A) < abs(B)) || (B > 0 && (abs(A) + abs(B) > MAX))? If so, that does seem useful, though it also seems like an extension, and like something that could be provided by a function as well. There are some concerns about allowing signed/unsigned values to mix that are relevant.

@glaebhoerl
Contributor

That sounds very relevant to the integer promotion debate - we could offer impl Add<i32> for u32 which works that way1 - which is conceptually very similar to how "heterogenous comparisons" would work.

1 (Though what would it return? Maybe this would need the polymorphic return type...)

@nikomatsakis
Contributor

@glaebhoerl yeah I said the same thing on IRC -- it's not promotion exactly, but it carries a similar feeling.

@nikomatsakis
Contributor

@glaebhoerl per the orphan rule setup, it should take type of the lhs operand I imagine...

@rkjnsn
Contributor
rkjnsn commented Jan 30, 2015

per the orphan rule setup, it should take type of the lhs operand I imagine...

This was also my first thought.

@eddyb
Member
eddyb commented Jan 31, 2015

@mkaufmann I didn't argue about that, but I have thousands of lines of code that depend on wrapping semantics and using wrapping_add instead of + would be a crime.
Unless you're referring to my other comment, in which case, see @nikomatsakis' reply.

@rocallahan

I wouldn't expect to find many/any bugs when converting code (e.g. rustc) to work with overflow checking --- if you're not intensively fuzzing that code. In my experience with Gecko, overflows are almost never triggered by normal usage in the wild, but they are triggered a lot by malicious inputs, such as from fuzzers.

So, an interesting experiment would be to generate a set of fuzzed testcases and run them through rustc with and without overflow checking, and see if overflow checking converts a significant number from "generates incorrect output" to "compilation failure via overflow check failure".

I strongly support with the goals of this RFC, especially the last one, "leave room in the future to move towards universal overflow checking", since I think there's a high chance that will be desirable at some point. I suspect the final semantics of overflow checking won't exactly match this RFC, but that won't stop this RFC from meeting its goals.

@nikomatsakis
Contributor

I've been re-reading comments on this thread and there is two points that @mahkoh has made at various times that I want to be respond to in more depth. I think these points have already been made but I want them collected and documented in one place; I will try to add them into the RFC as well if they are not already present.

Evidence of harm

As evidence that overflow bugs occur in the wild, I recently pointed at mailing list threads by @rocallahan and others. However, a more detailed look is available in the excellent paper "Understanding Integer Overflows in C/C++". One of the authors (John Regehr) was also a participant in the mailing list thread. The paper exhaustively documents the prevelance of bugs relating to overflow across a wide variety of projects and even well-tested benchmarks like SPEC. It also highlights the danger of undefined results, so it provides good evidence in favor of @mkaufmann's arguments for well-defined results in the absence of a panic (though I think an undefined result is significantly weaker than undefined behavior in general).

One of the major points that this paper makes is that it is very difficult to differentiate cases where overflow is accidental from intentional; both intentional/unintentional overflow occur frequently in real code bases. This is the situation that this RFC aims to avoid.

It is important to emphasize that even if the changes in this RFC fail to help identify overflow bugs, they will definitely help to segregate intentional use of overflow from unintentional use. As I've stated before, I agree that the measures in this RFC are not sufficient to detect overflow bugs: they must be used in tandem with fuzzing, static analysis, or other tools. But I think they will be helpful in making those tools practical, because they both provide an easy way to check for overflow and make it clear that intentional use of overflow is identified.

Undue syntactic burden on intentional use of overflow

The RFC does not include any means of disabling overflow checking completely other than the wrapping methods -- overflow checking can always be enabled via command-line switches from end-users. This means that library code which wishes to take advantage of wrapping semantics must use the wrapping methods or a newtype. This imposes some amount of syntactic burden on such code.

One easy way to lighten that burden would be to allow scoped annotations that explicitly disable overflow checks in some region of the code. I still consider this a viable future option if the syntactic pain proves too high. However, there are good reasons to try and avoid this option:

  1. It makes the semantics of + less clear. One would have to search the enclosing contexts to identify whether overflow checks have been explicitly disabled for that region of code. This is a non-local change that could occur at any point in the module hierarchy. This makes reading code harder.
  2. More than one way to do it. It is not clear whether it is preferred to use the provided methods or to disable checks in a local region of the code.
  3. Improving the ergonomics of wrapped integers is a worthwhile goal. There are language extensions we could consider (such as overloaded literals) that might go a long way towards making the more explicit options ergonomic.
  4. Scoped wrapping semantics don't extend to user-defined types. If users define a type MyInt, it will not be aware or respond to the scoped wrapping semantics.

Another option might be the addition of new operators (e.g. +%, as in Swift) that explicitly request wrapping semantics.

@rkjnsn
Contributor
rkjnsn commented Feb 3, 2015

One easy way to lighten that burden would be to allow scoped annotations that explicitly disable overflow checks in some region of the code.

This strikes me as the completely incorrect way to go about easing the use of wrapping semantics.

I can see scoped annotations being useful for security and/or performance reasons. One could then have overflow checking for a given security-critical region, even in release mode (in this case, only panicking if the value is used may be desirable). One could also choose enable overflow checking globally and mark performance-critical sections not to be checked (in this case, one may still want checks in those regions enabled for some debug builds).

However, in both of these cases, the semantics are the same: wrapping is unexpected and an error; it's only the amount of checking that varies. Using scoped annotations to change the semantic meaning of operations seems very, very wrong. As you mention, one would have to search all enclosing contexts to determine whether operations were meant to be wrapping, and it still wouldn't be clear if it were for performance reasons (overflow is still incorrect) or algorithmic reasons (overflow is expected). Combined with the other points you make, I would find using scoped annotations to determine whether wrapping is desired to be completely unreasonable.

Something like providing special wrapping operators seems like a much better approach, if better ergonomics are needed.

@glaebhoerl
Contributor

Agree with @rkjnsn. Scope-based attributes could also be a decent way to address the "severity" issue @mkaufmann raises (and which others have also raised) -- i.e., the programmer could endeavour to turn checks in release builds off for areas of the code where an overflow would merely result in an incorrect value being displayed, and on for areas where it would be more catastrophic. (Of course, the programmer's estimation of this is unlikely to be perfect, and lexical scope may not be the best granularity, but it's something.)

@alexchandel

👎 I expect my unsigned and two's complement integers to wrap around. I want them to. That's what finite integers like u8 and i8 do. Integer overflow should be the default for all uses of arithmetic operations. It is predictable, well-defined behavior. And it doesn't change between debug-time and release-time. I don't see anything wrong with an explicit checked_add() or add_or(), but that shouldn't extend to unadorned arithmetic. Almost no one needs overflow checking, and it shouldn't be assumed in every operation unless you opt out. Not to mention this creates a whole new class of bugs which are program errors yet which may not be detected in safe code in release builds. Adding this to Rust would be a bitter disappointment.

@pornel
pornel commented Feb 4, 2015

@alexchandel I don't share your position, even in case of u8. I work with graphics algorithms and there overflow is sometimes desirable (e.g. PNG decoding requires it), but most often it creates ugly errors (e.g. white areas of the image get black holes).

In video decoding algorithms it's very important to handle overflows in a correct way, but it's also very hard to do due to complexity and performance requirements of the code.

Checked overflow would help me catch these bugs earlier (and perhaps change to saturated add/multiply where it happened) and would make it sooo much easier to find the location that overflows.

@dgrunwald
Contributor

It makes the semantics of + less clear. One would have to search the enclosing contexts to identify whether overflow checks have been explicitly disabled for that region of code. This is a non-local change that could occur at any point in the module hierarchy. This makes reading code harder.

This is an interesting argument. In C#, unchecked/checked is only available as statement or expression. This reduces the "action at a distance" to the current function, so it's easy to see the context when looking at the arithmetic operator.

@aturon aturon referenced this pull request in rust-lang/rust Feb 6, 2015
Closed

Tracking issue for Integer Overflow (RFC 560) #22020

11 of 15 tasks complete
@aturon aturon merged commit 9fb9ad6 into rust-lang:master Feb 6, 2015
@aturon
Contributor
aturon commented Feb 6, 2015

This RFC is a continuation of a long-running discussion about overflow:

The topic has been covered in considerable depth in those earlier threads as well as on this RFC comment thread itself. While the discussion has been contentious at points, a fairly clear picture of the tradeoffs in each direction has emerged.

There were concerns about the performance hit even if checking is restricted to debugging builds. Some details about the hit and possible mitigations are in this comment. Other mitigation strategies include scoped disabling of checking, as was proposed in the initial RFC.

There were concerns about the clarity of the RFC text. This was partly intentional as the precise details of checking and panicking semantics will likely need some implementation work to fully sort out. But @mkaufmann's comment also helped clarify the text.

There were questions about whether over/underflow detection as proposed would actually catch many bugs. The clearest response is probably @nikomatsakis's comment, which discusses some published research on the topic as well as the need of using fuzzing and other tools in conjunction with the checking proposed here.

A key point that emerged is that, at heart, this RFC is about distinguishing intentional and unintentional over/underflow. There are many ways to make the distinction, but the most promising is by differentiating types, since mixtures of desired and undesired over/underflow for the same value are expected to be rare.

In the end, the core team has decided to move forward with this RFC. If we ever hope to distinguish between intentional wrapping and unintentional wrapping, this is the time. The specifics of interaction with optimization, defaults and opt-outs can be tweaked over time.

Tracking issue

@bors bors added a commit to rust-lang/rust that referenced this pull request Mar 1, 2015
@bors bors Auto merge of #22532 - pnkfelix:arith-overflow, r=pnkfelix
Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

cc @Aatch ; cc @nikomatsakis
4850180
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 2, 2015
@Manishearth Manishearth Rollup merge of #22532 - pnkfelix:arith-overflow, r=pnkfelix
 Rebase and follow-through on work done by @cmr and @aatch.

Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

See also PR #20795

cc @Aatch ; cc @nikomatsakis
a8fec5c
@bors bors added a commit to rust-lang/rust that referenced this pull request Mar 2, 2015
@bors bors Auto merge of #22532 - pnkfelix:arith-overflow, r=pnkfelix
Rebase and follow-through on work done by @cmr and @aatch.

Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

See also PR #20795

cc @Aatch ; cc @nikomatsakis
f572ac0
@bors bors added a commit to rust-lang/rust that referenced this pull request Mar 3, 2015
@bors bors Auto merge of #22532 - pnkfelix:arith-overflow, r=pnkfelix,eddyb
Rebase and follow-through on work done by @cmr and @aatch.

Implements most of rust-lang/rfcs#560. Errors encountered from the checks during building were fixed.

The checks for division, remainder and bit-shifting have not been implemented yet.

See also PR #20795

cc @Aatch ; cc @nikomatsakis
14f0942
@pnkfelix
Member
pnkfelix commented Mar 4, 2015

I am accumulating a list of actual bugs uncovered via the arithmetic overflow checks here:

I welcome others to tell me about any bugs they found via these checks. (Note that I am only interested in actual bugs; not in benign overflows; see the document for more discussion.)

Update: The above document was originally in an etherpad (you can find out which one via its commit history). But now it is in a github repo, so that others can submit pull requests with new stories of bugs arithmetic-overflow found in their code.

@rocallahan

I wouldn't expect you to find a lot of integer overflow bugs until you do some intensive fuzz testing.

@nikomatsakis
Contributor

@rocallahan agreed (as we said earlier), but it's still good for historical record to know what bugs were encountered even without fuzz testing (and there certainly have been some).

@pnkfelix pnkfelix added a commit to pnkfelix/collab-docs that referenced this pull request Mar 6, 2015
@pnkfelix pnkfelix Port of arith-overflow bug list from my etherpad ca617ae
@pnkfelix
Member

The RFC has this text:

When truncating, the casting operation as can overflow if the truncated bits contain non-zero values.

I have written an internals post regarding the fine details of what this sentence is saying (or should instead be saying); the post is here:

http://internals.rust-lang.org/t/on-casts-and-checked-overflow/1710

If you had an interest in the discussion here or want to provide input on the future semantics of <expr> as <int-type>, you may want to go read it and write feedback.

(I note in particular both @mahkoh and @quantheory were discussing the details of cast-semantics in the discussion above, and therefore may want to chime in on the internals thread.)

@pnkfelix
Member
pnkfelix commented Apr 2, 2015

@glaebhoerl wrote:

As per this discussion with @petrochenkov, I think we should remove unary negation for unsigned types as part of this. (cc also rust-lang/rust#5477)

As a very very last minute change (that should still manage to land in the beta release, if all goes well), based on discussion here: "A tale of two’s complement" and here: "Forbid -(unsigned integer)", the team decided in the most recent weekly meeting to feature-gate the unary-negation operator on unsigned integer values.

This is largely based on following the philosophy presented here on RFC #560: the arithmetic operators should be use in a manner that does not cause overflow; if one wants bitwise operations, then one can e.g. rewrite -expr as !expr + 1, or rather the most general case, as (!expr).wrapping_add(1).

The PR that puts in the feature-gates is: rust-lang/rust#23945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment