Closed
Description
Description
Vec::set_len documents a couple of safety requirements, including "The elements at old_len..new_len must be initialized." This is technically violated in a couple of places when inserting after holes:
- https://github.com/slide-rs/specs/blob/5c02b760431d88f8f8629bbf173ee3f823f70008/src/storage/storages.rs#L113-L117
- https://github.com/slide-rs/specs/blob/5c02b760431d88f8f8629bbf173ee3f823f70008/src/storage/storages.rs#L211-L215
I'm not entirely sure how bad these violations of the stdlib preconditions are in practice, but they could be resolved by switching these storage impls to use MaybeUninit under the hood, and possibly regular resize fns.
Motivation
Paranoia about UB. As MaybeUninit implies ManuallyDrop, this would also make things a little more drop friendly in the case of VecStorage:
use specs::{VecStorage, storage::UnprotectedStorage};
use hibitset::BitSet;
fn main() {
let mut storage = VecStorage::default();
unsafe {
storage.insert(3, String::from("asdf"));
storage.remove(3);
panic!("Totally unexpected panic!");
storage.clean(BitSet::new());
};
// Because we didn't reach storage.clean(...), dropping storage also drops
// Strings at 0, 1, 2, despite no such strings ever coming into existence.
// It also re-drops string 3. Heap corruption - or worse - ensues.
}C:\local\specs-test-case>cargo run
Compiling specs-test-case v0.1.0 (C:\local\specs-test-case)
warning: unreachable statement --> src\main.rs:10:9
|
10 | storage.clean(BitSet::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: #[warn(unreachable_code)] on by default
Finished dev [unoptimized + debuginfo] target(s) in 1.00s
Running `target\debug\specs-test-case.exe`
thread 'main' panicked at 'Totally unexpected panic!', src\main.rs:9:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: process didn't exit successfully: `target\debug\specs-test-case.exe`
(exit code: 0xc0000374, STATUS_HEAP_CORRUPTION)Removing the panic and clean you can also potentially run:
C:\local\specs-test-case>rustup install nightly-2019-09-11
...
C:\local\specs-test-case>cargo +nightly-2019-09-11 miri run
...
error[E0080]: Miri evaluation error: type validation failed: encountered uninitialized bytes, but expected something greater or equal to 1
--> C:\Users\Mike\.rustup\toolchains\nightly-2019-09-11-x86_64-pc-windows-msvc\lib\rustlib\src\rust\src\liballoc\raw_vec.rs:200:9
|
200 | self.ptr.as_ptr()
| ^^^^^^^^ Miri evaluation error: type validation failed: encountered uninitialized bytes, but expected something greater or equal to 1
...
Drawbacks
- Is it a breaking change? Minimum rust would be bumped to 1.36.0, otherwise no.
- Can it impact performance, learnability, etc? Maybe performance. To be benchmarked?
Unresolved questions
- Am I overreacting? Nobody so far has seem too concerned about uninitialized T in this specific case of "construction" via Vec if T goes completely unaccessed.
- Does
set_lenbecome "sane" in the presence of MaybeUninit, or wouldv.resize(n, MaybeUninit::uninit())be required?
I'd be willing to implement this, but my profiling-fu for specs is weak.