test_eigen.py test_nonunit_stride_to_python bug fix (ASAN failure) (#4217)

* Disable test triggering ASAN failure (to pin-point where the problem is).

* Fix unsafe "block" implementation in test_eigen.cpp

* Undo changes (i.e. revert back to master).

* Detect "type_caster for Eigen::Ref made a copy."

This is achieved without
* reaching into internals,
* making test_eigen.cpp depend on pybind11/numpy.h.

* Add comment pointing to PR, for easy reference.
This commit is contained in:
Ralf W. Grosse-Kunstleve 2022-10-07 09:20:38 -07:00 committed by GitHub
parent 6cb214748d
commit 4a42156209
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 46 additions and 8 deletions

View File

@ -197,11 +197,40 @@ TEST_SUBMODULE(eigen, m) {
// Return a block of a matrix (gives non-standard strides) // Return a block of a matrix (gives non-standard strides)
m.def("block", m.def("block",
[](const Eigen::Ref<const Eigen::MatrixXd> &x, [m](const py::object &x_obj,
int start_row, int start_row,
int start_col, int start_col,
int block_rows, int block_rows,
int block_cols) { return x.block(start_row, start_col, block_rows, block_cols); }); int block_cols) {
return m.attr("_block")(x_obj, x_obj, start_row, start_col, block_rows, block_cols);
});
m.def(
"_block",
[](const py::object &x_obj,
const Eigen::Ref<const Eigen::MatrixXd> &x,
int start_row,
int start_col,
int block_rows,
int block_cols) {
// See PR #4217 for background. This test is a bit over the top, but might be useful
// as a concrete example to point to when explaining the dangling reference trap.
auto i0 = py::make_tuple(0, 0);
auto x0_orig = x_obj[*i0].cast<double>();
if (x(0, 0) != x0_orig) {
throw std::runtime_error(
"Something in the type_caster for Eigen::Ref is terribly wrong.");
}
double x0_mod = x0_orig + 1;
x_obj[*i0] = x0_mod;
auto copy_detected = (x(0, 0) != x0_mod);
x_obj[*i0] = x0_orig;
if (copy_detected) {
throw std::runtime_error("type_caster for Eigen::Ref made a copy.");
}
return x.block(start_row, start_col, block_rows, block_cols);
},
py::keep_alive<0, 1>());
// test_eigen_return_references, test_eigen_keepalive // test_eigen_return_references, test_eigen_keepalive
// return value referencing/copying tests: // return value referencing/copying tests:

View File

@ -217,15 +217,24 @@ def test_negative_stride_from_python(msg):
) )
def test_block_runtime_error_type_caster_eigen_ref_made_a_copy():
with pytest.raises(RuntimeError) as excinfo:
m.block(ref, 0, 0, 0, 0)
assert str(excinfo.value) == "type_caster for Eigen::Ref made a copy."
def test_nonunit_stride_to_python(): def test_nonunit_stride_to_python():
assert np.all(m.diagonal(ref) == ref.diagonal()) assert np.all(m.diagonal(ref) == ref.diagonal())
assert np.all(m.diagonal_1(ref) == ref.diagonal(1)) assert np.all(m.diagonal_1(ref) == ref.diagonal(1))
for i in range(-5, 7): for i in range(-5, 7):
assert np.all(m.diagonal_n(ref, i) == ref.diagonal(i)), f"m.diagonal_n({i})" assert np.all(m.diagonal_n(ref, i) == ref.diagonal(i)), f"m.diagonal_n({i})"
assert np.all(m.block(ref, 2, 1, 3, 3) == ref[2:5, 1:4]) # Must be order="F", otherwise the type_caster will make a copy and
assert np.all(m.block(ref, 1, 4, 4, 2) == ref[1:, 4:]) # m.block() will return a dangling reference (heap-use-after-free).
assert np.all(m.block(ref, 1, 4, 3, 2) == ref[1:4, 4:]) rof = np.asarray(ref, order="F")
assert np.all(m.block(rof, 2, 1, 3, 3) == rof[2:5, 1:4])
assert np.all(m.block(rof, 1, 4, 4, 2) == rof[1:, 4:])
assert np.all(m.block(rof, 1, 4, 3, 2) == rof[1:4, 4:])
def test_eigen_ref_to_python(): def test_eigen_ref_to_python():