I don’t know if this idea was proposed previously, but I think it can be useful to add debug assertion which will check bound for get_unchecked, get_unchecked_mut, slice_unchecked, slice_mut_unchecked methods (btw shouldn’t we consider deprecation of the last two?) and maybe some other similar methods in the stdlib. It will allow us to easier catch potentially dangerous bugs (e.g. by fuzzing with enabled debug assertions or simply on tests) without hurting performance of release builds. Although one can argue that strictly speaking this change can be considered breaking. What do you think?
7 Likes
Someone will correct me if I’m wrong, but since indexing past a slice is undefined, I don’t think anyone would object to making it fail sooner & safer in debug mode.
That said, as an example of incorrect code that works:
let slice1 = &[0, 1, 2, 3, 4, 5, 6, 7, 8, 9][..];
let slice2 = &slice1[..5];
assert_eq!(slice2.get_unchecked(9), 9);
This can be “made correct” by casting the slice to the proper length that you know is actually there. This is actually somewhat common in parsing applications: you keep a slice of what this node covers but also an offset to the start of the buffer such that you can look at everything before to find the line/column late instead of eagerly. Since that’s going backwards instead of forwards, though, 100% of them correctly create the slice instead of just indexing past one they already have.
Another wrinkle is that debug_assert! in the stdlib basically never runs, as the stdlib is always compiled in optimized mode. This would have to hook into the existing (somewhat hacky workaround) plumbing that enables overflow checks in debug everywhere (or something similar).
However, this could be done exclusively in a crate if you’re ok using different method names: (pseudo-rust for simplicity)
impl DebugAssertSlicing for &[T] {
fn get_debug_checked(&self, idx: usize) -> Self::Item {
if cfg!(debug_assertions) {
self[idx]
} else {
self.get_unchecked(idx)
}
}
// and so on
}
3 Likes
Yeah, debug_assert! problem will be the main issue while implementing proposed behaviour.
As for “incorrect code that works” same can be said about integer overflow, so I think it will be a good incentive to write code more “correctly”.
It would be helpful.
Perhaps, as a precaution, there could be another “level” of debug assertions? e.g. debug_assertions = true working as it is today, but debug_assertions = 2 enabling the extra ones.
I can think of more checks for “unchecked” things, e.g. str::from_utf8_unchecked, or checks that &/&mut created from casts in unsafe blocks are actually non-NULL.
This reminds me of sanitizers, in a way. Maybe there could be a "rustsan" that checks these extra things...
I don’t have a link handy, but I recall this assertion being proposed before and it turned out quite a bit of unsafe code, including in the standard library, is using get_unchecked_mut on vectors with indices larger than the length (but smaller than the capacity, and being careful not to read uninitialized memory). Search src/liballoc/vec.rs for some examples. While that is somewhat iffy and might technically be UB under the Unsafe Code Guidelines eventually adopted, so far nobody’s put in the work and convincing necessary to change that.
2 Likes
The main blockers here are that 1) we never ship std with debug-assertions = true, and 2) cargo build should pick up the std library compiled with debug-assertions = true for debug builds, and the one with them set to false for release builds, and also do all of this depending on the debug-assertions value of the profile.
I think we should solve these two issues first, and then adding a whole bunch of debug_asserts to std might make much more sense. Right now one needs to recompile std with xargo to benefit from any of these, which is a pretty high barrier of entry in my opinion.
I suggested debug assertions on get_unchecked and get_unchecked_mut back in this thead and was quickly shot down by the fact that… yeah, people do this.
Of note there is bluss’ current solution: https://crates.io/crates/unchecked-index
2 Likes
This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.