Having a class which holds the same pointer inside can crash

[novoselov-ab]

Issue description

Having a class which holds the same pointer inside can crash.
The essence of the problem is that register_instance and deregister_instance search all instances with the same pointer and then select with matching type: Py_TYPE(self) == Py_TYPE(it->second). In certain situations there could be multiple instance of the same type and same pointer.

Reproducible example code

This class can cause a problem:

    struct SamePointer {};
    static SamePointer samePointer;

    py::class_<SamePointer, std::unique_ptr<SamePointer, py::nodelete>>(m, "SamePointer").def(py::init<>([&]() {
        return &samePointer;
    }));

To reproduce you need regular simple class:

    struct Empty {};
    py::class_<Empty> (m, "Empty")
        .def(py::init<>());

And this python code

import m
x = []
for _ in range(100):
    x.append(m.SamePointer())

for j in range(2):
    for i in range(100):
        if i % 2 == j:
            x[i] = m.Empty()

Creating other python classes will reuse the same python object, which was meant to be freed by python, but still considered valid by pybind.
This line will be triggered pybind11_fail("pybind11_object_dealloc(): Tried to deallocate unregistered instance!");

---

Maybe it is a bad idea to write a class this way, but it was non obvious and took a lot of time to catch a failure and debug.

[Blistic]

I hit a slight variant of this problem myself when returning cached shared_ptr's of pybind11 objects. I fixed the issue by changing the deregister_instance_impl if condition to check the exact pointer address itself as well as the type:

if (Py_TYPE(self) == Py_TYPE(it->second) && self == it->second) {
  registered_instances.erase(it);
  return true;
} 

After making this change it fixes my version of the problem, as well as the reproducible example code of @novoselov-ab and all of the other pybind11 tests pass on my system. The type check was explicitly put in by @jagerman in commit 1f8a100 without an address equality check so I assume there is a good reason for checking just the type equality that I do not properly understand? I just cannot find anything that breaks on my system when I use the more specific address equality check, and it fixes the current issue.

[jagerman]

If I'm remembering correctly, the issue here is that the address equality check will break multiple inheritance because a pointer to the base instance won't necessarily have the same address as the pointer to the derived instance; consider:

#include <iostream>

class Base1 { int b1; };
class Base2 { int b2; };
class Derived : public Base1, public Base2 {};

int main() {
    Derived x;
    std::cout << "Derived @ " << &x << "\n";
    std::cout << "Base1   @ " << static_cast<Base1 *>(&x) << "\n";
    std::cout << "Base2   @ " << static_cast<Base2 *>(&x) << "\n";
}

results in:

Derived @ 0x7ffea7807b98
Base1   @ 0x7ffea7807b98
Base2   @ 0x7ffea7807b9c

[Blistic]

Thank you for the quick reply @jagerman. From what I understand, the traverse_offset_bases function is the main area that would cause problems with multiple inheritance and this proposed fix, but in that function the type of self remains instance * and the same self pointer is recursively passed into the deregister_instance_impl function, so I think that the address of all the parents self would remain comparable.

I added prints to the deregister_instance_impl function to test a multiple inheritance case based on your suggestion, and the addresses of all of the parent classes are identical to each other and the derived class, and they are correctly deregistered. I am probably missing some subtleties, but for example the code below runs fine with the proposed fix:

test.cpp

#include <pybind11/pybind11.h>

class Base1 { int b1; };
class Base2 { int b2; };  
class Derived : public Base1, public Base2 { int d; };

PYBIND11_MODULE(test, m) {
  namespace py = pybind11;

  py::class_<Base1, std::shared_ptr<Base1>>(m, "Base1").def(py::init<>());

  py::class_<Base2, std::shared_ptr<Base2>>(m, "Base2").def(py::init<>());

  py::class_<Derived, Base1, Base2, std::shared_ptr<Derived>>(
      m, "Derived", py::multiple_inheritance())
      .def(py::init<>());
}

test.py

from test import Derived

derived = Derived()

Do you think there is a more complicated multiple inheritance situation in which the proposed fix would cause a problem, and that is not covered in the multiple inheritance tests?

[novoselov-ab]

I feel like this one is related: #1238

[eacousineau]

I feel like this one is related: #1238

Yup, def. related! Cross-posted in your PR too - thanks!

[henryiii]

Fixed in the upcoming 2.6!