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
9 changes: 9 additions & 0 deletions src/btreemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,15 @@ where
/// Removes all elements from the map.
// TODO: In next major release (v1.0), rename this method to `clear` to follow
// standard Rust collection naming conventions.
///
/// # Safety Note for Bucket Release
/// If using manual bucket release via `MemoryManager::release_virtual_memory_buckets()`:
/// 1. **MANDATORY**: Drop this BTreeMap object first (let it go out of scope)
/// 2. Call `release_virtual_memory_buckets()` on the memory manager
/// 3. Create new structures as needed
///
/// Using this BTreeMap after bucket release causes data corruption.
/// Note: You can still call `clear_new()` if you need to clear data without bucket release.
pub fn clear_new(&mut self) {
self.root_addr = NULL;
self.length = 0;
Expand Down
9 changes: 9 additions & 0 deletions src/btreeset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,15 @@ where
/// set.clear();
/// assert!(set.is_empty());
/// ```
///
/// # Safety Note for Bucket Release
/// If using manual bucket release via `MemoryManager::release_virtual_memory_buckets()`:
/// 1. **MANDATORY**: Drop this BTreeSet object first (let it go out of scope)
/// 2. Call `release_virtual_memory_buckets()` on the memory manager
/// 3. Create new structures as needed
///
/// Using this BTreeSet after bucket release causes data corruption.
/// Note: You can still call `clear()` if you need to clear data without bucket release.
pub fn clear(&mut self) {
self.map.clear_new();
}
Expand Down
41 changes: 29 additions & 12 deletions src/memory_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,13 @@ const HEADER_RESERVED_BYTES: usize = 32;
/// Bucket MAX_NUM_BUCKETS ↕ N pages
/// ```
///
/// # Current Limitations
/// # Current Limitations & Safety Requirements
///
/// - Bucket release is manual - call `release_virtual_memory_buckets()` after clearing
/// - No safety verification - user must ensure memory is empty before releasing
/// - Incorrect usage leads to data corruption and undefined behavior
/// - **Manual bucket release** - call `release_virtual_memory_buckets()` after dropping structures
/// - **Structure invalidation** - original structures become invalid after bucket release
/// - **Mandatory drop** - you MUST drop original structures BEFORE releasing buckets
/// - **No safety verification** - user discipline required to prevent data corruption
/// - **Incorrect usage** - using structures after bucket release causes data corruption
pub struct MemoryManager<M: Memory> {
inner: Rc<RefCell<MemoryManagerInner<M>>>,
}
Expand Down Expand Up @@ -164,16 +166,23 @@ impl<M: Memory> MemoryManager<M> {

/// Releases buckets allocated to the specified virtual memory for reuse.
///
/// **Warning**: No verification performed. User must clear data structures first
/// to avoid data corruption. Released buckets are automatically reused by other memories.
/// **CRITICAL SAFETY REQUIREMENT**: Before calling this method:
/// 1. You MUST drop the original structure object first
///
/// Returns the number of buckets released and added to the free pool (0 if none allocated).
/// **After calling this method**:
/// 2. The data structure using this memory ID becomes INVALID
/// 3. You MUST create a new structure if you want to reuse the memory ID
///
/// # Example
/// **Correct Usage Pattern:**
/// ```rust,ignore
/// map.clear_new(); // MANDATORY: Clear all data before releasing buckets
/// let count = memory_manager.release_virtual_memory_buckets(memory_id);
/// drop(map); // 1. MANDATORY: Drop the structure object first
/// let count = memory_manager.release_virtual_memory_buckets(memory_id); // 2. Release buckets
/// let new_map = BTreeMap::new(memory_manager.get(memory_id)); // 3. Create fresh structure
/// ```
///
/// **DANGER**: Using the original structure after bucket release causes data corruption.
///
/// Returns the number of buckets released and added to the free pool (0 if none allocated).
pub fn release_virtual_memory_buckets(&self, id: MemoryId) -> usize {
self.inner.borrow_mut().release_virtual_memory_buckets(id)
}
Expand Down Expand Up @@ -433,8 +442,10 @@ impl<M: Memory> MemoryManagerInner<M> {
/// Releases buckets for the specified memory, marking them unallocated in stable storage
/// and adding them to the free pool. Resets memory size to 0.
///
/// **Warning**: No verification performed - caller must ensure data is cleared first
/// to avoid data corruption.
/// **CRITICAL**: This invalidates any data structures using this memory ID.
/// Caller must ensure:
/// 1. Original structure object is dropped BEFORE calling this
/// 2. New structure is created if memory ID needs to be reused
///
Comment thread
maksymar marked this conversation as resolved.
/// Returns the number of buckets released and added to the free pool (0 if none allocated).
fn release_virtual_memory_buckets(&mut self, id: MemoryId) -> usize {
Expand Down Expand Up @@ -1249,6 +1260,9 @@ mod test {
let released_count = mem_mgr.release_virtual_memory_buckets(MemoryId::new(0));
assert_eq!(released_count, 1, "Should release exactly 1 bucket");

// CRITICAL: Drop the memory_0 after bucket release as it's now invalid
drop(memory_0);

// Verify memory size didn't change (buckets marked free, not deallocated)
assert_eq!(mem.size(), size_after_allocation);

Expand Down Expand Up @@ -1332,3 +1346,6 @@ mod bucket_release_core_tests;

#[cfg(test)]
mod bucket_release_btreemap_tests;

#[cfg(test)]
mod bucket_release_vec_tests;
144 changes: 127 additions & 17 deletions src/memory_manager/bucket_release_btreemap_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,30 @@
//!
//! These tests demonstrate real-world migration patterns where users move data
//! from one structure to another. They show how bucket release prevents memory
//! waste during common migration scenarios.
//! waste during common migration scenarios, and most importantly, demonstrate
//! the data corruption bug and its safe usage solution.
//!
//! **CRITICAL SAFETY REQUIREMENTS**:
//! All bucket release operations require mandatory Rust object drop BEFORE release.
//! Using original data structures after bucket release causes data corruption.
//! See MemoryManager documentation for proper usage patterns.

use super::{MemoryId, MemoryManager};
use crate::{btreemap::BTreeMap, vec_mem::VectorMemory, Memory};

/// Helper function to create a blob that triggers bucket allocation
fn blob() -> Vec<u8> {
vec![42u8; 2000] // 2KB blob
fn large_value(id: u64) -> Vec<u8> {
let mut data = vec![0u8; 1024]; // 1KB value
// Fill with pattern based on id to make values unique
for (i, byte) in data.iter_mut().enumerate() {
*byte = ((id + i as u64) % 256) as u8;
}
data
}

#[test]
fn migration_without_release_wastes_buckets() {
// Scenario: Populate A → Clear A without bucket release → Populate B
// Scenario: Populate A → Drop A without bucket release → Populate B
// Result: B cannot reuse A's buckets, causing memory waste (growth)
let (a, b) = (MemoryId::new(0), MemoryId::new(1));
let mock_stable_memory = VectorMemory::default();
Expand All @@ -23,29 +34,29 @@ fn migration_without_release_wastes_buckets() {
// Allocate in A
let mut a_map = BTreeMap::init(mm.get(a));
for i in 0u64..50 {
a_map.insert(i, blob());
a_map.insert(i, large_value(i));
}
assert_eq!(a_map.len(), 50);

// Clear A without releasing buckets
a_map.clear_new();
// Drop A without releasing buckets
drop(a_map);
let stable_before = mock_stable_memory.size();

// Allocate in B → should need new buckets since A's aren't released
let mut b_map = BTreeMap::init(mm.get(b));
for i in 0u64..50 {
b_map.insert(i, blob());
b_map.insert(i, large_value(i + 100));
}
let stable_after = mock_stable_memory.size();

// Verify: maps correct, stable memory grew (waste)
assert_eq!(a_map.len(), 0);
// Verify: B allocated new buckets, stable memory grew (waste)
assert_eq!(b_map.len(), 50);
assert!(stable_after > stable_before, "Stable memory grew (waste)");
}

#[test]
fn migration_with_release_reuses_buckets() {
// Scenario: Populate A → Clear A with bucket release → Populate B
// Scenario: Populate A → Drop A with bucket release → Populate B
// Result: B reuses A's released buckets, preventing memory waste (no growth)
let (a, b) = (MemoryId::new(0), MemoryId::new(1));
let mock_stable_memory = VectorMemory::default();
Expand All @@ -54,27 +65,126 @@ fn migration_with_release_reuses_buckets() {
// Allocate in A
let mut a_map = BTreeMap::init(mm.get(a));
for i in 0u64..50 {
a_map.insert(i, blob());
a_map.insert(i, large_value(i));
}
assert_eq!(a_map.len(), 50);

// Clear A and release its buckets
a_map.clear_new();
// MANDATORY: Drop the Rust object before releasing buckets
drop(a_map);

// Release the buckets after dropping the object
let released_buckets = mm.release_virtual_memory_buckets(a);
assert!(released_buckets > 0);
let stable_before = mock_stable_memory.size();

// Allocate in B → should reuse A's released buckets
let mut b_map = BTreeMap::init(mm.get(b));
for i in 0u64..50 {
b_map.insert(i, blob());
b_map.insert(i, large_value(i + 100));
}
let stable_after = mock_stable_memory.size();

// Verify: maps correct, stable memory unchanged (reuse)
assert_eq!(a_map.len(), 0);
// Verify: B reused A's buckets, stable memory unchanged (reuse)
assert_eq!(b_map.len(), 50);
assert!(
stable_after <= stable_before,
"Stable memory unchanged (reuse)"
);
}

/// **DANGER**: This test demonstrates data corruption from unsafe bucket release usage.
/// This shows what happens when you DON'T drop the original object before bucket release.
#[test]
fn data_corruption_without_mandatory_drop() {
let (a, b) = (MemoryId::new(0), MemoryId::new(1));
let mm = MemoryManager::init(VectorMemory::default());

// Create BTreeMap A with test data
let mut map_a = BTreeMap::init(mm.get(a));
map_a.insert(1u64, 100u64);
assert_eq!(map_a.get(&1u64).unwrap(), 100u64);

// DANGEROUS: Release buckets but keep map_a alive
Comment thread
maksymar marked this conversation as resolved.
mm.release_virtual_memory_buckets(a);

// Create BTreeMap B - reuses A's released buckets
let mut map_b = BTreeMap::init(mm.get(b));
map_b.insert(2u64, 200u64);
assert_eq!(map_b.get(&2u64).unwrap(), 200u64);

// CORRUPTION: map_a and map_b now share the same underlying memory
// This can manifest in different ways - either seeing shared data or allocation corruption

// First check if map_a can see map_b's data (shared memory corruption)
if let Some(corrupted_data) = map_a.get(&2u64) {
assert_eq!(
corrupted_data, 200u64,
"CORRUPTION: map_a sees map_b's data!"
);
return; // Corruption demonstrated through shared data
}

// If shared data isn't visible, try to trigger allocation corruption
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
map_a.insert(3u64, 300u64);
map_a.get(&3u64)
}));

// Should either panic or show corruption through shared allocations
match result {
Ok(_) => {
// If it succeeds, check if map_b can see the new data (shared allocation)
if map_b.get(&3u64).is_some() {
println!("CORRUPTION: Both maps operating on the same released memory space");
}
}
Err(_) => {
// Expected: panic due to allocator corruption
println!("CORRUPTION: Panic due to memory corruption - this proves the bug");
}
}

// This test proves why objects MUST be dropped before bucket release
}

/// **SAFE**: This test demonstrates the correct way to use bucket release.
/// This shows how mandatory drop prevents data corruption.
#[test]
fn safe_usage_with_mandatory_drop() {
let (a, b) = (MemoryId::new(0), MemoryId::new(1));
let mm = MemoryManager::init(VectorMemory::default());

// Create and populate BTreeMap A
let mut map_a = BTreeMap::init(mm.get(a));
map_a.insert(1u64, 100u64);
assert_eq!(map_a.get(&1u64).unwrap(), 100u64);

// MANDATORY: Drop the Rust object before releasing buckets
drop(map_a);

// Release the buckets after dropping the object
let released_buckets = mm.release_virtual_memory_buckets(a);
assert!(released_buckets > 0);

// Create BTreeMap B - safely reuses A's released buckets
let mut map_b = BTreeMap::init(mm.get(b));
map_b.insert(2u64, 200u64);
assert_eq!(map_b.get(&2u64).unwrap(), 200u64);

// Create new BTreeMap on same memory ID A - safe after proper drop
let mut map_a_new = BTreeMap::init(mm.get(a));
map_a_new.insert(3u64, 300u64);
assert_eq!(map_a_new.get(&3u64).unwrap(), 300u64);

// Verify maps are completely independent - no corruption
assert_eq!(map_b.len(), 1, "Map B should have 1 element");
assert_eq!(map_a_new.len(), 1, "Map A new should have 1 element");
assert_eq!(map_b.get(&2u64).unwrap(), 200u64);
assert_eq!(map_a_new.get(&3u64).unwrap(), 300u64);

// Both maps can grow independently without corruption
map_a_new.insert(4u64, 400u64);
map_b.insert(5u64, 500u64);
assert_eq!(map_a_new.len(), 2);
assert_eq!(map_b.len(), 2);
}
7 changes: 6 additions & 1 deletion src/memory_manager/bucket_release_core_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
//! These tests verify the basic memory management operations without dependency
//! on specific data structures. They test the fundamental bucket allocation,
//! release, and reuse mechanisms.
//!
//! **CRITICAL SAFETY REQUIREMENTS**:
//! All bucket release operations require mandatory Rust object drop BEFORE release.
//! Using original data structures after bucket release causes data corruption.
//! See MemoryManager documentation for proper usage patterns.

use super::{MemoryId, MemoryManager};
use crate::{btreemap::BTreeMap, vec_mem::VectorMemory, Memory};
Expand All @@ -17,7 +22,7 @@ fn allocate_buckets_via_btreemap(
for i in 0..count {
map.insert(i, vec![42u8; 2000]); // 2KB blob to trigger bucket allocation
}
map.clear_new(); // Clear the structure but keep buckets allocated
drop(map); // Drop the structure before bucket release
Comment thread
maksymar marked this conversation as resolved.
}

#[test]
Expand Down
Loading