dimforge / dimforge/nalgebra

Unsoundness: `axpy`/`axcpy` on a strided vector view reads and writes out of bounds (`axcpy_uninit` uses the bounding-box slice length as the loop count)

Open Beginner friendly
#1,616 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
4.8k
Forks
565
PR merge metrics
No merged PRs in 30d

Description

## Summary

`axcpy_uninit` derives its loop count from `x.data.as_slice_unchecked().len()`, which for a non-contiguous view is the bounding-box length `(nrows - 1) * rstride + 1`, not the vector's logical length `nrows()`. When `rstride > 1` the loop runs too many times, and both `y.get_unchecked_mut(i * stride1)` and `x.get_unchecked(i * stride2)` step past the end of their slices.

This is reachable from the safe public API `Matrix::axpy` / `Matrix::axcpy` with no `unsafe` on the user's side, and it produces a real heap out-of-bounds write, confirmed under valgrind.

Version tested: nalgebra 0.35.0 (crates.io). The same code is present on `main` today.

## Root cause

`src/base/blas_uninit.rs` (0.35.0, L106-115):

```rust
// SAFETY: the conversion to slices is OK because we access the
// elements taking the strides into account.
let y = y.data.as_mut_slice_unchecked();
let x = x.data.as_slice_unchecked();

if !b.is_zero() {
array_axcpy(status, y, a, x, c, b, rstride1, rstride2, x.len()); // <-- x.len()
} else {
array_axc(status, y, a, x, c, rstride1, rstride2, x.len()); // <-- x.len()
}
```

The strides *are* taken into account when indexing, but the **iteration count** is the slice length rather than `x.nrows()`. The two coincide only when `rstride == 1`.

`array_axcpy` (L46-52) then does:

```rust
for i in 0..len {
let y = Status::assume_init_mut(y.get_unchecked_mut(i * stride1));
*y = a.clone() * x.get_unchecked(i * stride2).clone() * c.clone()
+ beta.clone() * y.clone();
}
```

The slice length comes from `ViewStorage::as_slice_unchecked`, `src/base/matrix_view.rs` (L225-234), which deliberately returns the bounding box:

```rust
unsafe fn as_slice_unchecked(&self) -> &[T] { unsafe {
let (nrows, ncols) = self.shape();
if nrows.value() != 0 && ncols.value() != 0 {
let sz = self.linear_index(nrows.value() - 1, ncols.value() - 1);
slice::from_raw_parts(self.ptr, sz + 1)
} else {
slice::from_raw_parts(self.ptr, 0)
}
}}
```

Note also that `axcpy_uninit` bounds `SB: RawStorage` with no `IsContiguous` requirement, so passing a strided view is entirely legal.

## PoC

`Cargo.toml`:

```toml
[dependencies]
nalgebra = "=0.35.0"

[profile.release]
debug = true
```

`src/main.rs` — no `unsafe` anywhere:

```rust
use nalgebra::{DVector, Matrix3};

fn main() {
let m = Matrix3::::new(
1.0, 4.0, 7.0,
2.0, 5.0, 8.0,
3.0, 6.0, 9.0,
);

// strided view: 2 rows, 1 col, row-step 1 => row stride 2
let mv = m.view_with_steps((0, 0), (2, 1), (1, 0));
let v = mv.column(0);

let (rs, _) = v.strides();
println!("v.nrows() = {}", v.nrows()); // 2
println!("v.strides() = {:?}", v.strides()); // (2, 3)
println!("bounding-box slice len = {}", (v.nrows() - 1) * rs + 1); // 3

let mut y = DVector::::zeros(2);
y.axpy(1.0, &v, 1.0); // safe API -> OOB read of x and OOB write of y

println!("y = {:?}", y.as_slice());
}
```

## Observed behaviour

Ubuntu, x86-64, rustc 1.96.0-nightly, pristine `nalgebra 0.35.0` from crates.io.

