From 92e515298fb883c103400eb388c3f3dbe970aaa1 Mon Sep 17 00:00:00 2001 From: cliffburdick Date: Mon, 20 May 2024 14:23:30 -0700 Subject: [PATCH 1/6] Added check on iterator end position --- include/pybind11/detail/class.h | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 0b9ea42db6..fe6b94a920 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -388,15 +388,19 @@ inline void clear_patients(PyObject *self) { auto *instance = reinterpret_cast(self); auto &internals = get_internals(); auto pos = internals.patients.find(self); - assert(pos != internals.patients.end()); - // Clearing the patients can cause more Python code to run, which - // can invalidate the iterator. Extract the vector of patients - // from the unordered_map first. - auto patients = std::move(pos->second); - internals.patients.erase(pos); - instance->has_patients = false; - for (PyObject *&patient : patients) { - Py_CLEAR(patient); + + if (pos != internals.patients.end()) { + // Clearing the patients can cause more Python code to run, which + // can invalidate the iterator. Extract the vector of patients + // from the unordered_map first. + auto patients = std::move(pos->second); + internals.patients.erase(pos); + instance->has_patients = false; + for (PyObject *&patient : patients) { + Py_CLEAR(patient); + } + } else { + assert(pos != internals.patients.end()); } } From 0b81e674313a971cce7be026bc5c901d60d27f8c Mon Sep 17 00:00:00 2001 From: cliffburdick Date: Sun, 26 May 2024 08:26:57 -0700 Subject: [PATCH 2/6] Always use assert without conditional check --- include/pybind11/detail/class.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index fe6b94a920..c7f91862ec 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -389,6 +389,7 @@ inline void clear_patients(PyObject *self) { auto &internals = get_internals(); auto pos = internals.patients.find(self); + assert(pos != internals.patients.end()); if (pos != internals.patients.end()) { // Clearing the patients can cause more Python code to run, which // can invalidate the iterator. Extract the vector of patients @@ -399,8 +400,6 @@ inline void clear_patients(PyObject *self) { for (PyObject *&patient : patients) { Py_CLEAR(patient); } - } else { - assert(pos != internals.patients.end()); } } From 895bb4b30be684f0304cebd8c816a93ac3a133aa Mon Sep 17 00:00:00 2001 From: cliffburdick Date: Mon, 27 May 2024 12:36:09 -0700 Subject: [PATCH 3/6] Addressing code review comments --- include/pybind11/detail/class.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index c7f91862ec..429fea5054 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -401,6 +401,9 @@ inline void clear_patients(PyObject *self) { Py_CLEAR(patient); } } + else { + pybind11_fail("FATAL: Internal consistency check failed: Invalid clear_patients() call."); + } } /// Clears all internal data from the instance and removes it from registered instances in From 3ce2db263f93c7f24c58c109b9c4718c668a4a52 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 27 May 2024 19:36:36 +0000 Subject: [PATCH 4/6] style: pre-commit fixes --- include/pybind11/detail/class.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 429fea5054..3a3ae505c4 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -400,8 +400,7 @@ inline void clear_patients(PyObject *self) { for (PyObject *&patient : patients) { Py_CLEAR(patient); } - } - else { + } else { pybind11_fail("FATAL: Internal consistency check failed: Invalid clear_patients() call."); } } From f0953165cf1f8f1245910ccabfb236a44a9d5c46 Mon Sep 17 00:00:00 2001 From: cliffburdick Date: Mon, 27 May 2024 13:00:32 -0700 Subject: [PATCH 5/6] Remove assert and throw --- include/pybind11/detail/class.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 3a3ae505c4..0e475faad8 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -389,7 +389,6 @@ inline void clear_patients(PyObject *self) { auto &internals = get_internals(); auto pos = internals.patients.find(self); - assert(pos != internals.patients.end()); if (pos != internals.patients.end()) { // Clearing the patients can cause more Python code to run, which // can invalidate the iterator. Extract the vector of patients From 1c1a9c2d999c1da02d2995205fa51a41bf4fffac Mon Sep 17 00:00:00 2001 From: cliffburdick Date: Mon, 27 May 2024 21:23:25 -0700 Subject: [PATCH 6/6] Changed style slightly --- include/pybind11/detail/class.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 0e475faad8..73f30477fb 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -389,19 +389,19 @@ inline void clear_patients(PyObject *self) { auto &internals = get_internals(); auto pos = internals.patients.find(self); - if (pos != internals.patients.end()) { - // Clearing the patients can cause more Python code to run, which - // can invalidate the iterator. Extract the vector of patients - // from the unordered_map first. - auto patients = std::move(pos->second); - internals.patients.erase(pos); - instance->has_patients = false; - for (PyObject *&patient : patients) { - Py_CLEAR(patient); - } - } else { + if (pos == internals.patients.end()) { pybind11_fail("FATAL: Internal consistency check failed: Invalid clear_patients() call."); } + + // Clearing the patients can cause more Python code to run, which + // can invalidate the iterator. Extract the vector of patients + // from the unordered_map first. + auto patients = std::move(pos->second); + internals.patients.erase(pos); + instance->has_patients = false; + for (PyObject *&patient : patients) { + Py_CLEAR(patient); + } } /// Clears all internal data from the instance and removes it from registered instances in