PyO3 / rust-numpy

PyO3-based Rust bindings of the NumPy C-API
BSD 2-Clause "Simplified" License
1.11k stars 106 forks source link

Reshape crashes due to wrong size in release builds #340

Closed dragly closed 2 years ago

dragly commented 2 years ago

Running the reshape example as a test fails in release mode.

Steps to reproduce

Append the following to test/array.rs:

#[test]
fn reshape() {
    Python::with_gil(|py| {
        let array = PyArray::from_iter(py, 0..9)
            .reshape_with_order([3, 3], NPY_ORDER::NPY_FORTRANORDER)
            .unwrap();

        assert_eq!(
            array.readonly().as_array(),
            array![[0, 3, 6], [1, 4, 7], [2, 5, 8]]
        );
        assert!(array.is_fortran_contiguous());

        assert!(array.reshape([5]).is_err());
    });
}

Run the following command:

cargo test --release reshape

Output:

thread 'reshape' panicked at 'called `Result::unwrap()` on an `Err` value: PyErr { type: <class 'ValueError'>, value: ValueError('cannot reshape array of size 9 into shape (140325372964848,9)'), traceback: None }', tests/array.rs:554:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Workaround

My guess is that something goes wrong when pointing to memory that perhaps is optimized out in release mode.

The following workaround "fixes" the issue: src/array.rs:

    pub fn reshape_with_order<'py, ID: IntoDimension>(
        &'py self,
        dims: ID,
        order: NPY_ORDER,
    ) -> PyResult<&'py PyArray<T, ID::Dim>> {
        let dims = dims.into_dimension();
        let mut dims = dims.to_npy_dims();

It also seems like other tests are failing in release mode.

System information

Reproduced with

adamreichold commented 2 years ago

Thanks for reporting! I can confirm the issue and the fix. It also affects resize. I will prepare a 0.17.1 release with a fix.

Since this is a memory safety issue, I wonder whether we should yank 0.17.0 from crates.io? cc @davidhewitt

adamreichold commented 2 years ago

(We should probably add tests running under ASAN to the repository with all the unsafe code. Too bad Miri cannot be expected to handle our FFI interactions.)

davidhewitt commented 2 years ago

Was this a new bug in 0.17.0? Seems reasonable to yank if so once 0.17.1 released. Do you need me to do that?

adamreichold commented 2 years ago

Was this a new bug in 0.17.0? Seems reasonable to yank if so once 0.17.1 released. Do you need me to do that?

Yes, this was introduced during copy-editing and hence is new in 0.17.0. I have not tried yanking and will message you if it fails. Here, I first wanted to know your opinion/advice whether yanking is reasonable response.

davidhewitt commented 2 years ago

0.17.1 released and 0.17.0 yanked.

dragly commented 2 years ago

Great! Thanks for the incredibly rapid response and fix! :)