`GapBuffer` Data Structure Is Excessively Unsafe`

[Barca545]

The current implementation of the BackingBuffer which relies on a NonNull and unsafe operations introduces more unsafety into the GapBuffer than I believe is strictly necessary.

Here is the relevant code:

enum BackingBuffer {
    VirtualMemory(NonNull<u8>, usize),
    Vec(Vec<u8>),
}

impl Drop for BackingBuffer {
    fn drop(&mut self) {
        unsafe {
            if let Self::VirtualMemory(ptr, reserve) = *self {
                sys::virtual_release(ptr, reserve);
            }
        }
    }
}

After going through the codebase, my understanding is this is to allow for BackingBuffer::VirtualMemory to be obtained via Windows' VirtualAlloc and unix's memmap function as used in sys::unix::virtual_reserve and sys::windows::virtual_reserve.

I believe this unsafety could be avoided by making BackingBuffer::VirtualMemory wrap a vector Vec<u8, VirtualAllocator> which relies on a VirtualAllocator struct which implements Allocator.

This would allow the GapBuffer methods which currently rely on unsafe pointer arithmetic such as move_gap and delete_text to switch to using safe vector/slice operations such as copy_within.

Below I included and example of what a "VirtualAllocator" might look like:

#[derive(Debug, Clone, Copy,)]
struct VirtualAllocator;

unsafe impl Allocator for VirtualAllocator {
  /// When defining the `layout` its size should be `sizeof::<T> * num` not
  /// `sizeof::<T>`.
  fn allocate(&self, layout: Layout,) -> Result<NonNull<[u8],>, AllocError,> {
    let base = ptr::null_mut();
    let len = layout.size();
    unsafe {
      let c_ptr = Memory::VirtualAlloc(base, len, Memory::MEM_RESERVE, Memory::PAGE_READWRITE,);
      match NonNull::new(ptr::from_raw_parts_mut(c_ptr, len,),) {
        Some(ptr,) => Ok(ptr,),
        None => todo!(),
      }
    }
  }

  unsafe fn deallocate(&self, ptr: NonNull<u8,>, layout: std::alloc::Layout,) {
    todo!()
  }
}

static ALLOCATOR: VirtualAllocator = VirtualAllocator;

enum BackingBuffer {
    VirtualMemory(Vec<u8, VirtualAllocator>,),
    Vec(Vec<u8>),
}

[lhecker]

That would squarely run us into UB / soundness issues with Rust.

The premise of the gap buffer is that the backing contents remain uninitialized and that the uninitialized portions can be moved anywhere within the backing buffer. The only way to get uninitialized contents with a Vec like that is by using Vec::spare_capacity_mut, which is always at the end of the buffer. The alternative is calling Vec::set_len but at that point you may as well use a Box<[MaybeUninit<u8>]>, right?

I'll close this issue, as I personally will not make such a change myself and thus don't plan to track it. If using a Box<[MaybeUninit<u8>]> is something you'd like to pursue, please send a PR. If the resulting code is more compact / prettier, I'll merge it. The only requirement is that you can prove that the change is "sound" and that the binary size doesn't grow from it.

You're not the first to report issues about the use of "unsafe" in the project. To clarify: The project is optimized for performance and compactness first and the usage of Rust is just a circumstance in comparison.