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();
}