arrayfire / arrayfire/arrayfire-rust

[BUG] Multiple soundness issues reachable from safe code

Ouverte
#385 1 commentaire 0 réactions 0 personnes assignées Voir sur GitHub
Bug
Langage dominant
Rust
Étoiles
827
Forks
59
Métriques de merge des PR
Aucune PR mergée en 30 j

Description

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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/core/util.rs#L69-L74):

```rust
impl From 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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/core/defines.rs#L84-L105)), 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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/core/util.rs#L429-L437)) 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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/core/util.rs#L829-L841):

```rust
impl From 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:

```rust
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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/graphics/mod.rs#L180-L199):

```rust
#[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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/core/util.rs#L53-L61):

```rust
pub fn alloc_host(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:**

```rust
pub fn alloc_host(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`](https://github.com/arrayfire/arrayfire-rust/blob/master/src/core/array.rs#L493-L496):

```rust
/// 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

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Piste de recherche

Start with the cited locations in src/core/util.rs and src/core/defines.rs, then inspect src/graphics/mod.rs and src/core/array.rs, including the neighbouring handle methods. Use examples/unified.rs to understand backend switching and the documented failure path. Done means each reported safe-code path preserves valid enum and native-handle invariants without leaks or double releases.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
rust
Domaine
api, backend
Type d'issue
Bug
Difficulté
5/5
Temps estimé
Plus d'une semaine
Activité
Calme
Clarté
Clairement spécifiée
Accessibilité débutants
35/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.