nalgebra icon indicating copy to clipboard operation
nalgebra copied to clipboard

Fix UB in `RawStorageMut::swap_unchecked_linear`

Open yotamofek opened this issue 2 years ago • 0 comments

According to Miri, RawStorageMut::swap_unchecked_linear introduces a stacked borrows violation:

➜  nalgebra git:(dev) ✗ cargo miri test -- edition::swap
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
   Compiling nalgebra-macros v0.2.1 (/home/yotam/nalgebra/nalgebra-macros)
   Compiling nalgebra v0.32.3 (/home/yotam/nalgebra)

     Running tests/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/lib-85cd9506b7860f9a)

running 2 tests
test core::edition::swap_columns ... error: Undefined Behavior: attempting a read access using <268833> at alloc82401[0x18], but that tag does not exist in the borrow stack for this location
    --> /home/yotam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:878:9
     |
878  |         copy_nonoverlapping(x, tmp.as_mut_ptr(), 1);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |         |
     |         attempting a read access using <268833> at alloc82401[0x18], but that tag does not exist in the borrow stack for this location
     |         this error occurs as part of an access at alloc82401[0x18..0x20]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <268833> was created by a SharedReadWrite retag at offsets [0x0..0x78]
    --> /home/yotam/nalgebra/src/base/array_storage.rs:141:9
     |
141  |         self.0.as_mut_ptr() as *mut T
     |         ^^^^^^^^^^^^^^^^^^^
help: <268833> was later invalidated at offsets [0x0..0x78] by a Unique function-entry retag inside this call
    --> /home/yotam/nalgebra/src/base/storage.rs:200:17
     |
200  |         let b = self.get_address_unchecked_linear_mut(i2);
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: BACKTRACE (of the first span):
     = note: inside `std::ptr::swap::<f64>` at /home/yotam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:878:9: 878:52
note: inside `<na::ArrayStorage<f64, 3, 5> as na::RawStorageMut<f64, na::Const<3>, na::Const<5>>>::swap_unchecked_linear`
    --> /home/yotam/nalgebra/src/base/storage.rs:202:9
     |
202  |         ptr::swap(a, b);
     |         ^^^^^^^^^^^^^^^
note: inside `<na::ArrayStorage<f64, 3, 5> as na::RawStorageMut<f64, na::Const<3>, na::Const<5>>>::swap_unchecked`
    --> /home/yotam/nalgebra/src/base/storage.rs:214:9
     |
214  |         self.swap_unchecked_linear(lid1, lid2)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `na::Matrix::<f64, na::Const<3>, na::Const<5>, na::ArrayStorage<f64, 3, 5>>::swap_unchecked`
    --> /home/yotam/nalgebra/src/base/matrix.rs:1201:9
     |
1201 |         self.data.swap_unchecked(row_cols1, row_cols2)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `na::base::edition::<impl na::Matrix<f64, na::Const<3>, na::Const<5>, na::ArrayStorage<f64, 3, 5>>>::swap_columns`
    --> /home/yotam/nalgebra/src/base/edition.rs:331:26
     |
331  |                 unsafe { self.swap_unchecked((i, icol1), (i, icol2)) }
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `core::edition::swap_columns`
    --> tests/core/edition.rs:208:5
     |
208  |     m.swap_columns(1, 3);
     |     ^^^^^^^^^^^^^^^^^^^^
note: inside closure
    --> tests/core/edition.rs:197:18
     |
195  | #[test]
     | ------- in this procedural macro expansion
196  | #[rustfmt::skip]
197  | fn swap_columns() {
     |                  ^
     = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

error: test failed, to rerun pass `--test lib`

Caused by:
  process didn't exit successfully: `/home/yotam/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner /home/yotam/nalgebra/target/miri/x86_64-unknown-linux-gnu/debug/deps/lib-85cd9506b7860f9a 'edition::swap'` (exit status: 1)
note: test exited abnormally; to see the full output pass --nocapture to the harness.

My fix is possibly over-conservative, but I couldn't think of anything else that wouldn't be a breaking change. Granted, for a user to be affected by the change would require that they implemented the unsafe RawStorageMut trait on their own type, and overrode the default get_address_unchecked_linear_mut in a weird way. Still, I think it's better to err on the cautious side.

(BTW, a lot of the tests in edition.rs are currently crashing under miri due to UB, I'm investigating how to fix the rest of the issues but they seem less straight-forward than this one)

yotamofek avatar Nov 13 '23 10:11 yotamofek