From 864ed1120c704bf27d6ad5ab40645262d0d1e955 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 6 Oct 2022 16:11:34 -0400 Subject: [PATCH 1/4] chore: steal arg_v.value from copied arg in unpacking_collector (#4219) --- include/pybind11/cast.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index a0e32281b..b89e4946a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1545,7 +1545,7 @@ private: throw cast_error_unable_to_convert_call_arg(a.name, a.type); #endif } - m_kwargs[a.name] = a.value; + m_kwargs[a.name] = std::move(a.value); } void process(list & /*args_list*/, detail::kwargs_proxy kp) { From 6cb214748d6ec3a61fdc35ee27f9a90d170dd242 Mon Sep 17 00:00:00 2001 From: Axel Huebl Date: Thu, 6 Oct 2022 21:02:57 -0700 Subject: [PATCH 2/4] fix: NVCC 11.4.0 - 11.8.0 host bug workaround (#4220) * Work-Around: NVCC 11.4.0 - 11.8.0 Adds a targeted NVCC work around for limited number of CUDA releases. Fixed in NVCC development. * style: pre-commit fixes * CI: Bump CTK Version 11.2 -> 11.7 Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 4 ++-- include/pybind11/pybind11.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe57e7ce9..99caabf09 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -324,8 +324,8 @@ jobs: # Testing NVCC; forces sources to behave like .cu files cuda: runs-on: ubuntu-latest - name: "🐍 3.8 • CUDA 11.2 • Ubuntu 20.04" - container: nvidia/cuda:11.2.2-devel-ubuntu20.04 + name: "🐍 3.10 • CUDA 11.7 • Ubuntu 22.04" + container: nvidia/cuda:11.7.0-devel-ubuntu22.04 steps: - uses: actions/checkout@v3 diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c889dc416..3f6e27b75 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1578,6 +1578,22 @@ public: return *this; } +// Nvidia's NVCC is broken between 11.4.0 and 11.8.0 +// https://github.com/pybind/pybind11/issues/4193 +#if defined(__CUDACC__) && (__CUDACC_VER_MAJOR__ == 11) && (__CUDACC_VER_MINOR__ >= 4) \ + && (__CUDACC_VER_MINOR__ <= 8) + template + class_ &def(const T &op, const Extra &...extra) { + op.execute(*this, extra...); + return *this; + } + + template + class_ &def_cast(const T &op, const Extra &...extra) { + op.execute_cast(*this, extra...); + return *this; + } +#else template class_ &def(const detail::op_ &op, const Extra &...extra) { op.execute(*this, extra...); @@ -1589,6 +1605,7 @@ public: op.execute_cast(*this, extra...); return *this; } +#endif template class_ &def(const detail::initimpl::constructor &init, const Extra &...extra) { From 4a4215620920ee659145cd34597e14f0553a73f1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 7 Oct 2022 09:20:38 -0700 Subject: [PATCH 3/4] 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. --- tests/test_eigen.cpp | 39 ++++++++++++++++++++++++++++++++++----- tests/test_eigen.py | 15 ++++++++++++--- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 591dacc62..b0c7bdb48 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -197,11 +197,40 @@ 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) { + // 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) { + 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 9c6485de3..713432a81 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 7c6f2f80a73e330642226138ae3ddaf448a4f672 Mon Sep 17 00:00:00 2001 From: Daniel Galvez Date: Fri, 7 Oct 2022 12:27:54 -0700 Subject: [PATCH 4/4] fix: PyCapsule_GetDestructor is allowed to return a nullptr destructor (#4221) * fix: PyCapsule_GetDestructor is allowed to return a nullptr destructor Previously, this code would error out if the destructor happened to be a nullptr. This is incorrect. nullptrs are allowed for capsule destructors. "It is legal for a capsule to have a NULL destructor. This makes a NULL return code somewhat ambiguous; use PyCapsule_IsValid() or PyErr_Occurred() to disambiguate." See: https://docs.python.org/3/c-api/capsule.html#c.PyCapsule_GetDestructor I noticed this while working on a type caster related to #3858 DLPack happens to allow the destructor not to be defined on a capsule, and I encountered such a case. See: https://github.com/dmlc/dlpack/blob/e2bdd3bee8cb6501558042633fa59144cc8b7f5f/include/dlpack/dlpack.h#L219 * Add test for the fix. * Update tests/test_pytypes.cpp I tried this locally and it works! I never knew that there are cases where `reinterpret_cast` does not work but `static_cast` does. Let's see if all compilers are happy with this. Co-authored-by: Aaron Gokaslan * style: pre-commit fixes Co-authored-by: Ralf W. Grosse-Kunstleve Co-authored-by: Ralf W. Grosse-Kunstleve Co-authored-by: Aaron Gokaslan Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- include/pybind11/pytypes.h | 12 ++++++------ tests/test_pytypes.cpp | 6 ++++++ tests/test_pytypes.py | 11 +++++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 565df4375..29506b7ea 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1829,18 +1829,18 @@ public: // guard if destructor called while err indicator is set error_scope error_guard; auto destructor = reinterpret_cast(PyCapsule_GetContext(o)); - if (destructor == nullptr) { - if (PyErr_Occurred()) { - throw error_already_set(); - } - pybind11_fail("Unable to get capsule context"); + if (PyErr_Occurred()) { + throw error_already_set(); } const char *name = get_name_in_error_scope(o); void *ptr = PyCapsule_GetPointer(o, name); if (ptr == nullptr) { throw error_already_set(); } - destructor(ptr); + + if (destructor != nullptr) { + destructor(ptr); + } }); if (!m_ptr || PyCapsule_SetContext(m_ptr, (void *) destructor) != 0) { diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index da0dc8f6b..c95ff8230 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -289,6 +289,12 @@ TEST_SUBMODULE(pytypes, m) { return capsule; }); + m.def("return_capsule_with_explicit_nullptr_dtor", []() { + py::print("creating capsule with explicit nullptr dtor"); + return py::capsule(reinterpret_cast(1234), + static_cast(nullptr)); // PR #4221 + }); + // test_accessors m.def("accessor_api", [](const py::object &o) { auto d = py::dict(); diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index a34eaa59e..070849fc5 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -299,6 +299,17 @@ def test_capsule(capture): """ ) + with capture: + a = m.return_capsule_with_explicit_nullptr_dtor() + del a + pytest.gc_collect() + assert ( + capture.unordered + == """ + creating capsule with explicit nullptr dtor + """ + ) + def test_accessors(): class SubTestObject: