RFC changing the overflow behavior for usize in release builds to panic

[alex]

Rendered

[Ixrec]

I suspect this change is not feasible without also providing some way to opt-out of these runtime checks, even in release mode. An obvious option is to add a set of unchecked_add/sub/mul/etc methods to usize, though it would be a little weird to have both a checked_add and an unchecked_add method neither of which match the behavior of +.

[alex]

What makes you say it's not feasible, performance or some other concern? How would the behavior of unchecked_* differ from wrapped_*? (Is it the method I hypothesized that matches the current debug_assert! behavior?)

[Ixrec]

Yeah, performance. My understanding is that enforcing any specific overflow semantics has a measurable cost on some platforms, even if it's zero-cost on other platforms, because different hardware has different "default" overflow behavior. Rust is less weird than C because it requires two's complement, but as far as I know that doesn't guarantee a specific overflow behavior (at least, I can't imagine we'd have chosen the current behavior otherwise) (EDIT: huh, apparently we do already guarantee wrapping in release builds). Someone more familiar with these kinds of performance portability issues would have to comment on how big of a deal that is in practice.

[comex]

I think this RFC fundamentally needs actual benchmark results (ideally a large variety of them), to allow everyone to have an informed discussion about costs versus benefits.

[steveklabnik]

I like this RFC, and I believe that is the direction we want to move in, ideally. A few things:

• RFC 560 defined the rules for integer overflow, mentioning it would be good
• Additionally, note that the RFC text is a bit inaccurate in a sense; it's not "in debug mode" but "when debug_assertions are enabled. Basing it off of the RFC 560/reference text would be better.

Furthermore, I agree with @comex; RFC 560 mentions performance a number of times as the reason to make the decisions we made for the current behavior; if the performance were acceptable, I think there'd be broad support for checking in every case, let alone for just these sizes.

---

@Ixrec

My understanding is that enforcing any specific overflow semantics has a measurable cost on some platforms, even if it's zero-cost on other platforms, because different hardware has different "default" overflow behavior.

Yes, this is true, but we decided that we don't care as much about supporting that hardware as we do having consistent semantics. Non-two's compliment hardware is mostly legacy that we don't support particularly well in the first place, see discussions like https://gankro.github.io/blah/rust-layouts-and-abis/

NOTE: this is not a normative document and the Rust devs haven't been very diligent in committing to these claims, so be a bit wary of relying on a property here that lacks a citation.

For Rust to support a platform at all, its standard C dialect must:

• Have integers be two's complement

As the note says, it's not normative, but I think, in this moment, it's roughly true.

[alex]

I agree that measurements will ultimately be a necessity to accepting/merging this idea. In writing the RFC first, I wanted to address the chicken/egg scenario of writing the code before it was clear the idea was acceptable -- truth be told I expected people to find the idea of special casing usize to be more unusual than the feedback has been thus far!

There probably is performance differences on different architectures beyond the concern for those which don't have 2s complement arithmetic. x86 and ARM both have dedicated instructions for jump-on-overflow, however PPC and MIPS do not, so there's a non-trivial difference on the amount of code generated on these platforms for overflow checks.

That's an excellent point about the current behavior being keyed on debug_assertion, not debug. I'll update the text accordingly.

[hdevalence]

I think this is a great proposal and I'd love to see it adopted by Rust. Having an additional security layer is a huge advantage, and as noted in the description, it would have prevented exploitation of a number of past security issues.

Currently it's possible to enable overflow checks for all integer types using something like

[profile.release]
overflow-checks = true

in the Cargo.toml. However, this can't be used to test the proposed default, because it enables checks for all integer types, not just for usize/isize.

Perhaps a good way to move forward on this would be to add a (possibly unstable) mechanism, like usize-overflow-checks, which would allow enabling overflow checks just for usize/isize, and then running ecosystem-wide benchmarks (like lolbench) to see what the actual impact on real code is.

This mechanism might be useful independently of whether the behaviour proposed in the RFC is adopted; for instance, I would choose to use it in my projects.

[leonardo-m]

If you want to introduce this I'd like it with an optimization pass as done by the Swift compiler.

[alex]

@leonardo-m Can you explain what you mean by that? I don't see overflow-checking as being something that'd be implemented as a compiler optimization. Generally the compiler optimization piece of this is removing overflow checks where there are range bounds on the inputs.

[leonardo-m]

