From 38f410695cddf0c402e1e22d64896321fc0a314d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 5 Oct 2022 10:06:55 -0700 Subject: [PATCH 1/5] Disable test triggering ASAN failure (to pin-point where the problem is). --- tests/test_eigen.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_eigen.py b/tests/test_eigen.py index 9c6485de3c..935ef8b95e 100644 --- a/tests/test_eigen.py +++ b/tests/test_eigen.py @@ -223,6 +223,7 @@ def test_nonunit_stride_to_python(): for i in range(-5, 7): assert np.all(m.diagonal_n(ref, i) == ref.diagonal(i)), f"m.diagonal_n({i})" + return # TODO(rwgk): Fix ASAN failure. assert np.all(m.block(ref, 2, 1, 3, 3) == ref[2:5, 1:4]) assert np.all(m.block(ref, 1, 4, 4, 2) == ref[1:, 4:]) assert np.all(m.block(ref, 1, 4, 3, 2) == ref[1:4, 4:]) From a5462edcb6b8adce71fd3959fb0ccd866c5f7e11 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 5 Oct 2022 22:49:18 -0700 Subject: [PATCH 2/5] Fix unsafe "block" implementation in test_eigen.cpp --- tests/test_eigen.cpp | 9 ++++++++- tests/test_eigen.py | 1 - 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 591dacc624..72ec52112c 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -201,7 +201,14 @@ TEST_SUBMODULE(eigen, m) { int start_row, int start_col, int block_rows, - int block_cols) { return x.block(start_row, start_col, block_rows, block_cols); }); + int block_cols) { + // In case x is a copy made in the type_caster for Eigen::Ref + // (https://pybind11.readthedocs.io/en/stable/advanced/cast/eigen.html#pass-by-reference), + // returning the Eigen::Ref returned by x.block() will lead to heap-use-after-free, + // because the block references the copy, which is destroyed when this function + // returns. Therefore the block needs to be returned by value. + return Eigen::MatrixXd{x.block(start_row, start_col, block_rows, block_cols)}; + }); // test_eigen_return_references, test_eigen_keepalive // return value referencing/copying tests: diff --git a/tests/test_eigen.py b/tests/test_eigen.py index 935ef8b95e..9c6485de3c 100644 --- a/tests/test_eigen.py +++ b/tests/test_eigen.py @@ -223,7 +223,6 @@ def test_nonunit_stride_to_python(): for i in range(-5, 7): assert np.all(m.diagonal_n(ref, i) == ref.diagonal(i)), f"m.diagonal_n({i})" - return # TODO(rwgk): Fix ASAN failure. assert np.all(m.block(ref, 2, 1, 3, 3) == ref[2:5, 1:4]) assert np.all(m.block(ref, 1, 4, 4, 2) == ref[1:, 4:]) assert np.all(m.block(ref, 1, 4, 3, 2) == ref[1:4, 4:]) From 25892834e08d2414ed886ad93cf96d0139e80539 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 6 Oct 2022 14:03:06 -0700 Subject: [PATCH 3/5] Undo changes (i.e. revert back to master). --- tests/test_eigen.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 72ec52112c..591dacc624 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -201,14 +201,7 @@ TEST_SUBMODULE(eigen, m) { int start_row, int start_col, int block_rows, - int block_cols) { - // In case x is a copy made in the type_caster for Eigen::Ref - // (https://pybind11.readthedocs.io/en/stable/advanced/cast/eigen.html#pass-by-reference), - // returning the Eigen::Ref returned by x.block() will lead to heap-use-after-free, - // because the block references the copy, which is destroyed when this function - // returns. Therefore the block needs to be returned by value. - return Eigen::MatrixXd{x.block(start_row, start_col, block_rows, block_cols)}; - }); + int block_cols) { return x.block(start_row, start_col, block_rows, block_cols); }); // test_eigen_return_references, test_eigen_keepalive // return value referencing/copying tests: From 537574e0b6196c2fc9400ee7ee3d96bb14d362d9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 6 Oct 2022 16:50:17 -0700 Subject: [PATCH 4/5] 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. --- tests/test_eigen.cpp | 37 ++++++++++++++++++++++++++++++++----- tests/test_eigen.py | 15 ++++++++++++--- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 591dacc624..1aaa1b7ec1 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -197,11 +197,38 @@ TEST_SUBMODULE(eigen, m) { // Return a block of a matrix (gives non-standard strides) m.def("block", - [](const Eigen::Ref &x, - int start_row, - int start_col, - int block_rows, - int block_cols) { return x.block(start_row, start_col, block_rows, block_cols); }); + [m](const py::object &x_obj, + int start_row, + int start_col, + int block_rows, + 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 &x, + int start_row, + int start_col, + int block_rows, + int block_cols) { + auto i0 = py::make_tuple(0, 0); + auto x0_orig = x_obj[*i0].cast(); + 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 // return value referencing/copying tests: diff --git a/tests/test_eigen.py b/tests/test_eigen.py index 9c6485de3c..713432a815 100644 --- a/tests/test_eigen.py +++ b/tests/test_eigen.py @@ -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(): assert np.all(m.diagonal(ref) == ref.diagonal()) assert np.all(m.diagonal_1(ref) == ref.diagonal(1)) 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.block(ref, 2, 1, 3, 3) == ref[2:5, 1:4]) - assert np.all(m.block(ref, 1, 4, 4, 2) == ref[1:, 4:]) - assert np.all(m.block(ref, 1, 4, 3, 2) == ref[1:4, 4:]) + # Must be order="F", otherwise the type_caster will make a copy and + # m.block() will return a dangling reference (heap-use-after-free). + 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(): From 630f2c55da08c6761528aaf99f52de4e0384c1e8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 7 Oct 2022 07:50:36 -0700 Subject: [PATCH 5/5] Add comment pointing to PR, for easy reference. --- tests/test_eigen.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 1aaa1b7ec1..b0c7bdb48a 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -213,6 +213,8 @@ TEST_SUBMODULE(eigen, m) { 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(); if (x(0, 0) != x0_orig) {