Unsound implementation in MemoryRange::as_slice/as_slice_mut

[shinmao]

The source of unsoundness

xous-core/xous-rs/src/definitions.rs

Lines 318 to 324 in f17ce55

	pub fn as_slice<T>(&self) -> &[T] {
	// This is safe because the pointer and length are guaranteed to
	// be valid, as long as the user hasn't already called `as_ptr()`
	// and done something unsound with the resulting pointer.
	unsafe {
	core::slice::from_raw_parts(
	self.as_ptr() as *const T,

We consider as_slice unsound because: at line 324, the pointer with any bit patterns could be cast to the slice of arbitrary types. The pointer could be created by unsafe new and deprecated from_parts. We consider that from_parts should be removed in latest version because it will help trigger unsoundness in as_slice. With new declared as unsafe, as_slice should also declared as unsafe.

To reproduce the bug

use xous::definitions::MemoryRange;

fn main() {
    let a: u64 = 3;
    let addr = &a as *const u64 as usize;
    let mr = unsafe { MemoryRange::new(addr, 8usize).unwrap() };
    println!("{:?}", mr.as_slice::<bool>());
}

to run with miri,

note: inside `main`
   --> src/main.rs:7:22
    |
7   |     println!("{:?}", mr.as_slice::<bool>());
    |                      ^^^^^^^^^^^^^^^^^^^^^

error: Undefined Behavior: constructing invalid value: encountered 0x03, but expected a boolean

[xobs]

Nobody seems to be using from_parts(), so I agree it makes sense to remove it.

The call to as_slice() (and as_slice_mut()) is a bit more problematic. I'll put together a patchset and have @bunnie take a look at it.

[shinmao]

@xobs yes. I think we can remind the user who combines the usage of new and as_slice should keep in mind that T actually can't be any type they provide.

[xobs]

I think that a better API would have been to turn a MemoryRange into a smart pointer of some sort that encodes the type, and make only the constructor be unsafe. Unfortunately it's a bit too late to do that in this API -- it will have to be done for some Xous-rs 2.0 API.

[xobs]

I created #411 to address this issue.

[bunnie]

this got merged, so i think we can close this issue?