Streamline implementation and avoid unsafe reinterpret_cast<instance *>() introduced with PR #2152

The `reinterpret_cast<instance *>(self)` is unsafe if `__new__` is mocked,
which was actually found in the wild: the mock returned `None` for `self`.
This was inconsequential because `inst` is currently cast straight back to
`PyObject *` to compute `all_type_info()`, which is empty if `self` is not
a pybind11 `instance`, and then `inst` is never dereferenced. However, the
unsafe detour through `instance *` is easily avoided and the updated
implementation is less prone to accidents while debugging or refactoring.
This commit is contained in:
Ralf W. Grosse-Kunstleve 2023-07-30 23:27:50 -07:00
parent d7088364f9
commit cf5958d516
2 changed files with 19 additions and 17 deletions

View File

@ -180,17 +180,6 @@ extern "C" inline PyObject *pybind11_meta_getattro(PyObject *obj, PyObject *name
return PyType_Type.tp_getattro(obj, name); return PyType_Type.tp_getattro(obj, name);
} }
// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762.
inline bool is_redundant_value_and_holder(const std::vector<type_info *> &bases,
std::size_t ix_base) {
for (std::size_t i = 0; i < ix_base; i++) {
if (PyType_IsSubtype(bases[i]->type, bases[ix_base]->type) != 0) {
return true;
}
}
return false;
}
/// metaclass `__call__` function that is used to create all pybind11 objects. /// metaclass `__call__` function that is used to create all pybind11 objects.
extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, PyObject *kwargs) { extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, PyObject *kwargs) {
@ -201,19 +190,15 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P
} }
// Ensure that the base __init__ function(s) were called // Ensure that the base __init__ function(s) were called
const auto &bases = all_type_info((PyTypeObject *) type); values_and_holders vhs(self);
values_and_holders vhs(reinterpret_cast<detail::instance *>(self));
assert(bases.size() == vhs.size());
std::size_t ix_base = 0;
for (const auto &vh : vhs) { for (const auto &vh : vhs) {
if (!vh.holder_constructed() && !is_redundant_value_and_holder(bases, ix_base)) { if (!vh.holder_constructed() && !vhs.is_redundant_value_and_holder(vh)) {
PyErr_Format(PyExc_TypeError, PyErr_Format(PyExc_TypeError,
"%.200s.__init__() must be called when overriding __init__", "%.200s.__init__() must be called when overriding __init__",
get_fully_qualified_tp_name(vh.type->type).c_str()); get_fully_qualified_tp_name(vh.type->type).c_str());
Py_DECREF(self); Py_DECREF(self);
return nullptr; return nullptr;
} }
ix_base++;
} }
return self; return self;

View File

@ -336,6 +336,13 @@ public:
explicit values_and_holders(instance *inst) explicit values_and_holders(instance *inst)
: inst{inst}, tinfo(all_type_info(Py_TYPE(inst))) {} : inst{inst}, tinfo(all_type_info(Py_TYPE(inst))) {}
explicit values_and_holders(PyObject *obj)
: inst{nullptr}, tinfo(all_type_info(Py_TYPE(obj))) {
if (!tinfo.empty()) {
inst = reinterpret_cast<instance *>(obj);
}
}
struct iterator { struct iterator {
private: private:
instance *inst = nullptr; instance *inst = nullptr;
@ -378,6 +385,16 @@ public:
} }
size_t size() { return tinfo.size(); } size_t size() { return tinfo.size(); }
// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762.
bool is_redundant_value_and_holder(const value_and_holder &vh) {
for (size_t i = 0; i < vh.index; i++) {
if (PyType_IsSubtype(tinfo[i]->type, tinfo[vh.index]->type) != 0) {
return true;
}
}
return false;
}
}; };
/** /**