Pybind incorrectly deregisters instances (by type comparison), yields errors when C++ recycles memory

[EricCousineau-TRI]

I ran into an interesting (read: Heisen) bug that occurred when I had code that looked like this:

obj = m.UniquePtrHeld(1)
m.unique_ptr_terminal(obj)  # Destroys the value held by `obj`
obj = m.UniquePtrHeld(1)

After C++ destroyed the first instance, the second instance was allocated into the same address, when casting the instance back, pybind registered a new instance (in get_internals().registered_instances) when the same ptr-value, but a different instance.
In deregister_instance_impl, it finds all values that match the given ptr, and only checks if the Py_TYPE is the same.

The fix in this case is just to compare instances (if there wasn't a strong reason for getting the type). An example change:
https://github.com/EricCousineau-TRI/pybind11/blob/feature/unique_ptr_arg_pr1-wip/include/pybind11/detail/class.h#L225

Even though I encountered this for my unique_ptr PRs, I believe it could still be an issue when returning bare pointers, say:

// c++
struct MyType { int value; }
...
m.def("do_something", [](MyType *in) {
  delete in;
  return new MyType{10};
});

# python
obj = m.MyType(10)
obj = do_something(obj);

[jagerman]

I'm not entirely sure I see what the issue is. Is it that there are two entries in registered_instances, but that the old one is no longer valid? It sounds like that old instance needs to be deleted at some earlier point.

As for the proposed fix, there is a strong reason for the type comparison, namely that two separate objects can share the same address, typically when one object is the first member of a class (or first member of a first member, etc.).

In other words, something like this can trigger it:

struct A { /* ... */ };
struct B {
    A a;
    /* ... */
};
py::class_<A>(m, "A");
py::class_<B>(m, "B")
    .def("a", [](B &self) { return &self.a; }, py::return_value_policy::reference_internal);

in which case both the A and B instances will share the same address, hence the check also on the type for all registered instance handling. In theory, you could probably delete all of the instances when deleting the outermost object (e.g. the "B"), but I don't think there's a trivial way to figure out which one is the outermost.

[EricCousineau-TRI]

To preface, this is for my unique_ptr PR, so it is not a behavior that'd normally occur in pybind (unless ownership transfer flags are introduced, per #1226 / #1200).

namely that two separate objects can share the same address...

Ah, that makes sense! Thank you! I guess a slightly more costly solution then is to compare instance and type.

Is it that there are two entries in registered_instances, but that the old one is no longer valid?

Yup!

It sounds like that old instance needs to be deleted at some earlier point.

That is true, but that would require the user to intervene, del obj (and call gc.collect() for PyPy), whereas the comparison in deregister_instances_impl could be changed and solve it always (in ownership transfer situations, that is).