Fix a concurrency bug in mapping chunk side metadata for mallocspace (#404)

Author: qinsoon

src/policy/mallocspace/global.rs

@@ -201,6 +201,7 @@ impl<VM: VMBinding> MallocSpace<VM> {
 
         if !address.is_zero() {
             let actual_size = unsafe { malloc_usable_size(raw) };
+            // If the side metadata for the address has not yet been mapped, we will map all the side metadata for the address.
             if !is_meta_space_mapped(address) {
                 let chunk_start = conversions::chunk_align_down(address);
                 debug!(

src/policy/mallocspace/metadata.rs

@@ -20,7 +20,7 @@ lazy_static! {
         local: vec![],
     };
 
-    // lock to synchronize the mapping for the active chunk space
+    // lock to synchronize the mapping of side metadata for a newly allocated chunk by malloc
     static ref CHUNK_MAP_LOCK: Mutex<()> = Mutex::new(());
 }
 
@@ -110,27 +110,37 @@ fn map_active_chunk_metadata(chunk_start: Address) {
 // We map the active chunk metadata (if not previously mapped), as well as the alloc bit metadata
 // and active page metadata here
 pub fn map_meta_space_for_chunk(metadata: &SideMetadataContext, chunk_start: Address) {
-    {
-        // In order to prevent race conditions, we synchronize on the lock first and then
-        // check if we need to map the active chunk metadata for `chunk_start`
-        let _lock = CHUNK_MAP_LOCK.lock().unwrap();
-        if !is_chunk_mapped(chunk_start) {
-            map_active_chunk_metadata(chunk_start);
-        }
+    // In order to prevent race conditions, we synchronize on the lock first and then
+    // check if we need to map the active chunk metadata for `chunk_start`
+    let _lock = CHUNK_MAP_LOCK.lock().unwrap();
+
+    // Check if the chunk bit metadata is mapped. If it is not mapped, map it.
+    // Note that the chunk bit metadata is global. It may have been mapped because other policy mapped it.
+    if !is_chunk_mapped(chunk_start) {
+        map_active_chunk_metadata(chunk_start);
     }
 
+    // If we have set the chunk bit, return. This is needed just in case another thread has done this before
+    // we can acquire the lock.
     if is_chunk_marked(chunk_start) {
         return;
     }
 
-    set_chunk_mark(chunk_start);
+    // Attempt to map the local metadata for the policy.
+    // Note that this might fail. For example, we have marked a chunk as active but later we freed all
+    // the objects in it, and unset its chunk bit. However, we do not free its metadata. So for the chunk,
+    // its chunk bit is mapped, but not marked, and all its local metadata is also mapped.
     let mmap_metadata_result = metadata.try_map_metadata_space(chunk_start, BYTES_IN_CHUNK);
-    trace!("set chunk mark bit for {}", chunk_start);
     debug_assert!(
         mmap_metadata_result.is_ok(),
         "mmap sidemetadata failed for chunk_start ({})",
         chunk_start
     );
+
+    // Set the chunk mark at the end. So if we have chunk mark set, we know we have mapped side metadata
+    // for the chunk.
+    trace!("set chunk mark bit for {}", chunk_start);
+    set_chunk_mark(chunk_start);
 }
 
 // Check if a given object was allocated by malloc