C++ object is destructed before it can be used, when returned as a shared_ptr and using default holder type

[tadeu]

Issue description

When returning a shared_ptr to a bound object from a C++ function, the object seems to be immediately destructed, although it continues to be "usable" from Python, but this is dangerous, because it is a "dangling" object.

What I think would be more appropriate in this case:

1. Give an error message stating that a function may not return shared_ptr of an object when it's holder type is the default one.
2. If that is not possible, at least let the function return None, this is way less dangerous.

Note that the object ends up being destructed twice!

Reproducible example code

#include <pybind11/pybind11.h>
#include <memory>
#include <iostream>

namespace py = pybind11;

class BindingsTest {
public:
    static size_t constructor_called;
    static size_t destructor_called;

    BindingsTest() { constructor_called += 1; status = "OK";  }
    ~BindingsTest() { destructor_called += 1; status = "INVALID"; }
    BindingsTest(BindingsTest const&) = delete;
    const char* get_status() const { return status; }

protected:
    const char* status;
    bool destructor_already_called;
};

size_t BindingsTest::constructor_called = 0;
size_t BindingsTest::destructor_called = 0;

std::shared_ptr<BindingsTest> create_bindings_test() {
    return std::make_shared<BindingsTest>();
}

size_t get_constructor_called() { return BindingsTest::constructor_called; }
size_t get_destructor_called() { return BindingsTest::destructor_called; }
void reset_called() {
    BindingsTest::constructor_called = 0;
    BindingsTest::destructor_called = 0;
}

PYBIND11_MODULE(_pb11pg, m) {
    m.doc() = "pybind11 testing module";

    py::class_<BindingsTest>(m, "BindingsTest")
        .def("get_status", &BindingsTest::get_status);
    m.def("create_bindings_test", &create_bindings_test);
    m.def("reset_called", &reset_called);
    m.def("get_constructor_called", &get_constructor_called);
    m.def("get_destructor_called", &get_destructor_called);
}

def test_bindings_problem():
    import _pb11pg
    _pb11pg.reset_called()

    x = _pb11pg.create_bindings_test()

    # Destructor was already called here
    assert x.get_status() == 'OK'
    assert _pb11pg.get_constructor_called() == 1
    assert _pb11pg.get_destructor_called() == 0

    x = None
    assert _pb11pg.get_constructor_called() == 1
    assert _pb11pg.get_destructor_called() == 1

Workaround

Use shared_ptr as a holder type, but the problem is that it is very hard to find that this is the issue causing the segfaults and unexpected behaviours:

py::class_<BindingsTest, shared_ptr<BindingsTest>> instead of py::class_<BindingsTest>

[EricCousineau-TRI]

I believe the problem might actually be that you're mixing holder types (where py::class_<BindingsTest> = py::class_<BindingsTest, std::unique_ptr<BindingsTest>>). See #1138 and #1161 for more details.

From what I understand, py::class_ stores the holder in a type-erased fashion, and upon casting to/from a possible holder type (e.g. unique_ptr or shared_ptr), it will not check the types, and effectively do a bad reinterpret_cast, interpreting memory allocated for unique_ptr as shared_ptr.
#1161 could provide a mechanism to detect this.