Review For My Small ( 1917 LOC ) Rust ECS

Hey there!

I've just finished the first pass on a small Entity Component System library with the aim of supporting trivial snapshots with potential for modding in the future.

I don't usually write much unsafe code, but this time I needed to because because I needed to implement an untyped storage API to support future modding possibilities.

It was forked from the great Planck ECS, but modified heavily.

I'd really appreciate any kind of review you care to give on the project.

It's got a 76% test coverage and is tested with Miri on CI, so I'm feeling pretty good about that, with hopes of improving those metrics moving forward.

Thanks!

2 Likes

The functions in the RawFns blanket impl have an issue with unwinding. If a call to <*mut T>::drop_in_place() or T::clone() unwinds, then the program will unwind through an extern "C" function, which is nominally UB, even though the compiler currently allows it. In a later version, the compiler will start immediately aborting the program in this scenario, but it's likely best to either explicitly abort on unwind, or use a non-extern "C" function to allow unwinding. Miri produces an error for this if #![feature(c_unwind)] is enabled.

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

cargo +nightly miri run
*/

#![feature(c_unwind)]

use board_ecs::resources::UntypedResource;

#[derive(Clone)]
struct A(u8);

impl Drop for A {
    fn drop(&mut self) {
        panic!();
    }
}

fn main() {
    drop(UntypedResource::new(A(0)));
}
thread 'main' panicked at 'explicit panic', src/main.rs:18:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: Undefined Behavior: unwinding past a stack frame that does not allow unwinding
   --> /home/lm978/.cargo/git/checkouts/jumpy-3e7ad82e208a45e7/2b726b5/crates/board-ecs/src/resources.rs:139:17
    |
139 |                 drop_fn(ptr);
    |                 ^^^^^^^^^^^^ unwinding past a stack frame that does not allow unwinding
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `<board_ecs::resources::UntypedResource as std::ops::Drop>::drop` at /home/lm978/.cargo/git/checkouts/jumpy-3e7ad82e208a45e7/2b726b5/crates/board-ecs/src/resources.rs:139:17: 139:29
    = note: inside `std::ptr::drop_in_place::<board_ecs::resources::UntypedResource> - shim(Some(board_ecs::resources::UntypedResource))` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:490:1: 490:56
    = note: inside `std::mem::drop::<board_ecs::resources::UntypedResource>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/mod.rs:979:24: 979:25
note: inside `main`
   --> src/main.rs:23:5
    |
23  |     drop(UntypedResource::new(A(0)));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error
/*
[dependencies.board-ecs]
git = "https://github.com/fishfolk/jumpy"
rev = "2b726b563cbf43f647b09f1161da7f06ccf69576"

cargo +nightly miri run
*/

#![feature(c_unwind)]

use board_ecs::resources::UntypedResource;

struct A;

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

fn main() {
    let _ = UntypedResource::new(A).clone();
}
thread 'main' panicked at 'explicit panic', src/main.rs:17:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: Undefined Behavior: unwinding past a stack frame that does not allow unwinding
  --> /home/lm978/.cargo/git/checkouts/jumpy-3e7ad82e208a45e7/2b726b5/crates/board-ecs/src/resources.rs:82:13
   |
82 |             (self.clone_fn)(*prev_ptr, new_ptr);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unwinding past a stack frame that does not allow unwinding
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE:
   = note: inside `<board_ecs::resources::UntypedResource as std::clone::Clone>::clone` at /home/lm978/.cargo/git/checkouts/jumpy-3e7ad82e208a45e7/2b726b5/crates/board-ecs/src/resources.rs:82:13: 82:48
note: inside `main`
  --> src/main.rs:22:13
   |
22 |     let _ = UntypedResource::new(A).clone();
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error
2 Likes

Great find, thanks!

Since I'm going to need a C FFI later, I'll use catch_unwind around the clone and drop implementations and then abort if it panics.

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

Thanks again for your time and review! I'm in the middle of moving the project to a new repo, and I'll work on fixing these once I move it.

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.