Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
split-debuginfo = "unpacked"
lto = "thin"

# Enable basic optimizations for unittests
[profile.test]
opt-level = 1

# Enable optimizations for procmacros for faster recompile
[profile.dev.build-override]
opt-level = 1

[workspace]
members = [
"account",
Expand Down
77 changes: 56 additions & 21 deletions program-entrypoint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,18 +281,61 @@ macro_rules! custom_panic_default {

/// The bump allocator used as the default rust heap when running programs.
pub struct BumpAllocator {
#[deprecated(
since = "2.2.2",
note = "This field should not be accessed directly. It will become private in future versions"
)]
pub start: usize,
#[deprecated(
since = "2.2.2",
note = "This field should not be accessed directly. It will become private in future versions"
)]
pub len: usize,
}

impl BumpAllocator {
/// Creates the allocator tied to a provided slice.
/// This will not initialize the provided memory, except for the first
/// bytes where the pointer is stored.
///
/// # Safety
/// As long as BumpAllocator or any of its allocations are alive,
/// writing into or deallocating the arena will cause UB.
///
/// Integer arithmetic in this global allocator implementation is safe when
/// operating on the prescribed `HEAP_START_ADDRESS` and `HEAP_LENGTH`. Any
/// other use may overflow and is thus unsupported and at one's own risk.
#[inline]
#[allow(clippy::arithmetic_side_effects)]
pub unsafe fn new(arena: &mut [u8]) -> Self {
debug_assert!(
arena.len() > size_of::<usize>(),
"Arena should be larger than usize"
);

// create a pointer to the start of the arena
// that will hold an address of the byte following free space
let pos_ptr = arena.as_mut_ptr() as *mut usize;
// initialize the data there
*pos_ptr = pos_ptr as usize + arena.len();
Comment on lines +316 to +320
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good, but for my own understanding, how does this avoid undefined behavior exactly?

Is the idea that the new call forces the optimizer to keep heap around in the tests because some part of it is written to? Would there still be UB if this code here were removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn new() correctly initializes the arena content where the allocated amount is, which was not done before, as the code assumed all arenas come zeroed. If you were to instantiate the BumpAllocator over a non-zeroed slice, it would cause UB the moment you call alloc().

The reason bug UB was triggered before this change was due to heap variable not marked as mut, so compiler assumed that it does not need to reinitialize it (since noone would write into it).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah very interesting, thanks for the explanation!

Comment on lines +310 to +320
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a potential alignment issue here.
arena: &mut [u8] is not guaranteed to have proper alignment for it to be cast to a pointer to a usize.
A [u8] can have any alignment, while a pointer to usize needs to have an alignment of 4 or higher.

You can use write_unaligned() to avoid the undefined behavior.
write_unaligned() example is almost literary the code you are writing here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write unaligned could work, yes, but I think to properly address this, the entire BumpAllocator should be rewritten to use some form of interior mutability in the struct itself rather than storing the position pointer inline with the allocated data. It would make code a lot cleaner too. I will make a PR for that.


#[allow(deprecated)] //we get to use deprecated pub fields
Self {
start: pos_ptr as usize,
len: arena.len(),
}
}
}

/// Integer arithmetic in this global allocator implementation is safe when
/// operating on the prescribed `HEAP_START_ADDRESS` and `HEAP_LENGTH`. Any
/// other use may overflow and is thus unsupported and at one's own risk.
#[allow(clippy::arithmetic_side_effects)]
unsafe impl std::alloc::GlobalAlloc for BumpAllocator {
#[inline]
#[allow(deprecated)] //we get to use deprecated pub fields
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
let pos_ptr = self.start as *mut usize;

let mut pos = *pos_ptr;
if pos == 0 {
// First time, set starting position
Expand Down Expand Up @@ -513,31 +556,22 @@ mod test {
fn test_bump_allocator() {
// alloc the entire
{
let heap = [0u8; 128];
let allocator = BumpAllocator {
start: heap.as_ptr() as *const _ as usize,
len: heap.len(),
};
let mut heap = [0u8; 128];
let allocator = unsafe { BumpAllocator::new(&mut heap) };
for i in 0..128 - size_of::<*mut u8>() {
let ptr = unsafe {
allocator.alloc(Layout::from_size_align(1, size_of::<u8>()).unwrap())
};
assert_eq!(
ptr as *const _ as usize,
heap.as_ptr() as *const _ as usize + heap.len() - 1 - i
);
assert_eq!(ptr as usize, heap.as_ptr() as usize + heap.len() - 1 - i);
}
assert_eq!(null_mut(), unsafe {
allocator.alloc(Layout::from_size_align(1, 1).unwrap())
});
}
// check alignment
{
let heap = [0u8; 128];
let allocator = BumpAllocator {
start: heap.as_ptr() as *const _ as usize,
len: heap.len(),
};
let mut heap = [0u8; 128];
let allocator = unsafe { BumpAllocator::new(&mut heap) };
let ptr =
unsafe { allocator.alloc(Layout::from_size_align(1, size_of::<u8>()).unwrap()) };
assert_eq!(0, ptr.align_offset(size_of::<u8>()));
Expand All @@ -558,13 +592,14 @@ mod test {
}
// alloc entire block (minus the pos ptr)
{
let heap = [0u8; 128];
let allocator = BumpAllocator {
start: heap.as_ptr() as *const _ as usize,
len: heap.len(),
let mut heap = [0u8; 128];
let allocator = unsafe { BumpAllocator::new(&mut heap) };
let ptr = unsafe {
allocator.alloc(
Layout::from_size_align(heap.len() - size_of::<usize>(), size_of::<u8>())
.unwrap(),
)
};
let ptr =
unsafe { allocator.alloc(Layout::from_size_align(120, size_of::<u8>()).unwrap()) };
assert_ne!(ptr, null_mut());
assert_eq!(0, ptr.align_offset(size_of::<u64>()));
}
Expand Down