**1. Debug build — std's UB check fires and aborts**

```
v.nrows() = 2
v.strides() = (2, 3)
bounding-box slice len = (nrows-1)*rstride + 1 = 3
y.len() = 2
loop will run 3 times, writing y[i*1] for i in 0..3
-> max y index touched = 2 but y only has 2 elements

calling SAFE api y.axpy(1.0, &v, 1.0) ...

thread 'main' panicked at .../nalgebra-0.35.0/src/base/blas_uninit.rs:48:47:
unsafe precondition(s) violated: slice::get_unchecked_mut requires that the index is within the slice
thread caused non-unwinding panic. aborting.
DEBUG_EXIT=134
```

**2. Release build — segmentation fault**

```
calling SAFE api y.axpy(1.0, &v, 1.0) ...
RELEASE_EXIT=139 # SIGSEGV
```

**3. Release under valgrind — heap OOB read *and* write, pinned to the source line**

```
==2918041== Invalid read of size 8
==2918041== at 0x11C41B: repro_nalgebra::main (clone.rs:615)
==2918041== Address 0x4ac6130 is 0 bytes after a block of size 16 alloc'd
==2918041== by 0x11C31B: repro_nalgebra::main (alloc.rs:101)

==2918041== Invalid write of size 8
==2918041== at 0x11C416: repro_nalgebra::main (src/base/blas_uninit.rs:49)
==2918041== Address 0x4ac6130 is 0 bytes after a block of size 16 alloc'd
==2918041== by 0x11C31B: repro_nalgebra::main (alloc.rs:101)

==2918041== Invalid read of size 8
==2918041== Address 0x1fff001000 is not stack'd, malloc'd or (recently) free'd

==2918041== ERROR SUMMARY: 279 errors from 3 contexts
```

The invalid write is attributed to `blas_uninit.rs:49` and lands **0 bytes after the 16-byte allocation** backing `DVector::zeros(2)` — i.e. exactly the `y[2]` write predicted above.

## Scope

The same `x.len()`-as-loop-count pattern feeds the other BLAS entry points that route through this code, so `gemv` (whose `a.column(j)` is a strided view for non-contiguous `a`) and the small-dimension/non-`f32`/`f64` fallback paths in `gemm_uninit` are worth auditing alongside it.

Contiguous storage (`rstride == 1`) is unaffected, which is why ordinary whole-matrix arithmetic never hits this.

## Suggested fix

Use the logical length instead of the slice length in `axcpy_uninit`:

```diff
--- a/src/base/blas_uninit.rs
+++ b/src/base/blas_uninit.rs
@@
assert_eq!(y.nrows(), x.nrows(), "Axcpy: mismatched vector shapes.");

+ let len = x.nrows();
let rstride1 = y.strides().0;
let rstride2 = x.strides().0;

// SAFETY: the conversion to slices is OK because we access the
// elements taking the strides into account.
let y = y.data.as_mut_slice_unchecked();
let x = x.data.as_slice_unchecked();

if !b.is_zero() {
- array_axcpy(status, y, a, x, c, b, rstride1, rstride2, x.len());
+ array_axcpy(status, y, a, x, c, b, rstride1, rstride2, len);
} else {
- array_axc(status, y, a, x, c, rstride1, rstride2, x.len());
+ array_axc(status, y, a, x, c, rstride1, rstride2, len);
}
```

(`len` must be read before the re-binding of `x`, since the shadowed `x` is a slice.) It may also be worth documenting on `as_slice_unchecked` that the returned slice spans the bounding box, so its `len()` is not a valid element count for strided views. I'm happy to open a PR with this change plus a regression test if that's useful.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/base/blas_uninit.rs at axcpy_uninit and the array_axcpy/array_axc call sites; compare the logical vector length with the bounding-box slice length used for iteration. Add a regression test using a strided vector view through Matrix::axpy or Matrix::axcpy, then run the relevant test suite and verify that no out-of-bounds access occurs.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.