Review For My Small ( 1917 LOC ) Rust ECS

code review
4

I've spotted a few more issues, some related to public unsafe functions with insufficient preconditions in their documentation.

The preconditions on RawFns::raw_drop() are insufficient: the pointer must be writable.

/*
[dependencies.board-ecs]
git = "https://github.com/fishfolk/jumpy"
rev = "f60d766b3db100f8f36927a977a0a9f1da50210e"
*/

use board_ecs::RawFns;
use std::mem::ManuallyDrop;

fn main() {
    let s = ManuallyDrop::new(String::new());
    let ptr = &*s as *const String as *mut u8;
    // SAFETY: `ptr` points to a valid `String`, and `s` is not used again.
    unsafe {
        String::raw_drop(ptr);
    }
}

<UntypedResource as Drop>::drop() assumes that dealloc() is valid for ptr with the given layout. However, the preconditions on UntypedResource::new_raw() are insufficient to ensure this.

use board_ecs::{
    atomic::AtomicRefCell,
    resources::{UntypedResource, UntypedResourceInfo},
};
use std::{alloc::Layout, ptr, sync::Arc};

fn main() {
    extern "C" fn clone_fn(_: *const u8, _: *mut u8) {}
    extern "C" fn drop_fn(_: *mut u8) {}
    let info = UntypedResourceInfo {
        layout: Layout::new::<u8>(),
        cell: Arc::new(AtomicRefCell::new(ptr::null_mut())),
        clone_fn,
        drop_fn: Some(drop_fn),
    };
    // SAFETY: `info.clone_fn` and `info.drop_fn` do nothing unsound, since they do
    // nothing at all.
    let resource = unsafe { UntypedResource::new_raw(info) };
    drop(resource);
}

If UntypedResource::new() takes a ZST token as a resource, it drops the token, then creates an UntypedResource that can drop it again, resulting in a double-drop.

/*
[dependencies]
qcell = "0.5.2"
*/

use board_ecs::resources::UntypedResource;
use qcell::TCellOwner;

struct A(TCellOwner<()>);

impl Clone for A {
    fn clone(&self) -> Self {
        unreachable!()
    }
}

fn main() {
    let resource = UntypedResource::new(A(TCellOwner::<()>::new()));
    let mut owner1 = TCellOwner::<()>::new();
    drop(resource);
    let mut owner2 = TCellOwner::<()>::new();
    let cell = owner1.cell(0);
    let value = owner1.rw(&cell);
    owner2.rw(&cell);
    assert!(*value == 0);
}

If alloc::alloc() indicates failure in UntypedResource::new(), the function attempts to write to the null pointer it returns.

use board_ecs::resources::UntypedResource;
use std::{
    alloc::{GlobalAlloc, Layout, System},
    ptr,
    sync::atomic::{AtomicBool, Ordering},
};

struct Alloc(AtomicBool);

// SAFETY: Wraps `System`'s methods, possibly indicating failure.
unsafe impl GlobalAlloc for Alloc {
    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        if self.0.load(Ordering::Relaxed) {
            System.alloc(layout)
        } else {
            ptr::null_mut()
        }
    }

    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
        System.dealloc(ptr, layout)
    }
}

#[global_allocator]
static ALLOC: Alloc = Alloc(AtomicBool::new(true));

fn main() {
    ALLOC.0.store(false, Ordering::Relaxed);
    UntypedResource::new(0);
}

(Simply using Box::into_raw(Box::new(resource)) as *mut u8 to create the pointer would fix both of these issues.)

Similarly, UntypedResource::new() creates an UntypedResource with a clone_fn that assumes that the pointer points to a valid T. However, <UntypedResource as Clone>::clone() can call clone_fn with a null pointer if alloc::alloc() indicates failure.

use board_ecs::resources::UntypedResource;
use std::{
    alloc::{GlobalAlloc, Layout, System},
    ptr,
    sync::atomic::{AtomicBool, Ordering},
};

struct Alloc(AtomicBool);

// SAFETY: Wraps `System`'s methods, possibly indicating failure.
unsafe impl GlobalAlloc for Alloc {
    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        if self.0.load(Ordering::Relaxed) {
            System.alloc(layout)
        } else {
            ptr::null_mut()
        }
    }

    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
        System.dealloc(ptr, layout)
    }
}

#[global_allocator]
static ALLOC: Alloc = Alloc(AtomicBool::new(true));

fn main() {
    let resource = UntypedResource::new(0);
    ALLOC.0.store(false, Ordering::Relaxed);
    let _ = resource.clone();
}
2 Likes