Fixes for allocator + factory + misc improvements (#3534)

* Clear up import/export misunderstandings * Fetch minimum period from runtime * Remove unnecessary comment This variable is already fetched from the runtime in the line below. * Fix bug in factory The `best_block_id` stayed the same, it was always the genesis hash. This resulted in the factory failing after 4096 blocks, since `client/db` discards hashes (in this case the genesis hash) after 4096 blocks from the database. * Fix tense in error message * Improve allocator documentation * Fix bug in allocator Under certain circumstances an invalid pointer was returned: when the `ptr` was calculated as equal to the `max_heap_size`. This is an invalid pointer since there is no access allowed after the heap limit. The way to provoke this was to repeatedly allocate with sizes which were previously not allocated and immediately deallocate right afterwards. What this did was to increment the `bumper` with each allocation, whilst keeping the `total_size` of the heap `0`. If this repeated allocation/deallocation scheme resulted in `max_heap_size == ptr` the `ptr` was still returned. The allocator only checked if the `total_size` was still within the `max_heap_size` limits, and not if the resulting `ptr` was still within the valid heap region. This commit introduces a check to validate if the calculated `ptr` is within the heap. * Add test for zero byte allocation and document behavior * Improve code readability by introducing a const * Fix error message in test * Apply suggestions from code review Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Fix code review suggestions * Replace early return with assertion * Remove test for zero size allocations * Shorten test code * Shorten comment * Make bump() return Result * Add comment for bump() * Remove ambiguous comment * Replace value with const * Use proof for panic message * Fix merge * Add comment regarding minimum allocation size
2026-04-29 16:07:57 +00:00 · 2019-09-13 15:47:15 +02:00
parent b7c6bc1ed5
commit 5cb8c0dc1c
5 changed files with 147 additions and 62 deletions
@@ -15,14 +15,45 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.

 //! This module implements a freeing-bump allocator.
-//! See more details at https://github.com/paritytech/substrate/issues/1615.
+//!
+//! The algorithm is as follows:
+//! We store `N` linked list heads, where `N` is the total number of sizes
+//! of allocations to support. A simple set is powers of two from 8 bytes
+//! to 16,777,216 bytes (2^3 - 2^24 inclusive), resulting in `N = 22`:
+//!
+//! ```ignore
+//!	let mut heads [u64; N] = [0; N];
+//! fn size(n: u64) -> u64 { 8 << n }
+//! let mut bumper = 0;
+//! fn bump(n: u64) -> u64 { let res = bumper; bumper += n; res }
+//! ```
+//!
+//! We assume there is a slab of heap to be allocated:
+//!
+//! ```ignore
+//! let mut heap = [0u8; HEAP_SIZE];
+//! ```
+//!
+//! Whenever we allocate, we select the lowest linked list item size that
+//! will fit the allocation (i.e. the next highest power of two).
+//! We then check to see if the linked list is empty. If empty, we use
+//! the bump allocator to get the allocation with an extra 8 bytes
+//! preceding it. We initialise those preceding 8 bytes to identify the
+//! list to which it belongs (e.g. `0x__ffffffffffffff` where `__` is the
+//! linked list index). If it is not empty, we unlink the first item from
+//! the linked list and then reset the 8 preceding bytes so they now record
+//! the identity of the linked list.
+//!
+//! To deallocate we use the preceding 8 bytes of the allocation to knit
+//! back the allocation into the linked list from the head.

 use crate::error::{Error, Result};
 use log::trace;
 use wasmi::{MemoryRef, memory_units::Bytes};
 use wasm_interface::{Pointer, WordSize};

-// The pointers need to be aligned to 8 bytes.
+// The pointers need to be aligned to 8 bytes. This is because the
+// maximum value type handled by wasm32 is u64.
 const ALIGNMENT: u32 = 8;

 // The pointer returned by `allocate()` needs to fulfill the alignment
@@ -32,6 +63,11 @@ const ALIGNMENT: u32 = 8;
 // index) and then a subsequent item of 2^x bytes, where x = [3..24].
 const N: usize = 22;
 const MAX_POSSIBLE_ALLOCATION: u32 = 16777216; // 2^24 bytes
+const MIN_POSSIBLE_ALLOCATION: u32 = 8;
+
+// Each pointer is prefixed with 8 bytes, which identify the list index
+// to which it belongs.
+const PREFIX_SIZE: u32 = 8;

 pub struct FreeingBumpHeapAllocator {
 	bumper: u32,
@@ -79,14 +115,17 @@ impl FreeingBumpHeapAllocator {

 	/// Gets requested number of bytes to allocate and returns a pointer.
 	/// The maximum size which can be allocated at once is 16 MiB.
+	/// There is no minimum size, but whatever size is passed into
+	/// this function is rounded to the next power of two. If the requested
+	/// size is below 8 bytes it will be rounded up to 8 bytes.
 	pub fn allocate(&mut self, size: WordSize) -> Result<Pointer<u8>> {
 		if size > MAX_POSSIBLE_ALLOCATION {
 			return Err(Error::RequestedAllocationTooLarge);
 		}

-		let size = size.max(8);
+		let size = size.max(MIN_POSSIBLE_ALLOCATION);
 		let item_size = size.next_power_of_two();
-		if item_size + 8 + self.total_size > self.max_heap_size {
+		if item_size + PREFIX_SIZE + self.total_size > self.max_heap_size {
 			return Err(Error::AllocatorOutOfSpace);
 		}

@@ -94,19 +133,27 @@ impl FreeingBumpHeapAllocator {
 		let ptr: u32 = if self.heads[list_index] != 0 {
 			// Something from the free list
 			let item = self.heads[list_index];
+			let ptr = item + PREFIX_SIZE;
+			assert!(
+				ptr + item_size <= self.max_heap_size,
+				"Pointer is looked up in list of free entries, into which
+				only valid values are inserted; qed"
+			);
+
 			let four_bytes = self.get_heap_4bytes(item)?;
 			self.heads[list_index] = Self::le_bytes_to_u32(four_bytes);
-			item + 8
+			ptr
 		} else {
 			// Nothing to be freed. Bump.
-			self.bump(item_size + 8) + 8
+			self.bump(item_size)? + PREFIX_SIZE
 		};

-		(1..8).try_for_each(|i| self.set_heap(ptr - i, 255))?;
+		// Reset prefix
+		(1..PREFIX_SIZE).try_for_each(|i| self.set_heap(ptr - i, 255))?;

-		self.set_heap(ptr - 8, list_index as u8)?;
+		self.set_heap(ptr - PREFIX_SIZE, list_index as u8)?;

-		self.total_size = self.total_size + item_size + 8;
+		self.total_size = self.total_size + item_size + PREFIX_SIZE;
 		trace!(target: "wasm-heap", "Heap size is {} bytes after allocation", self.total_size);

 		Ok(Pointer::new(self.ptr_offset + ptr))
@@ -115,31 +162,42 @@ impl FreeingBumpHeapAllocator {
 	/// Deallocates the space which was allocated for a pointer.
 	pub fn deallocate(&mut self, ptr: Pointer<u8>) -> Result<()> {
 		let ptr = u32::from(ptr) - self.ptr_offset;
-		if ptr < 8 {
+		if ptr < PREFIX_SIZE {
 			return Err(error("Invalid pointer for deallocation"));
 		}

-		let list_index = usize::from(self.get_heap_byte(ptr - 8)?);
-		(1..8).try_for_each(|i| self.get_heap_byte(ptr - i).map(|byte| assert!(byte == 255)))?;
+		let list_index = usize::from(self.get_heap_byte(ptr - PREFIX_SIZE)?);
+		(1..PREFIX_SIZE).try_for_each(|i|
+			self.get_heap_byte(ptr - i).map(|byte| assert!(byte == 255))
+		)?;
 		let tail = self.heads[list_index];
-		self.heads[list_index] = ptr - 8;
+		self.heads[list_index] = ptr - PREFIX_SIZE;

-		let mut slice = self.get_heap_4bytes(ptr - 8)?;
+		let mut slice = self.get_heap_4bytes(ptr - PREFIX_SIZE)?;
 		Self::write_u32_into_le_bytes(tail, &mut slice);
-		self.set_heap_4bytes(ptr - 8, slice)?;
+		self.set_heap_4bytes(ptr - PREFIX_SIZE, slice)?;

 		let item_size = Self::get_item_size_from_index(list_index);
-		self.total_size = self.total_size.checked_sub(item_size as u32 + 8)
+		self.total_size = self.total_size.checked_sub(item_size as u32 + PREFIX_SIZE)
 			.ok_or_else(|| error("Unable to subtract from total heap size without overflow"))?;
 		trace!(target: "wasm-heap", "Heap size is {} bytes after deallocation", self.total_size);

 		Ok(())
 	}

-	fn bump(&mut self, n: u32) -> u32 {
+	/// Increases the `bumper` by `item_size + PREFIX_SIZE`.
+	///
+	/// Returns the `bumper` from before the increase.
+	/// Returns an `Error::AllocatorOutOfSpace` if the operation
+	/// would exhaust the heap.
+	fn bump(&mut self, item_size: u32) -> Result<u32> {
+		if self.bumper + PREFIX_SIZE + item_size > self.max_heap_size {
+			return Err(Error::AllocatorOutOfSpace);
+		}
+
 		let res = self.bumper;
-		self.bumper += n;
-		res
+		self.bumper += item_size + PREFIX_SIZE;
+		Ok(res)
 	}

 	fn le_bytes_to_u32(arr: [u8; 4]) -> u32 {
@@ -199,7 +257,8 @@ mod tests {
 		let ptr = heap.allocate(1).unwrap();

 		// then
-		assert_eq!(ptr, to_pointer(8));
+		// returned pointer must start right after `PREFIX_SIZE`
+		assert_eq!(ptr, to_pointer(PREFIX_SIZE));
 	}

 	#[test]
@@ -230,14 +289,14 @@ mod tests {

 		// then
 		// a prefix of 8 bytes is prepended to each pointer
-		assert_eq!(ptr1, to_pointer(8));
+		assert_eq!(ptr1, to_pointer(PREFIX_SIZE));

 		// the prefix of 8 bytes + the content of ptr1 padded to the lowest possible
 		// item size of 8 bytes + the prefix of ptr1
 		assert_eq!(ptr2, to_pointer(24));

 		// ptr2 + its content of 16 bytes + the prefix of 8 bytes
-		assert_eq!(ptr3, to_pointer(24 + 16 + 8));
+		assert_eq!(ptr3, to_pointer(24 + 16 + PREFIX_SIZE));
 	}

 	#[test]
@@ -247,7 +306,7 @@ mod tests {
 		let mut heap = FreeingBumpHeapAllocator::new(mem, 0);
 		let ptr1 = heap.allocate(1).unwrap();
 		// the prefix of 8 bytes is prepended to the pointer
-		assert_eq!(ptr1, to_pointer(8));
+		assert_eq!(ptr1, to_pointer(PREFIX_SIZE));

 		let ptr2 = heap.allocate(1).unwrap();
 		// the prefix of 8 bytes + the content of ptr 1 is prepended to the pointer
@@ -259,7 +318,7 @@ mod tests {
 		// then
 		// then the heads table should contain a pointer to the
 		// prefix of ptr2 in the leftmost entry
-		assert_eq!(heap.heads[0], u32::from(ptr2) - 8);
+		assert_eq!(heap.heads[0], u32::from(ptr2) - PREFIX_SIZE);
 	}

 	#[test]
@@ -271,13 +330,13 @@ mod tests {

 		let ptr1 = heap.allocate(1).unwrap();
 		// the prefix of 8 bytes is prepended to the pointer
-		assert_eq!(ptr1, to_pointer(padded_offset + 8));
+		assert_eq!(ptr1, to_pointer(padded_offset + PREFIX_SIZE));

 		let ptr2 = heap.allocate(9).unwrap();
 		// the padded_offset + the previously allocated ptr (8 bytes prefix +
 		// 8 bytes content) + the prefix of 8 bytes which is prepended to the
 		// current pointer
-		assert_eq!(ptr2, to_pointer(padded_offset + 16 + 8));
+		assert_eq!(ptr2, to_pointer(padded_offset + 16 + PREFIX_SIZE));

 		// when
 		heap.deallocate(ptr2).unwrap();
@@ -285,7 +344,7 @@ mod tests {

 		// then
 		// should have re-allocated
-		assert_eq!(ptr3, to_pointer(padded_offset + 16 + 8));
+		assert_eq!(ptr3, to_pointer(padded_offset + 16 + PREFIX_SIZE));
 		assert_eq!(heap.heads, [0; N]);
 	}

@@ -305,12 +364,12 @@ mod tests {
 		heap.deallocate(ptr3).unwrap();

 		// then
-		assert_eq!(heap.heads[0], u32::from(ptr3) - 8);
+		assert_eq!(heap.heads[0], u32::from(ptr3) - PREFIX_SIZE);

 		let ptr4 = heap.allocate(8).unwrap();
 		assert_eq!(ptr4, ptr3);

-		assert_eq!(heap.heads[0], u32::from(ptr2) - 8);
+		assert_eq!(heap.heads[0], u32::from(ptr2) - PREFIX_SIZE);
 	}

 	#[test]
@@ -323,12 +382,9 @@ mod tests {
 		let ptr = heap.allocate(PAGE_SIZE - 13);

 		// then
-		assert_eq!(ptr.is_err(), true);
-		if let Err(err) = ptr {
-			match err {
-				Error::AllocatorOutOfSpace => {},
-				_ => panic!("Expected out of space error"),
-			}
+		match ptr.unwrap_err() {
+			Error::AllocatorOutOfSpace => {},
+			e => panic!("Expected allocator out of space error, got: {:?}", e),
 		}
 	}

@@ -337,20 +393,17 @@ mod tests {
 		// given
 		let mem = MemoryInstance::alloc(Pages(1), Some(Pages(1))).unwrap();
 		let mut heap = FreeingBumpHeapAllocator::new(mem, 0);
-		let ptr1 = heap.allocate((PAGE_SIZE / 2) - 8).unwrap();
-		assert_eq!(ptr1, to_pointer(8));
+		let ptr1 = heap.allocate((PAGE_SIZE / 2) - PREFIX_SIZE).unwrap();
+		assert_eq!(ptr1, to_pointer(PREFIX_SIZE));

 		// when
 		let ptr2 = heap.allocate(PAGE_SIZE / 2);

 		// then
 		// there is no room for another half page incl. its 8 byte prefix
-		assert_eq!(ptr2.is_err(), true);
-		if let Err(err) = ptr2 {
-			match err {
-				Error::AllocatorOutOfSpace => {},
-				_ => panic!("Expected out of space error"),
-			}
+		match ptr2.unwrap_err() {
+			Error::AllocatorOutOfSpace => {},
+			e => panic!("Expected allocator out of space error, got: {:?}", e),
 		}
 	}

@@ -365,7 +418,7 @@ mod tests {
 		let ptr = heap.allocate(MAX_POSSIBLE_ALLOCATION).unwrap();

 		// then
-		assert_eq!(ptr, to_pointer(8));
+		assert_eq!(ptr, to_pointer(PREFIX_SIZE));
 	}

 	#[test]
@@ -378,12 +431,42 @@ mod tests {
 		let ptr = heap.allocate(MAX_POSSIBLE_ALLOCATION + 1);

 		// then
-		assert_eq!(ptr.is_err(), true);
-		if let Err(err) = ptr {
-			match err {
-				Error::RequestedAllocationTooLarge => {},
-				e => panic!("Expected out of space error, got: {:?}", e),
-			}
+		match ptr.unwrap_err() {
+			Error::RequestedAllocationTooLarge => {},
+			e => panic!("Expected allocation size too large error, got: {:?}", e),
+		}
+	}
+
+	#[test]
+	fn should_return_error_when_bumper_greater_than_heap_size() {
+		// given
+		let mem = MemoryInstance::alloc(Pages(1), None).unwrap();
+		let mut heap = FreeingBumpHeapAllocator::new(mem, 0);
+		heap.max_heap_size = 64;
+
+		let ptr1 = heap.allocate(32).unwrap();
+		assert_eq!(ptr1, to_pointer(PREFIX_SIZE));
+		heap.deallocate(ptr1).expect("failed freeing ptr1");
+		assert_eq!(heap.total_size, 0);
+		assert_eq!(heap.bumper, 40);
+
+		let ptr2 = heap.allocate(16).unwrap();
+		assert_eq!(ptr2, to_pointer(48));
+		heap.deallocate(ptr2).expect("failed freeing ptr2");
+		assert_eq!(heap.total_size, 0);
+		assert_eq!(heap.bumper, 64);
+
+		// when
+		// the `bumper` value is equal to `max_heap_size` here and any
+		// further allocation which would increment the bumper must fail.
+		// we try to allocate 8 bytes here, which will increment the
+		// bumper since no 8 byte item has been allocated+freed before.
+		let ptr = heap.allocate(8);
+
+		// then
+		match ptr.unwrap_err() {
+			Error::AllocatorOutOfSpace => {},
+			e => panic!("Expected allocator out of space error, got: {:?}", e),
 		}
 	}

@@ -398,7 +481,7 @@ mod tests {
 		heap.allocate(9).unwrap();

 		// then
-		assert_eq!(heap.total_size, 8 + 16);
+		assert_eq!(heap.total_size, PREFIX_SIZE + 16);
 	}

 	#[test]
@@ -409,7 +492,7 @@ mod tests {

 		// when
 		let ptr = heap.allocate(42).unwrap();
-		assert_eq!(ptr, to_pointer(16 + 8));
+		assert_eq!(ptr, to_pointer(16 + PREFIX_SIZE));
 		heap.deallocate(ptr).unwrap();

 		// then
@@ -69,8 +69,8 @@ pub enum Error {
 	/// Some error occurred in the allocator
 	#[display(fmt="Error in allocator: {}", _0)]
 	Allocator(&'static str),
-	/// The allocator run out of space.
-	#[display(fmt="Allocator run out of space")]
+	/// The allocator ran out of space.
+	#[display(fmt="Allocator ran out of space")]
 	AllocatorOutOfSpace,
 	/// Someone tried to allocate more memory than the allowed maximum per allocation.
 	#[display(fmt="Requested allocation size is too large")]