Fix a long-standing bug in the handling of Python multiple inheritance (#4762)

* Equivalent of 5718e4d080

* Resolve clang-tidy errors.

* Moving test_PPCCInit() first changes the behavior!

* Resolve new Clang dev C++11 errors:

```
The CXX compiler identification is Clang 17.0.0
```

```
pytypes.h:1615:23: error: identifier '_s' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator]
```

```
cast.h:1380:26: error: identifier '_a' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator]
```

* Resolve gcc 4.8.5 error:

```
pytypes.h:1615:12: error: missing space between '""' and suffix identifier
```

* Specifically exclude `__clang__`

* Snapshot of debugging code (does NOT pass pre-commit checks).

* Revert "Snapshot of debugging code (does NOT pass pre-commit checks)."

This reverts commit 1d4f9ff263.

* [ci skip] Order Dependence Demo

* Revert "[ci skip] Order Dependence Demo"

This reverts commit d37b5409d4.

* One way to deal with the order dependency issue. This is not the best way, more like a proof of concept.

* Move test_PC() first again.

* Add `all_type_info_add_base_most_derived_first()`, use in `all_type_info_populate()`

* Revert "One way to deal with the order dependency issue. This is not the best way, more like a proof of concept."

This reverts commit eb09c6c1b9.

* clang-tidy fixes (automatic)

* Add `is_redundant_value_and_holder()` and use to avoid forcing `__init__` overrides when they are not needed.

* 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.

* Fix actual undefined behavior exposed by previous changes.

It turns out the previous commit message is incorrect, the `inst` pointer is actually dereferenced, in the `value_and_holder` ctor here:

f3e0602802/include/pybind11/detail/type_caster_base.h (L262-L263)

```
259     // Main constructor for a found value/holder:
260     value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index)
261         : inst{i}, index{index}, type{type},
262           vh{inst->simple_layout ? inst->simple_value_holder
263                                  : &inst->nonsimple.values_and_holders[vpos]} {}
```

* Add test_mock_new()

* Experiment: specify indirect bases

* Revert "Experiment: specify indirect bases"

This reverts commit 4f90d85f9f.

* Add `all_type_info_check_for_divergence()` and some tests.

* Call `all_type_info_check_for_divergence()` also from `type_caster_generic::load_impl<>`

* Resolve clang-tidy error:

```
include/pybind11/detail/type_caster_base.h:795:21: error: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty,-warnings-as-errors]
                if (matching_bases.size() != 0) {
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
                    !matching_bases.empty()
```

* Revert "Resolve clang-tidy error:"

This reverts commit df27188dc6.

* Revert "Call `all_type_info_check_for_divergence()` also from `type_caster_generic::load_impl<>`"

This reverts commit 5f5fd6a68e.

* Revert "Add `all_type_info_check_for_divergence()` and some tests."

This reverts commit 0a9599f775.
This commit is contained in:
Ralf W. Grosse-Kunstleve 2023-11-08 12:44:04 -08:00 committed by GitHub
parent 2c35fde389
commit e250155afa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 140 additions and 12 deletions

View File

@ -189,12 +189,10 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P
return nullptr;
}
// This must be a pybind11 instance
auto *instance = reinterpret_cast<detail::instance *>(self);
// Ensure that the base __init__ function(s) were called
for (const auto &vh : values_and_holders(instance)) {
if (!vh.holder_constructed()) {
values_and_holders vhs(self);
for (const auto &vh : vhs) {
if (!vh.holder_constructed() && !vhs.is_redundant_value_and_holder(vh)) {
PyErr_Format(PyExc_TypeError,
"%.200s.__init__() must be called when overriding __init__",
get_fully_qualified_tp_name(vh.type->type).c_str());

View File

@ -102,8 +102,22 @@ public:
inline std::pair<decltype(internals::registered_types_py)::iterator, bool>
all_type_info_get_cache(PyTypeObject *type);
// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762.
inline void all_type_info_add_base_most_derived_first(std::vector<type_info *> &bases,
type_info *addl_base) {
for (auto it = bases.begin(); it != bases.end(); it++) {
type_info *existing_base = *it;
if (PyType_IsSubtype(addl_base->type, existing_base->type) != 0) {
bases.insert(it, addl_base);
return;
}
}
bases.push_back(addl_base);
}
// Populates a just-created cache entry.
PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector<type_info *> &bases) {
assert(bases.empty());
std::vector<PyTypeObject *> check;
for (handle parent : reinterpret_borrow<tuple>(t->tp_bases)) {
check.push_back((PyTypeObject *) parent.ptr());
@ -136,7 +150,7 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector<type_
}
}
if (!found) {
bases.push_back(tinfo);
all_type_info_add_base_most_derived_first(bases, tinfo);
}
}
} else if (type->tp_bases) {
@ -322,18 +336,29 @@ public:
explicit values_and_holders(instance *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 {
private:
instance *inst = nullptr;
const type_vec *types = nullptr;
value_and_holder curr;
friend struct values_and_holders;
iterator(instance *inst, const type_vec *tinfo)
: inst{inst}, types{tinfo},
curr(inst /* instance */,
types->empty() ? nullptr : (*types)[0] /* type info */,
iterator(instance *inst, const type_vec *tinfo) : inst{inst}, types{tinfo} {
if (inst != nullptr) {
assert(!types->empty());
curr = value_and_holder(
inst /* instance */,
(*types)[0] /* type info */,
0, /* vpos: (non-simple types only): the first vptr comes first */
0 /* index */) {}
0 /* index */);
}
}
// Past-the-end iterator:
explicit iterator(size_t end) : curr(end) {}
@ -364,6 +389,16 @@ public:
}
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;
}
};
/**

View File

@ -144,6 +144,7 @@ set(PYBIND11_TEST_FILES
test_opaque_types
test_operator_overloading
test_pickling
test_python_multiple_inheritance
test_pytypes
test_sequences_and_iterators
test_smart_ptr

View File

@ -1,3 +1,5 @@
from unittest import mock
import pytest
import env
@ -203,6 +205,18 @@ def test_inheritance_init(msg):
assert msg(exc_info.value) == expected
@pytest.mark.parametrize(
"mock_return_value", [None, (1, 2, 3), m.Pet("Polly", "parrot"), m.Dog("Molly")]
)
def test_mock_new(mock_return_value):
with mock.patch.object(
m.Pet, "__new__", return_value=mock_return_value
) as mock_new:
obj = m.Pet("Noname", "Nospecies")
assert obj is mock_return_value
mock_new.assert_called_once_with(m.Pet, "Noname", "Nospecies")
def test_automatic_upcasting():
assert type(m.return_class_1()).__name__ == "DerivedClass1"
assert type(m.return_class_2()).__name__ == "DerivedClass2"

View File

@ -0,0 +1,45 @@
#include "pybind11_tests.h"
namespace test_python_multiple_inheritance {
// Copied from:
// https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python_multiple_inheritance.h
struct CppBase {
explicit CppBase(int value) : base_value(value) {}
int get_base_value() const { return base_value; }
void reset_base_value(int new_value) { base_value = new_value; }
private:
int base_value;
};
struct CppDrvd : CppBase {
explicit CppDrvd(int value) : CppBase(value), drvd_value(value * 3) {}
int get_drvd_value() const { return drvd_value; }
void reset_drvd_value(int new_value) { drvd_value = new_value; }
int get_base_value_from_drvd() const { return get_base_value(); }
void reset_base_value_from_drvd(int new_value) { reset_base_value(new_value); }
private:
int drvd_value;
};
} // namespace test_python_multiple_inheritance
TEST_SUBMODULE(python_multiple_inheritance, m) {
using namespace test_python_multiple_inheritance;
py::class_<CppBase>(m, "CppBase")
.def(py::init<int>())
.def("get_base_value", &CppBase::get_base_value)
.def("reset_base_value", &CppBase::reset_base_value);
py::class_<CppDrvd, CppBase>(m, "CppDrvd")
.def(py::init<int>())
.def("get_drvd_value", &CppDrvd::get_drvd_value)
.def("reset_drvd_value", &CppDrvd::reset_drvd_value)
.def("get_base_value_from_drvd", &CppDrvd::get_base_value_from_drvd)
.def("reset_base_value_from_drvd", &CppDrvd::reset_base_value_from_drvd);
}

View File

@ -0,0 +1,35 @@
# Adapted from:
# https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python/python_multiple_inheritance_test.py
from pybind11_tests import python_multiple_inheritance as m
class PC(m.CppBase):
pass
class PPCC(PC, m.CppDrvd):
pass
def test_PC():
d = PC(11)
assert d.get_base_value() == 11
d.reset_base_value(13)
assert d.get_base_value() == 13
def test_PPCC():
d = PPCC(11)
assert d.get_drvd_value() == 33
d.reset_drvd_value(55)
assert d.get_drvd_value() == 55
assert d.get_base_value() == 11
assert d.get_base_value_from_drvd() == 11
d.reset_base_value(20)
assert d.get_base_value() == 20
assert d.get_base_value_from_drvd() == 20
d.reset_base_value_from_drvd(30)
assert d.get_base_value() == 30
assert d.get_base_value_from_drvd() == 30