Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reshape crashes due to wrong size in release builds #340

Closed
dragly opened this issue Aug 26, 2022 · 6 comments · Fixed by #341
Closed

Reshape crashes due to wrong size in release builds #340

dragly opened this issue Aug 26, 2022 · 6 comments · Fixed by #341

Comments

@dragly
Copy link

dragly commented Aug 26, 2022

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

  • Ubuntu 20.04
  • Rust 1.63 and 1.57
  • NumPy 1.23 and 1.20
  • Python 3.10 and 3.8
@adamreichold
Copy link
Member

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
Copy link
Member

(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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

0.17.1 released and 0.17.0 yanked.

@dragly
Copy link
Author

dragly commented Aug 28, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants