wvwwvwwv / scalable-concurrent-containers

High performance containers and utilities for concurrent and asynchronous programming
Apache License 2.0
306 stars 15 forks source link

Memory leak and alignment issue for `BucketArray` and `Collector` found using Miri. #86

Closed icmccorm closed 1 year ago

icmccorm commented 1 year ago

Using Miri, I've found a couple potential bugs related to BucketArray and Collector. Both were found when running the test hash_table::bucket_array::test::alloc.

On line 185 of src/hash_table/bucket_array.rs, the memory layout for a BucketArray is instantiated with an alignment of 1.

  (size_of_t, allocation_size, unsafe {
      Layout::from_size_align_unchecked(allocation_size, 1)
  })

However, the raw pointer allocated with this layout is eventually cast to & mut when bucket_mut is called. The alignment of the mutable reference type doesn't match the alignment of the allocation, making this conversion undefined behavior. Here's an error message that can be reproduced by running hash_table::bucket_array::test::alloc in Miri:

error: Undefined Behavior: accessing memory with alignment 4, but alignment 8 is required
   --> src/hash_table/bucket_array.rs:100:18
    |
100 |         unsafe { &mut (*(self.bucket_ptr.add(index) as *mut Bucket<K, V, LOCK_FREE>)) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory with alignment 4, but alignment 8 is required
    |

My proposed fix would be changing the layout from 1 to align_of::<T>(), but I'm unsure if this is valid.

After trying this fix, Miri detects an additional memory leak from the test hash_table::bucket_array::test::alloc, which stems from the allocation of a new Collector. In particular, when a new Collector is allocated in a Box, it's immediately unwrapped into a raw pointer. It doesn't appear that this allocation is ever re-wrapped in a Box, causing it to leak.

    fn alloc() -> *mut Collector {
        let boxed = Box::new(Collector {
           ...
        });
        let ptr = Box::into_raw(boxed);
        ...
        ptr
    }

I'm unsure of what changes would need to be made to prevent this leak.

wvwwvwwv commented 1 year ago

Wow, super thanks for the investigation! Unfortunately, I'm kind of busy these days, so not sure when I can spare some time to look into it. Just a quick note here about the super strange alignment - it is a niche optimization to let Rust directly call calloc instead of malloc + memset as calloc does not incur page faults until the memory is actually used though I'm not sure if this optimization is still valid. And, Collector itself is subject to the EBR garbage collector, therefore Collector must be deallocated correctly via Collector::epoch_updated. It is possible that a Collector would survive until the process ends, so it's no surprise that miri finds it erroneous.

Anyway, I'll surely look into those issues throughly in March!

wvwwvwwv commented 1 year ago

Fixed one obvious issue with the bucket array alignment (v1.1.2). I'll address other issues in March.

icmccorm commented 1 year ago

Thanks so much for your thorough explanation and for looking into this so quickly!

wvwwvwwv commented 1 year ago

Tried to make Collector more friendly to Miri, but failed, since the code heavily relies on some sort of stacked borrows, e.g., &mut Collector calls Collectible::drop_and_dealloc which then calls a &mut Collector method within the same stack. I'd like to close the issue for now as I have no idea how to satisfy Miri, and fortunately, I've found no problems in the code. Let me revisit this Miri related issues later.