Skip to content

Commit

Permalink
fixup! solved multiple access issue
Browse files Browse the repository at this point in the history
- Unregister instance after moving, thus avoiding reusing an invalidated instance pointer later on.
  Note: this implementation is rather hacky, because in hopes that the function clear_instance()
        inlined in detail/class.h will be available in cast.h as well.
        A clean solution should move the corresponding code into a shared header.
        Not sure also, I should clear_instance() or only deregister_instance()?

- (Partially) reverts 8b45197
  Moving the same variable twice into a function will error in C++ as well.
  We don't need to catch that in Python, do we?
  • Loading branch information
rhaschke committed Dec 10, 2020
1 parent 8b45197 commit 3ab9655
Showing 1 changed file with 28 additions and 13 deletions.
41 changes: 28 additions & 13 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail)

/// HACK: Declare inline function defined in class.h to be called in move_only_holder_caster's destructor
/// Correct solution would be to move that function into a common header
void clear_instance(PyObject *self);

/// A life support system for temporary objects created by `type_caster::load()`.
/// Adding a patient will keep it alive up until the enclosing function returns.
class loader_life_support {
Expand Down Expand Up @@ -588,9 +592,23 @@ class type_caster_generic {

// Base methods for generic caster; they are overridden in copyable_holder_caster
void load_value(value_and_holder &&v_h) {
value = v_h.value_ptr();
if (value == nullptr)
throw cast_error("Invalid object instance");
auto *&vptr = v_h.value_ptr();
// Lazy allocation for unallocated values:
if (vptr == nullptr) {
auto *type = v_h.type ? v_h.type : typeinfo;
if (type->operator_new) {
vptr = type->operator_new(type->type_size);
} else {
#if defined(__cpp_aligned_new) && (!defined(_MSC_VER) || _MSC_VER >= 1912)
if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
vptr = ::operator new(type->type_size,
std::align_val_t(type->type_align));
else
#endif
vptr = ::operator new(type->type_size);
}
}
value = vptr;
}
bool try_implicit_casts(handle src, bool convert) {
for (auto &cast : typeinfo->implicit_casts) {
Expand Down Expand Up @@ -1564,6 +1582,12 @@ struct move_only_holder_caster : public type_caster_base<type> {
using base::typeinfo;
using base::value;

~move_only_holder_caster() {
// if held object was actually moved, unregister it
if (!holder_helper<holder_type>::get(*holder_ptr))
clear_instance(reinterpret_cast<PyObject*>(v_h.inst));
}

bool load(handle& src, bool convert) {
bool success = base::template load_impl<move_only_holder_caster<type, holder_type>>(src, convert);
if (success) // On success, remember src pointer to withdraw later
Expand All @@ -1578,16 +1602,7 @@ struct move_only_holder_caster : public type_caster_base<type> {
#if !defined(__ICC) && !defined(__INTEL_COMPILER)
explicit
#endif
operator holder_type&&() {
// In load_value() value_ptr was still valid. If it's invalid now, another argument consumed the same value before.
if (!v_h.value_ptr())
throw cast_error("Multiple access to moved argument");
v_h.value_ptr() = nullptr;
// TODO: release object instance in python
// clear_instance(src_handle->ptr()); ???

return std::move(*holder_ptr);
}
operator holder_type&&() { return std::move(*holder_ptr); }

static handle cast(const holder_type &src, return_value_policy, handle) {
const auto *ptr = holder_helper<holder_type>::get(src);
Expand Down

0 comments on commit 3ab9655

Please sign in to comment.