Potential unsoundness

[Nugine]

There is no guarantee that the safety requirements are met here.

https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#safety

async-rdma/src/memory_region/local.rs

Lines 15 to 19 in 97fad33

	/// Get the memory region as slice
	#[inline]
	fn as_slice(&self) -> &[u8] {
	unsafe { slice::from_raw_parts(self.as_ptr(), self.length()) }
	}

async-rdma/src/memory_region/local.rs

Lines 34 to 38 in 97fad33

	/// Get the memory region as mut slice
	#[inline]
	fn as_mut_slice(&mut self) -> &mut [u8] {
	unsafe { slice::from_raw_parts_mut(self.as_mut_ptr(), self.length()) }
	}

[Nugine]

Alignment mismatch

#[test]
fn align_check() {
    use std::mem::align_of;
    assert_eq!(align_of::<Gid>(), align_of::<ibv_gid>());
}

running 1 test
thread 'gid::align_check' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `8`', src/gid.rs:68:5

async-rdma/src/gid.rs

Lines 45 to 63 in 9a32a81

	impl AsRef<ibv_gid> for Gid {
	fn as_ref(&self) -> &ibv_gid {
	// alignment is guaranteed
	#[allow(clippy::cast_ptr_alignment)]
	unsafe {
	&*self.raw.as_ptr().cast::<ibv_gid>()
	}
	}
	}
	impl AsMut<ibv_gid> for Gid {
	fn as_mut(&mut self) -> &mut ibv_gid {
	// alignment is guaranteed
	#[allow(clippy::cast_ptr_alignment)]
	unsafe {
	&mut *self.raw.as_mut_ptr().cast::<ibv_gid>()
	}
	}
	}

[rogercloud]

There is no guarantee that the safety requirements are met here.

https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#safety

async-rdma/src/memory_region/local.rs

Lines 15 to 19 in 97fad33

	/// Get the memory region as slice
	#[inline]
	fn as_slice(&self) -> &[u8] {
	unsafe { slice::from_raw_parts(self.as_ptr(), self.length()) }
	}

async-rdma/src/memory_region/local.rs

Lines 34 to 38 in 97fad33

	/// Get the memory region as mut slice
	#[inline]
	fn as_mut_slice(&mut self) -> &mut [u8] {
	unsafe { slice::from_raw_parts_mut(self.as_mut_ptr(), self.length()) }
	}

These two trait are crate pub traits, the usage is limited in the crates, so the discussion boundry is limited.

Safety cares the following points:

1. The memory is aligned properly and the memory region is available for read. The alignment of u8 is natrually met, and the read permission is garanteed in all the implementations.
2. The content is properly initialized, for u8 type the requirement is meaningless, any value is valid.
3. The mutation property is considered, we can only get one mutable reference. The property is guarded by the Rust compiler.
4. The length of the memory region should not exceed isize::MAX. We're not checking it correctly. It's a good catch

[rogercloud]

Alignment mismatch

#[test]
fn align_check() {
    use std::mem::align_of;
    assert_eq!(align_of::<Gid>(), align_of::<ibv_gid>());
}

running 1 test
thread 'gid::align_check' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `8`', src/gid.rs:68:5

async-rdma/src/gid.rs

Lines 45 to 63 in 9a32a81

	impl AsRef<ibv_gid> for Gid {
	fn as_ref(&self) -> &ibv_gid {
	// alignment is guaranteed
	#[allow(clippy::cast_ptr_alignment)]
	unsafe {
	&*self.raw.as_ptr().cast::<ibv_gid>()
	}
	}
	}
	impl AsMut<ibv_gid> for Gid {
	fn as_mut(&mut self) -> &mut ibv_gid {
	// alignment is guaranteed
	#[allow(clippy::cast_ptr_alignment)]
	unsafe {
	&mut *self.raw.as_mut_ptr().cast::<ibv_gid>()
	}
	}
	}

It's a good catch. We'll add more alignment constrain on Gid struct.

[Nugine]

@rogercloud

These two trait are crate pub traits, the usage is limited in the crates,

The traits are parts of the public api in v0.2.0. They should be marked as sealed traits or unsafe traits.

https://docs.rs/async-rdma/0.2.0/async_rdma/index.html#traits

The content is properly initialized, for u8 type the requirement is meaningless, any value is valid.

The trait itself can not ensure this. Even raw bytes must be initialized before reading.

Example:
Click Tools -> Miri, it reports undefined behavior.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5d09005a17d00ab33ff9d86ce8844de6

Related comment: #49 (comment)

the memory region is available for read

The as_ptr method in the trait does not ensure that the returned pointer is valid.

[rogercloud]

@Nugine

I think we should mark the trait as sealed traits, so that they can't be implemented by the users.

For u8 type initialization means setting zero, which is inefficient for a large memory block. Let's provide two types of APIs, one for fast and unsafe requirements, the other for zeroed and safe requirements.

Todo:

• mark the traits sealed
• provide two apis, safe and unsafe one.
• change Gid alignment.

[Nugine]

@rogercloud

What about sealed unsafe traits?

[rogercloud]

Yes of course, if it contains unsafe interface, it's unsafe.

[Nugine]

Exporting uninitialized memory
#49 (comment)

async-rdma/src/mr_allocator.rs

Lines 108 to 132 in ab99fa3

	/// Allocate a MR according to the `layout`
	#[allow(clippy::as_conversions)]
	pub(crate) fn alloc(self: &Arc<Self>, layout: &Layout) -> io::Result<LocalMr> {
	self.alloc_from_je(layout).map_or_else(||{
	Err(io::Error::new(io::ErrorKind::OutOfMemory, "insufficient contiguous memory was available to service the allocation request"))
	}, |addr|{
	let raw_mr = self.lookup_raw_mr(addr as usize);
	Ok(LocalMr::new(addr as usize, layout.size(), raw_mr))
	})
	}
	/// Alloc memory for RDMA operations from jemalloc
	fn alloc_from_je(&self, layout: &Layout) -> Option<*mut u8> {
	let addr = unsafe {
	tikv_jemalloc_sys::mallocx(
	layout.size(),
	(MALLOCX_ALIGN(layout.align()) | MALLOCX_ARENA(self.arena_ind.cast())).cast(),
	)
	};
	if addr.is_null() {
	None
	} else {
	Some(addr.cast::<u8>())
	}
	}