Skip to content

[BUG] Multiple soundness issues reachable from safe code #385

Description

@phil-opp

Five separate soundness problems, all reachable from safe code. Grouping them since they're related in character; happy to split into individual issues if that's easier to triage.

Found by Claude Opus 5 after prompting it to look for potential causes for 0xC0000005 errors on Windows.

Description

1. AfError::from transmutes values that are not valid discriminants

src/core/util.rs:69-74:

impl From<i32> for AfError {
    fn from(t: i32) -> Self {
        assert!(AfError::SUCCESS as i32 <= t && t <= AfError::ERR_UNKNOWN as i32);
        unsafe { mem::transmute(t) }
    }
}

The assert bounds t to [0, 999], but AfError has 17 sparse variants in that range. A range check is not a validity check for a sparse enum, so any in-range value that isn't a declared discriminant becomes an invalid #[repr(u32)] enum value — immediate UB.

Five error codes that ArrayFire genuinely returns have no Rust variant (verified against include/af/defines.h at v3.8.0; unchanged in 3.10):

C value C name
303 AF_ERR_NONFREE
403 AF_ERR_NO_HALF
501 AF_ERR_LOAD_LIB
502 AF_ERR_LOAD_SYM
503 AF_ERR_ARR_BKND_MISMATCH

AF_ERR_LOAD_LIB = 501 is what the unified backend's CALL macro returns when it cannot locate a backend (src/api/unified/symbol_manager.hpp:155, "ArrayFire couldn't locate any backends."). Since build.rs links the unified af library by default, this is live on every call for anyone whose backend DLL fails to load.

AF_ERR_ARR_BKND_MISMATCH = 503 is returned whenever an Array from one backend is used after set_backend switched to another — which examples/unified.rs does.

The value then flows into Display for AfError (src/core/defines.rs:84-105), an exhaustive match with no wildcard arm, which rustc lowers to a switch with an unreachable default.

Observed behaviour with a faithful standalone repro of the enum + Display + From:

  • Debug: panicked: trying to construct an enum from an invalid value 0x1f5, then thread caused non-unwinding panic. aborting.
  • Release (-O): survives, and reports "Unknown Error" — LLVM happens to bounds-guard the jump table.

So on current codegen this is more often a diagnostic failure than a crash: users are told "Unknown Error" instead of "ArrayFire couldn't locate any backends," which I suspect is a large part of why #285 and #311 have gone undiagnosed for years. But it is UB regardless and the release-mode behaviour is not guaranteed.

Note this is the same failure mode as RUSTSEC-2018-0011; the #[repr(u32)] half of that fix landed, but this conversion still constructs invalid values.

Fix: replace with an exhaustive match mapping unknown codes to ERR_UNKNOWN, and add the five missing variants. RandomEngineType::from (util.rs:429-437) has the same sparse-range hole, though I couldn't find a reachable trigger for it.


2. MatProp::from transmutes with no validation at all, and BitOr manufactures invalid values

src/core/util.rs:829-841:

impl From<u32> for MatProp {
    fn from(t: u32) -> Self {
        unsafe { mem::transmute(t) }
    }
}

impl BitOr for MatProp {
    type Output = Self;
    fn bitor(self, rhs: Self) -> Self {
        Self::from(self as u32 | rhs as u32)
    }
}

MatProp is a bit-flag enum with variants 0, 1, 2, 4, 32, 64, 128, 512, 1024, 2048, 4096, 8192. Combining any two non-adjacent flags produces a value with no corresponding variant:

let p = MatProp::UPPER | MatProp::DIAGUNIT;   // 32 | 128 = 160 -> UB

This is 100% safe code, using the API the docs point at for matmul/solve/LAPACK routines.

Fix: MatProp should be a bitflags-style newtype over u32 rather than an enum. A minimal stopgap is to drop the From/BitOr impls and have callers pass u32.


3. #[derive(Clone)] on Window alongside Drop is a double free

src/graphics/mod.rs:180-199:

#[derive(Clone)]
pub struct Window {
    handle: af_window,
    ...
}

impl Drop for Window {
    fn drop(&mut self) {
        let err_val = unsafe { af_destroy_window(self.handle) };

The derived Clone bit-copies the raw af_window; both copies then call af_destroy_window on the same handle.

Every other handle wrapper in the crate gets this right with a hand-written Clone that calls the corresponding retain function — Array (af_retain_array, array.rs:706-718), Features (af_retain_features, vision/mod.rs:194-200), RandomEngine (af_retain_random_engine, random.rs:197-205). Window looks like it was simply missed.

Fix: replace the derive with a manual Clone that retains, or remove Clone if Forge has no retain equivalent for windows.


4. alloc_host always returns NULL and leaks

src/core/util.rs:53-61:

pub fn alloc_host<T>(elements: usize, _type: DType) -> *const T {
    let ptr: *const T = ::std::ptr::null();
    let bytes = (elements * get_size(_type)) as dim_t;

    let err_val = unsafe { af_alloc_host(&mut (ptr as *const c_void), bytes) };
    HANDLE_ERROR(AfError::from(err_val));

    ptr
}

&mut (ptr as *const c_void) takes a mutable borrow of the temporary produced by the cast, not of ptr. ArrayFire writes the allocated pointer into that temporary, which is discarded at the end of the statement. ptr isn't even declared mut, so it cannot be written to.

The function therefore returns NULL unconditionally and leaks the host allocation on every call. Any caller dereferencing the result gets a null-pointer access violation.

Fix:

pub fn alloc_host<T>(elements: usize, _type: DType) -> *const T {
    let mut ptr: *mut c_void = ::std::ptr::null_mut();
    let bytes = (elements * get_size(_type)) as dim_t;
    let err_val = unsafe { af_alloc_host(&mut ptr, bytes) };
    HANDLE_ERROR(AfError::from(err_val));
    ptr as *const T
}

5. Array::set is safe but installs an arbitrary handle

src/core/array.rs:493-496:

/// Set the native FFI handle for Rust object `Array`
pub fn set(&mut self, handle: af_array) {
    self.handle = handle;
}

Safe code can store any pointer value here; Drop (array.rs:721-729) then calls af_release_array on it. It also leaks the previously held handle.

The neighbouring getter get() at array.rs:489 is correctly marked unsafe, so this looks like an oversight rather than a deliberate choice.

Fix: make it unsafe fn, and document the invariant that the handle must be a valid, owned af_array.

Reproducible Code and/or Steps

System Information

Checklist

  • Using the latest available ArrayFire release
  • GPU drivers are up to date

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions