Conversation
MaxOhn
left a comment
There was a problem hiding this comment.
Haven't checked read_struct_ptr_array yet but got some feedback for the other two methods.
src/memory/process.rs
Outdated
| let mut buff = vec![0u8; size_of::<T>()]; | ||
|
|
||
| let byte_buff = unsafe { | ||
| std::slice::from_raw_parts_mut( | ||
| buff.as_mut_ptr() as *mut u8, | ||
| buff.len() | ||
| ) | ||
| }; | ||
|
|
||
| self.read(addr, byte_buff.len(), byte_buff)?; | ||
|
|
||
| let s: T = unsafe { std::ptr::read(buff.as_ptr() as *const _) }; | ||
|
|
||
| Ok(s) |
There was a problem hiding this comment.
The heap allocation through the Vec is unfortunate but what's worse is that the Vec's data might not be aligned properly w.r.t. T. Both should be fixed by this:
let mut uninit = std::mem::MaybeUninit::<T>::uninit();
let ptr: *mut u8 = uninit.as_mut_ptr().cast();
let slice = unsafe { std::slice::from_raw_parts_mut(ptr, std::mem::size_of::<T>()) };
self.read(addr, slice.len(), slice)?;
Ok(unsafe { uninit.assume_init() })Might be a good idea to run miri over it to make sure the unsafe code is valid.
Marking the method itself as unsafe might be warranted too since the unsafety invariant basically requires the generic T to be repr(C) which cannot be specified by trait bounds to the caller will have to promise.
src/memory/process.rs
Outdated
| let size = size_of::<T>() + align_of::<T>(); | ||
| let mut buff = vec![0u8; size * len]; | ||
|
|
||
| let mut byte_buff = unsafe { | ||
| std::slice::from_raw_parts_mut( | ||
| buff.as_mut_ptr() as *mut u8, | ||
| buff.len() | ||
| ) | ||
| }; | ||
|
|
||
| self.read(addr, byte_buff.len(), byte_buff)?; | ||
|
|
||
| let mut arr = Vec::with_capacity(len); | ||
|
|
||
| while byte_buff.len() >= size_of::<T>() { | ||
| let (head, tail) = byte_buff.split_at_mut(size); | ||
| let s: T = unsafe { std::ptr::read(head.as_ptr() as *const T) }; | ||
| arr.push(s); | ||
| byte_buff = tail; | ||
| } | ||
|
|
||
| Ok(arr) |
There was a problem hiding this comment.
Same alignment and heap allocation issue here. Could be replaced with this:
let mut buff: Vec<_> = std::iter::repeat_with(std::mem::MaybeUninit::<T>::uninit)
.take(len)
.collect();
let ptr: *mut u8 = buff.as_mut_ptr().cast();
let slice = unsafe { std::slice::from_raw_parts_mut(ptr, size_of::<T>() * len) };
self.read(addr, slice.len(), slice)?;
let ptr: *mut T = buff.as_mut_ptr().cast();
let len = buff.len();
let cap = buff.capacity();
std::mem::forget(buff);
Ok(unsafe { Vec::from_raw_parts(ptr, len, cap) })Again, miri could help ensuring there's nothing wrong with the unsafety and marking the whole method as unsafe might be in order.
Pretty sure this could be optimized even more
Draft until a legitimate use case is present