shared_ptr holder constructed but not freed by pybind11

[stukowski]

I think I have found a problem with the construction/destruction of smart pointer holders. It seems like pybind11 does not always destruct the smart pointer holders it creates, because it thinks it doesn't "own" them.

Here is a small code snippet that demonstrates that behavior. It registers a class that uses std::shared_ptr as holder type. As suggested by the pybind11 documentation, std::enable_shared_from_this is used so that shared pointers and raw pointers can be used interchangeably.

	class MyClass : public std::enable_shared_from_this<MyClass> {};
	py::class_<MyClass, std::shared_ptr<MyClass>>(m, "MyClass");

	std::shared_ptr<MyClass> instance = std::make_shared<MyClass>();	
	std::cout << instance.use_count() << std::endl;	// Prints "1", that's the shared_ptr instance above.

	py::object o = py::cast(instance.get());  // Note we are passing a raw pointer to cast()
	std::cout << instance.use_count() << std::endl;	// Prints "2", we now have a 2nd shared_ptr (the holder)

	o = py::none();	// Delete Python variable
	std::cout << instance.use_count() << std::endl;	// Prints "2" (This seems wrong! The holder should have been destroyed)

Note that cast() assumes return_value_policy::reference, because we are passing it a raw pointer. Even though a new shared_ptr holder is constructed by pybind11 as part of the cast, the wrapper->owned field is set to false by type_caster_generic::cast(). This prevents the internal shared_ptr from getting freed when the wrapper is destroyed. A dangling reference to the C++ class instance remains.

The above problem can be worked around either by passing the shared_ptr to cast(), not the raw pointer. Or by explicitly passing return_value_policy::take_ownership to cast() as second argument.

However, my C++ interface that I am trying to wrap with pybind11 typically returns raw pointers and relies on the std::enable_shared_from_this mechanism. This allows constructing reference counted pointers from raw pointers only when needed. That means I need cast() to behave the same for shared pointers and raw pointers. In both cases a shared_ptr holder should be constructed and, when the Python wrapper goes out of scope, destructed again.

[wjakob]

@stukowski: you can never depend on deallocations happening at any particular moment. The only guarantee is that the shared data structure will be deallocated eventually (at the latest when the interpreter shuts down). Try adding a message to the destructor of MyClass, and I think you'll see that everything works out as expected.

Feel free to reopen this ticket if it doesn't.

[wjakob]

(and in that case, please submit a complete and self contained testcase as required per CONTRIBUTING.md)

[stukowski]

I now wrote a self-contained test case to demonstrate the issue:

test_case.zip

The code mainly consists of the snippet I posted above. I added print statements to the constructor and the destructor of the C++ class as @wjakob suggested to show that the destructor is never called. Not even when the Python interpreter terminates.

This is the output the test case produces:

MyClass constructor
1
2
2

Note that in line 29 of the test program the Python variable is deleted. I used the debugger to verify that this immediately triggers the deletion of the wrapper object (pybind11::class_::dealloc() gets called). However, this function does not delete the internal shared_ptr holder, that's why the use count remains at 2.

[dean0x7d]

I can reproduce the problem with the code in test_case.zip. The std::shared_ptr holder is never freed.

I've made an alternative test case that produces the same issue but fits into the existing pytest suit: dean0x7d@a2bbd8a. The inst->owned = true line is a possible fix -- without it, the test fails on the last assert. However, I'm really not sure if that's a good solution or if there are any side-effects.

The test case in the commit is based on the std::shared_ptr example from the documentation. The section does warn agains doing this, but it also suggests std::enable_shared_from_this as a solution, so it seems like it should work.

[jagerman]

That fix works only because inst->constructed = true, which is already there, doesn't actually do anything unless inst->owned is also true. I think it should also go into the "variant 2" init_holder_helper so that returning a shared_ptr also does the right thing.

But I think instead there's something confusing in this logic in class_::dealloc:

• if we own the pointer:
  • if we constructed a holder, invoke its destructor
  • otherwise delete it via delete

It seems to me that the logic should be:

• if we have constructed a holder around the pointer, invoke the holder's destructor
• otherwise, if we own the pointer, delete it via delete

But if I make that change I start getting double-frees. Alternatively--and effectively the same as changing the logic since this is the only place ->constructed = true is ever set--I can add ->owned = true to all three init_holder_helper variants. For the first two, everything tests fine, but when adding it to the third I start getting the same double-frees. But that suggests to me that something else is wrong: we're constructing a non-copyable holder to hold a pointer (whenever we hit variant 3, i.e. usually since unique_ptr hits this one), but, at least in some cases, it looks like we never actually invoke that holder's destructor (unless I'm missing something).

So I think the proper fix here is for class_::dealloc to invoke the holder destructor whenever inst->constructed (without requiring inst->owned): but that's currently triggering another problem related to something being fishy in the non-copyable holder case.

[dean0x7d]

The logic in class_::dealloc is fine: values with return_value_policy::reference are not owned, so calling either the destructor or delete will result in double-frees.

The issue here is specific to std::enable_shared_from_this since init_holder_helper will increment it's reference count even for return_value_policy::reference. The solution should then be either:

1. Set owned = true to correctly decrement the refcount. However, this does essentially violate return_value_policy::reference since we are taking ownership.
2. Keep owned = false but avoid incrementing the refcount in the first place. This could be done by skipping init_holder for certain return value policies. However, I'm not sure how this would play out with the different combinations of return value policies, holder types, type_caster_holder::load() and type_caster::load().

[wjakob]

The purpose of return value policies is to coordinate ownership when the object isn't reference counted and it's impossible to say (without extra information) who is responsible for deallocating it. In certain cases we might have to make a copy just to play it safe.

For shared pointers, that is not the case. We always deallocate when the reference count reaches zero, so RVPs are not needed (should probably be ignored if specified).

[wjakob]

(And this is indeed what the code does now: https://github.com/pybind/pybind11/blob/master/include/pybind11/cast.h#L918). Thus I am surprised how owned could be false.

[wjakob]

Aha! I get it now. It's because the non-shared ptr type caster gets called. Fix pending.

[wjakob]

Re: @jagerman:

Note the use of ::operator delete in dealloc(). This deletes a raw pointer without calling deconstructors. This is only needed in the case where pybind11 allocated memory for an in-place constructor, which then threw an exception.

[wjakob]

I think this should address the problem. @dean0x7d: I moved your testcases to test_issues.py/cpp and added a second inst->owner = true.