From 24a2054dbcf8bae3e00927466c1942103f9df0f6 Mon Sep 17 00:00:00 2001 From: Ben North Date: Thu, 20 Oct 2016 21:09:25 +0100 Subject: [PATCH 1/2] Fix wrapper's 'value' and 'owned' if ctor missing type_caster_generic::cast(): The values of wrapper->value wrapper->owned are incorrect in the case that a return value policy of 'copy' is requested but there is no copy-constructor. (Similarly 'move'.) In particular, if the source object is a static instance, the destructor of the 'object' 'inst' leads to class_::dealloc() which incorrectly attempts to 'delete' the static instance. This commit re-arranges the code to be clearer as to what the values of 'value' and 'owned' should be in the various cases. Behaviour is different to previous code only in two situations: policy = copy but no copy-ctor: Old code leaves 'value = src, owned = true', which leads to trouble. New code leaves 'value = nullptr, owned = false', which is correct. policy = move but no move- or copy-ctor: old code leaves 'value = src, owned = true', which leads to trouble. New code leaves 'value = nullptr, owned = false', which is correct. --- include/pybind11/cast.h | 45 ++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index f3690262..dbbeb922 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -258,31 +258,48 @@ public: auto wrapper = (instance *) inst.ptr(); - wrapper->value = src; - wrapper->owned = true; + wrapper->value = nullptr; + wrapper->owned = false; - if (policy == return_value_policy::automatic) - policy = return_value_policy::take_ownership; - else if (policy == return_value_policy::automatic_reference) - policy = return_value_policy::reference; + switch (policy) { + case return_value_policy::automatic: + case return_value_policy::take_ownership: + wrapper->value = src; + wrapper->owned = true; + break; - if (policy == return_value_policy::copy) { + case return_value_policy::automatic_reference: + case return_value_policy::reference: + wrapper->value = src; + wrapper->owned = false; + break; + + case return_value_policy::copy: if (copy_constructor) - wrapper->value = copy_constructor(wrapper->value); + wrapper->value = copy_constructor(src); else throw cast_error("return_value_policy = copy, but the object is non-copyable!"); - } else if (policy == return_value_policy::move) { + wrapper->owned = true; + break; + + case return_value_policy::move: if (move_constructor) - wrapper->value = move_constructor(wrapper->value); + wrapper->value = move_constructor(src); else if (copy_constructor) - wrapper->value = copy_constructor(wrapper->value); + wrapper->value = copy_constructor(src); else throw cast_error("return_value_policy = move, but the object is neither movable nor copyable!"); - } else if (policy == return_value_policy::reference) { - wrapper->owned = false; - } else if (policy == return_value_policy::reference_internal) { + wrapper->owned = true; + break; + + case return_value_policy::reference_internal: + wrapper->value = src; wrapper->owned = false; detail::keep_alive_impl(inst, parent); + break; + + default: + throw cast_error("unhandled return_value_policy: should not happen!"); } tinfo->init_holder(inst.ptr(), existing_holder); From bbe45082f4e40159eba3e066c66ce415152c677c Mon Sep 17 00:00:00 2001 From: Ben North Date: Thu, 20 Oct 2016 21:19:30 +0100 Subject: [PATCH 2/2] Test uncopyable static member Without the previous commit, this test generates a core dump. --- tests/CMakeLists.txt | 1 + tests/test_copy_move_policies.cpp | 41 +++++++++++++++++++++++++++++++ tests/test_copy_move_policies.py | 17 +++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 tests/test_copy_move_policies.cpp create mode 100644 tests/test_copy_move_policies.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 684535a4..7470c5d9 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,7 @@ set(PYBIND11_TEST_FILES test_chrono.cpp test_class_args.cpp test_constants_and_functions.cpp + test_copy_move_policies.cpp test_eigen.cpp test_enum.cpp test_eval.cpp diff --git a/tests/test_copy_move_policies.cpp b/tests/test_copy_move_policies.cpp new file mode 100644 index 00000000..de1c6e64 --- /dev/null +++ b/tests/test_copy_move_policies.cpp @@ -0,0 +1,41 @@ +/* + tests/test_copy_move_policies.cpp -- 'copy' and 'move' + return value policies + + Copyright (c) 2016 Ben North + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#include "pybind11_tests.h" + +template +struct empty { + static const derived& get_one() { return instance_; } + static derived instance_; +}; + +struct lacking_copy_ctor : public empty { + lacking_copy_ctor() {} + lacking_copy_ctor(const lacking_copy_ctor& other) = delete; +}; + +template <> lacking_copy_ctor empty::instance_ {}; + +struct lacking_move_ctor : public empty { + lacking_move_ctor() {} + lacking_move_ctor(const lacking_move_ctor& other) = delete; + lacking_move_ctor(lacking_move_ctor&& other) = delete; +}; + +template <> lacking_move_ctor empty::instance_ {}; + +test_initializer copy_move_policies([](py::module &m) { + py::class_(m, "lacking_copy_ctor") + .def_static("get_one", &lacking_copy_ctor::get_one, + py::return_value_policy::copy); + py::class_(m, "lacking_move_ctor") + .def_static("get_one", &lacking_move_ctor::get_one, + py::return_value_policy::move); +}); diff --git a/tests/test_copy_move_policies.py b/tests/test_copy_move_policies.py new file mode 100644 index 00000000..07f09f69 --- /dev/null +++ b/tests/test_copy_move_policies.py @@ -0,0 +1,17 @@ +import pytest + + +def test_lacking_copy_ctor(): + from pybind11_tests import lacking_copy_ctor + with pytest.raises(RuntimeError) as excinfo: + lacking_copy_ctor.get_one() + assert "the object is non-copyable!" in str(excinfo.value) + + +def test_lacking_move_ctor(): + from pybind11_tests import lacking_move_ctor + with pytest.raises(RuntimeError) as excinfo: + lacking_move_ctor.get_one() + assert "the object is neither movable nor copyable!" in str(excinfo.value) + +