I meant adding this to Rust:
https://github.com/apple/swift/blob/master/lib/SILOptimizer/Transforms/RedundantOverflowCheckRemoval.cpp

[alex]

Yes, as I mentioned in the Future Plans section, I suspect this work will spur desires for new and improved optimizations, either in Rust or LLVM. It is my hope that such optimizations will be nice to haves and not hard requirements for this to be practical.

There seems to be a lot of interest (unsurprisingly) in quantifying this. I'll take on trying to put together a proof of concept of this for Rustc that's good enough to get some measurements.

[scottmcm]

As a small thing, I think this should also happen for isize -- code that's using offset might be calculating in that type instead, and just generally I'd like the two types to be as similar as possible.

[scottmcm]

I really like the nuance in doing this just for the types that are most likely the problem. And the scariest perf problem would be something like a[i+1], but LLVM already needs to be proving that that addition doesn't wrap in order to remove the bounds check anyway, so I suspect this might not be quite as scary as it seems. (If it can't remove the wrapping check, it can't remove the bounds check either, so two highly-predictable branches might not be materially worse than the already-existing one.)

That said, LLVM still can't fold sequential checked_adds (https://rust.godbolt.org/z/z7LUMt -- but it can saturating ones, thank you rust-lang/rust#58003!), so there's definitely low-hanging work here that could improve perf. (Maybe I should re-open rust-lang/rust#52203.)

As an upper-bound perf experiment, consider just changing the default in release to be panic, and we can run a try+perf on the experimental PR to see how far off things might be.

[briansmith]

ring: Near-miss integer overflow, which would have led to heap buffer overflow.

There was never such a bug in ring and the necessary checks are done there, so I don't think it makes sense to mention it here. (in_prefix_len > in_out.len() is a condition that is complete nonsense in that codebase.) If anything ring's track record is evidence that this RFC is unnecessary.

[briansmith]

CrosVM: Integer overflow leading to heap buffer overflow in ChromeOS's hypervisor.

According to the bug report "This didn't end up being an actual issue as it was covered by an early bounds check."

I didn't check the other two examples cited as motivation. Also, I am mostly supportive of this proposal. However, I think the citations suggesting non-vulnerabilities are vulnerabilities should be removed.

[alex]

In Ring's case, that's why I described it as a "near miss" -- since it was unreachable, but the check wasn't near the overflow, and there was no type or other structure for ensuring the invariant. It was included because it demonstrated the potential pattern of overflow on usize + unsafe code. If you still think that's misleading, I can remove it.

In CrosVM's case, the overflow was probably unexploitable (at least, none of us figured out a way to), because of the size of the input data was bounded, not because there was any checking on the actual result of the arithmetic. This one definitely makes sense to include, in my view.

[briansmith]

I think in both cases, the code that prevents the overflow is far away from the code that it is protecting, and minimizing that distance is generally good.

I don't know all the details regarding the CrosVM case. In the case of ring, at least, it's not accidental that overflow is avoided. Bounding the input size very early to prevent integer overflows later is a pretty common practice. For example, this is one reason why mozilla::pkix (C++) limits its input sizes to 0xffff (uint16_t) or less. The untrusted library does something similar, but not as drastic. These libraries do things this way partly to minimize any potential performance impact; i.e. as an alternative to what's being proposed here.

Anyway, again, I'm not trying to argue against this RFC; just I think the way ring is referenced is very misleading.

[scottmcm]

I didn't check the other two examples cited as motivation.

As the one who found the first one, it's definitely exactly this, and was fixed by making the overflow panic: https://github.com/rust-lang/rust/pull/54399/files#diff-773807740b9d7f176c85b4e2e34b2607R434

[tomwhoiscontrary]

There probably is performance differences on different architectures beyond the concern for those which don't have 2s complement arithmetic.

There is a proposal floating around in C++ space to declare that Signed Integers are Two’s Complement. In that proposal, there is a survey of signed integer representations which concludes that there are no current non-two's-complement hardware architectures. The closest thing is some Unisys product line which has an ASIC to support code written for its '60s mainframes, which were ones' complement (and had 36 bit words!).

So, i would suggest that it's safe to not consider non-two's complement machines at all.

[joshtriplett]

We discussed this briefly in the lang team meeting, and generally agreed with the thread on the following:

• We should add a way to enable overflow-checks just for isize/usize and not other types.
• Once we have that, we should get some data on the performance and security impact of using that option: does enabling overflow-checks only for isize/usize provide a substantial security/correctness improvement with a disproportionately low performance cost?
• Given that data, we could decide what the default should be, but in the meantime people could opt into this new mode.

[steveklabnik]

According to the previous RFC and policies, this is a backwards compatible change. Of course, we've always tried to do better than simply the letter of the law, but we'd have to demonstrate real-world breakage.

…

On Mon, Feb 18, 2019 at 2:19 PM vorner ***@***.***> wrote: I do find usize acting differently than eg. u8 a bit weird and usually I really dislike inconsistencies. But that being said, I think this instance of weirdness is quite OK ‒ rust's stance to overflowing is mostly „don't do it, but I might not check every time you don't, it's up to you“. So tweaking the rules when this „might not check“ happens sounds good to me, especially if it tends to prevent bugs. But I'd like to raise a concern that I haven't seen here. Is this backwards compatible change? Not everyone necessarily shares my understanding of „please don't overflow“ and might have decided to a) turn off the checks in Cargo.toml, b) rely on the overflow not panicking. By upgrading the compiler to never version, wouldn't such code stop working? Or is the general stance that every overflow is actually a bug therefore that code should have never worked in the first place? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2635 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABsiiRK5WrKFQ7odoHOxsbBQ7cbjk18ks5vOwrGgaJpZM4azQVI> .

[mark-i-m]

Perhaps an unresolved question could be added to request a crater run?

[rfcbot]

Team member @scottmcm has proposed to postpone this. The next step is review by the rest of the tagged team members:

• @Centril
• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[scottmcm]

While I like this conceptually, it seems like current measurements of experimenting with this are showing about a 2-6% impact to the speed of the compiler. As such, I propose that we postpone considering changing the release default for now, pending further work.

@rfcbot fcp postpone

Process notes: I'm only proposing postponing the T-lang change here. I (personally) would absolutely like to see work here continue, as better optimizations around checked math will help even if we never change a default here (for example, in Range::nth). And if T-compiler decides that this is a useful option to expose for general opt-in use, I would be happy for that too. But AFAIK those aren't lang decisions.

[alex]

First missed optimization I've found: rust-lang/rust#58692

[alex]

Ok, tracking down optimizations and getting fixes upstreamed into LLVM is a slower process than I'd hoped.

In order to keep some momentum here, I'd like to propose a flag for rustc that implements the behavior described here; of doing panic for usize arithmetic but leaving other types alone. Does this require a full of RFC, or can it simply be pull requests to rustc?

[scottmcm]

@alex AFAIK the compiler team decides what flags the compiler has, and an unstable flag is plausibly something that would be accepted, assuming the code maintenance cost is low.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[the8472]

Would it make sense to only add checks to either functions or modules that contain unsafe code? The heuristic here would be that bounds checks either happen in the unsafe code itself, the surrounding function or its sibling functions that encapsulate it into a safe interface?

[alex]

functions is definitely overly granular, you can fairly straightforwardly have an integer overflow where the actual corruption happens in an unsafe block elsewhere. The problem I see with schemes like these is that they make it too difficult to reason about the level of protection you're getting.

…

On Thu, Apr 25, 2019 at 6:01 PM the8472 ***@***.***> wrote: Would it make sense to only add checks to either functions or modules that contain unsafe code? The heuristic here would be that bounds checks either happen in the unsafe code itself, the surrounding function or its sibling functions that encapsulate it into a safe interface? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2635 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAAGBEMZEHL2VG7PMKSJOTPSIS2NANCNFSM4GWNAVEA> .

-- All that is necessary for evil to succeed is for good people to do nothing.

[the8472]

Well, if your module publishes a function, even if it is just pub(crate), and it can corrupt memory unless the caller gets some checks right then it itself should be unsafe. On the other hand a crate-level pessimization seems overly coarse, e.g. if you have one set of heavy operations on slices in one module and the unsafe stuff segregated into another one. So module-level granularity feels right.

[fowl2]

Could some of the performance anxiety be mitigated by using unchecked_add where profiles demanded it? Ahh #2508 suggests this could be unsafe, I guess arbitrary_but_safe_add doesn't quite roll off the keyboard.

[rfcbot]

The final comment period, with a disposition to postpone, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC is now postponed.

[alex]

I have not forgotten about this, and continue to (as time allows) work on the missed-optimizations problem: rust-lang/rust#72549 is the current known blocker.

rust-lang/rust#72237 was brought to my attention as an example of another bug that would have been caught by this RFC.