Add debug asserts to some unsafe operations

[newpavlov]

The following functions have restrictions which (ideally) should be respected:

• get_unchecked
• get_unchecked_mut
• slice_unchecked
• slice_mut_unchecked
• unreachable_unchecked
• raw pointer deref (Check alignment of raw pointers in debug mode #54915)
• etc. (please comment if you know additional examples)

It would be nice to check these restrictions with debug asserts. The main blockers:

• stdlib is distributed with disabled debug assertions, so either implementation will have to use the same hack as in wrapping checks, or we'll have to distribute two versions of stdlib with enabled and disabled debug assertions, and teach cargo to switch between them depending on a compilation profile.
• Some code in the stdlib (and probably in some external crates) consciously breaks those restrictions (e.g. src/liballoc/vec.rs). Probably it should be rewritten in a more "correct" fashion.

See internals thread for additional discussion.

[gnzlbg]

or we'll have to distribute two versions of stdlib with enabled and disabled debug assertions

I think we would need at least three stdlib versions for this: debug+debug_assert, release+debug_assert, and release. Depending on how you look at this, we might also need to ship a debug version without debug assertions, so that would mean we need to ship four versions.

This gets out of hand quickly. Given M options with 2 values, we end up with 2^M stdlibs that we have to ship. These are some of the options (from the top of my head) that we might want to ship:

• opt=debug/release
• debug-assert=true/false
• asan=true/false
• msan=true/false
• tsan=true/false
• lsan=true/false

which makes it 2^6 = 64 different stdlibs that we might want to ship... and the list is probably woefully incomplete... (e.g. target-feature=sse2 / sse4.2 / avx2 / ...).

Honestly, we need something better. In my opinion, the options that a user specifies for its crate should apply to the whole dependency graph, including the stdlib which should just be compiled as a normal rust crate. So ideally, instead of shipping any stdlib binaries, we would just ship the stdlib sources. If the std lib takes too long to build for some users, we can just refactor it into smaller crates.

[andrewhickman]

I disagree that code that, for example, calls get_unchecked with an index out of range is necessarily incorrect. Half of the use case for these functions is when the invariant ("this memory is actually valid") is enforced in a different way. This change would only make sense for the other use case (performance). Encouraging people to go through raw pointer methods in the name of safety feels counterproductive, since the method call is far more readable.

This absolutely makes sense for unreachable_unchecked since the invariant can be checked with no 'false positives'

[newpavlov]

The idea is to explicitly construct slice which will cover valid memory region before indexing it, e.g. Vec code can have an unsafe method get_cap_slice:

// method is unsafe because slice can contain uninitialized data
unsafe fn get_cap_slice(&self) -> &[T] {
    slice::from_raw_parts(self.buf.ptr, self.buf.cap)
}

Yes, this change will require more steps for some use-cases, but arguably it's worth it, as many consider get_unchecked as an escape hatch for additional performance and use it accordingly without any tools at hand to check if used assumptions are indeed correct.

[PaulGrandperrin]

Hi, I just want to say that I came across the same issue:
PaulGrandperrin/playground@b37152b

and I was thinking, wouldn't it be possible to do something like that for unreachable_unchecked in the std?:

#[macro_export]
macro_rules! unreachable_unchecked {
    () => ({
        #[cfg(debug_assertions)]
        panic!("internal error: entered unreachable code")
        #[cfg(not(debug_assertions))]
        std::hint::unreachable_unchecked()
    });
    ($msg:expr) => ({
        unreachable_unchecked!("{}", $msg)
    });
    ($msg:expr,) => ({
        unreachable_unchecked!($msg)
    });
    ($fmt:expr, $($arg:tt)*) => ({
        #[cfg(debug_assertions)]
        panic!(concat!("internal error: entered unreachable code: ", $fmt), $($arg)*)
        #[cfg(not(debug_assertions))]
        std::hint::unreachable_unchecked()
    });
}

As a macro, it would be expanded at crate compilation time and so would respect the debug_assertion flag.

(this code is inspired by the unreachable! macro source code in the std: https://doc.rust-lang.org/src/core/macros.rs.html#485-498)

[sfackler]

As a macro, it would be expanded at crate compilation time and so would respect the debug_assertion flag.

"crate compilation time" is the time that you're compiling std, not the downstream crate.

[RalfJung]

Cc #53871 #54915

[RalfJung]

It seems like the checks we already have already cause a very notable slowdown when building rustc with debug assertions enabled. We mitigated this a bit by making some checks abort instead of panic on failure (avoiding unwind paths), but this still still something to be aware of.

It might be a good idea to have a separate cfg flag for checks that would run a lot, like the ones we already have in copy/copy_nonoverlapping/write (and potentially more) or the ones that this issue proposes.

[newpavlov]

@RalfJung
Considering that debug asserts are usually enabled only for debug builds with low optimization level, shouldn't this slowdown be acceptable?

[RalfJung]

It is quite common for compiler contributors to enable debug assertions in the compiler (this helps catch many bugs). This also enables debug assertions in libstd which makes the generated compiler a lot slower.

With #72146 we now have the option of only enabling debug assertions in rustc and not libstd, but contributors still have to know to do that. And the more people do this, the less coverage we have for these debug assertions...

[saethlin]

I think we now have answers to the two main blockers. Just commenting here because I haven't seen these answers mentioned.

stdlib is distributed with disabled debug assertions...

These assertions would still be useful for developers, because Cargo has -Zbuild-std. You have to know to turn this on which is less than awesome, but IMO that's not a huge leap because developers of unsafe code should be using other opt-in tools such as ASan, Miri, and fuzzers.

Perhaps if there is any concern left here, it is that just adding -Zbuild-std has a surprising effect, and users may keep asking for the checks to be defaulted?

Some code in the stdlib (and probably in some external crates) consciously breaks those restrictions (e.g. src/liballoc/vec.rs). Probably it should be rewritten in a more "correct" fashion.

core and alloc are unlikely to trip any suggested assertion on their own, I'm pretty sure their test suites now pass Miri's Stacked Borrows checker. std is perhaps a different matter, but I didn't see any suggestions for assertions in that crate. I have absolutely encountered third-party crates that expect to be able to index past the end of a slice with get_unchecked, and that has been clearly documented to be UB for 3 years now. Btw, it's unclear to me if the examples I've seen are people consciously breaking the rules.

[saethlin]

Maybe I deserve this for commenting so quickly after learning about this issue, but it seems that while std/alloc do not rely on indexing out of bounds, the compiler does.

[RalfJung]

FWIW, with https://github.com/RalfJung/cargo-careful it becomes fairly easy for people to run their code with a stdlib that has debug assertions, so these assertions should be much easier to apply to real-world code now. :)

[pitaj]

So it looks like a lot of debug assertions were added in #92686. Looking through various cases, it appears that the following from the list above are covered by some form of debug assertion:

• slice::get_unchecked(_mut)
• unreachable_unchecked
• unwrap_unchecked
• new_unchecked

Some notable missing cases I found:

• str::get_unchecked(_mut)
• Arc/Rc::get_mut_unchecked
• from_utf8_unchecked
• slice::split_at_unchecked (but split_at_mut_unchecked has a debug assertion)

I've opened #105117 to address these.

Also there are a lot of number-related unchecked operations that's aren't covered:

• unchecked math operations
• to_int_unchecked
• forward/backward_unchecked
• unchecked SIMD-related operations

But I'm not sure if adding debug assertions for those would be welcomed.

[saethlin]

But I'm not sure if adding debug assertions for those would be welcomed.

My not-libs opinion is that we should have some way to make all of these checked. Debug assertions might not be the best switch for these, but keeping them all behind assert_unsafe_precondition! makes it possible to swap out for some custom --cfg or something else.

And just in case this is what you're worried about: I do not think runtime overhead is a good argument against adding more of these. We started with things like copy_nonoverlapping and slice::get_unchecked where the overhead in runtime or compile time is highest.

[newpavlov]

I think this issue can be closed in favor of #120848?

[RalfJung]

Yes, that list is much more up-to-date than the one here.