Unsoundness due to CodePtr allowing construction of null fn

[HiRadical]

Seeing as CodePtr is just a simple struct with a public raw pointer field

libffi-rs/libffi-rs/src/low.rs

Line 48 in 5000087

pub struct CodePtr(pub *mut c_void);

it is possible to construct a null CodePtr and use the CodePtr::as_fun to extract an invalid null fn from it:

fn main() {
    let ptr = libffi::low::CodePtr(std::ptr::null_mut());

    // Miri UB:
    // constructing invalid value: encountered a null function pointer
    let _f = *ptr.as_fun();
}

I am uncertain why CodePtr::as_fun has the following signature at all

libffi-rs/libffi-rs/src/low.rs

Line 79 in 874ff66

pub fn as_fun(&self) -> &unsafe extern "C" fn() {

seeing as fns are Copy and valid for 'static and do not require references which also goes for its fn-reference-returning friends.

To fix this, either mark CodePtr::as_fun unsafe or, in my opinion, preferably, introduce NonNull.

Also note that closure_alloc currently has the following signature despite returning null on failure.

libffi-rs/libffi-rs/src/low.rs

Line 444 in 874ff66

pub fn closure_alloc() -> (*mut ffi_closure, CodePtr) {

[arihant2math]

Interesting, unfortunately NotNull is a breaking change, but was the first thing I also thought of.

Interestingly enough, the code in question was written before NotNull was stabilized.

[HiRadical]

The problem with not having NonNull encoded into the type is obviously going to be that it encodes a less useful meaning.

What is CodePtr supposed to be if not a non-null pointer to a function?
A pointer which may actually be null that encodes the meaning of Option<NonNull<()>>?
A type with a private inner pointer that has the library-level invariant of being non-null without the actual type or perhaps an allocated but potentially unprepped non-null code pointer?

Whatever the approach ends up being, it should be made as clear as possible. Any ambiguous nullability without help from the type system is unsoundness just waiting to happen, especially seeing as this is something which is expected to be marked unsafe anyway and is not mentioned in the safety docs. In fact, it has already happened: Closure::new and Closure::new_mut are missing null checks.

Seeing as closure_alloc doesn't document the fact that it may return null and in fact even says that it must be deallocated

libffi-rs/libffi-rs/src/low.rs

Lines 433 to 435 in 874ff66

	/// initialized with [`prep_closure`] and [`prep_closure_mut`]. The
	/// closure must be deallocated using [`closure_free`], after which
	/// point the code pointer should not be used.

I do not see a reason to fear breaking code that is already unsound and cannot imagine any direct user of low is checking for null here. Even if they are turning the result to something like an Option<NonNull<ffi_closure>, CodePtr>, it would make sense to be able to be able to express the idea that if the returned closure allocation is non-null so will the code pointer, as opposed to forcing users to make that connection.