fix(guest-bin): align user memory allocations #753
Conversation
andreiltd
requested review from
danbugs,
ludfjig,
dblnz,
devigned,
syntactically,
marosset,
jprendes and
simongdavies
as code owners
andreiltd
force-pushed
the
fix-guest-align-mem
branch
from
7aee0e0 to
b81669d
Compare
andreiltd
added
the
kind/enhancement
|
|
||
| ---------------------------------------------------------------------------------------- | ||
| | Layout { size: 64 + size_of::<Layout>(), ... } | 64 bytes of memory | ... | ||
| | allocated memory... | Header { layout, offset } | 64 bytes of memory | ... |
There was a problem hiding this comment.
It might be nicer to say something like "padding memory" instead of "allocated memory".
Are we guaranteed to be respecting the alignment of the Header structure itself here? I guess we are as long as the Header size is a multiple of its alignment, which is a Rust invariant?
There was a problem hiding this comment.
Yes, because the actual alignment is: alignment.max(align_of::<Header>()) which guarantee we can place header just before user ptr safely.
|
|
||
| The header contains: | ||
| - layout: The Layout used for the entire allocation (total_size, actual_align) | ||
| - offset: The offset from raw_ptr to the user data (data_offset) |
There was a problem hiding this comment.
Is keeping the offset around necessary? Can't it be computed from the alignment in the Layout?
There was a problem hiding this comment.
I rewrote this to recalculate the offset instead in free and realloc.
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn aligned_alloc(alignment: usize, size: usize) -> *mut c_void { | ||
| // Validate alignment | ||
| if alignment == 0 || (alignment & (alignment - 1)) != 0 { |
There was a problem hiding this comment.
Maybe add a comment on alloc_helper detailing this invariant.
| @@ -105,12 +139,20 @@ pub unsafe extern "C" fn calloc(nmemb: usize, size: usize) -> *mut c_void { | |||
| /// `ptr` must be a pointer to a memory block previously allocated by `memory::malloc`, `memory::calloc`, or `memory::realloc`. | |||
| #[unsafe(no_mangle)] | |||
| pub unsafe extern "C" fn free(ptr: *mut c_void) { | |||
| if !ptr.is_null() { | |||
| unsafe { | |||
| let block_start = (ptr as *const Layout).sub(1); | |||
There was a problem hiding this comment.
FWIW I think this idiom is safe to continue using as both (a) raw pointers can be unaligned and (b) size is always supposed to be a multiple of alignment. Feel free to ignore this comment if you had a different reason for preferring the other idiom.
|
|
||
| unsafe { | ||
| let raw_ptr = match zero { | ||
| true => alloc::alloc::alloc_zeroed(layout), | ||
| false => alloc::alloc::alloc(layout), | ||
| }; | ||
|
|
||
| if raw_ptr.is_null() { | ||
| abort_with_code(&[ErrorCode::MallocFailed as u8]); |
There was a problem hiding this comment.
I know it's not part of your changes, but should we about here, or return null?
The aligned_alloc method returns null is the alignment is wrong. Should that also abort?
There was a problem hiding this comment.
I would say aborting is more useful and probably "safer" in memory constrained env, in the same time if we want to be somewhat posix compatible we should return null and set errno to ENOMEM. 🤷
| let Header { layout, offset } = header_ptr.read(); | ||
|
|
||
| // Deallocate from the original base pointer | ||
| let raw_ptr = user_ptr.sub(offset) as *mut u8; |
There was a problem hiding this comment.
could the header just store the raw_ptr instead of the offset?
There was a problem hiding this comment.
I think you can recompute that just from the layout, so I would be in favour of still only storing the layout I think?
There was a problem hiding this comment.
Yes, I rewrote this to recalculate the offset on free.
andreiltd
force-pushed
the
fix-guest-align-mem
branch
from
b81669d to
f3efbb5
Compare
The guest allocation wrapper aligned only the total memory block but did not guarantee that the pointer returned to the user was itself properly aligned. This patch attempts to fix that. It also adds alloc_aligned function which would be tricky to implement outside the guest but will make implementing other posix like allocation API much easier. Signed-off-by: Tomasz Andrzejak <[email protected]>
andreiltd
force-pushed
the
fix-guest-align-mem
branch
from
f3efbb5 to
cc20014
Compare
Signed-off-by: Tomasz Andrzejak <[email protected]>
andreiltd
force-pushed
the
fix-guest-align-mem
branch
from
0e3f72e to
c623979
Compare
The guest allocation wrapper aligned only the total memory block but did not guarantee that the pointer returned to the user was itself properly aligned. This patch attempts to fix that.
It also adds alloc_aligned function which would be tricky to implement outside the guest but will make implementing other posix like allocation API much easier.