Merge pull request #455 from bennorth/bugfix/bad-delete-if-no-copy-ctor

Bugfix: bad delete if no copy ctor

include/pybind11/cast.h

@@ -258,31 +258,48 @@ public:
 
 
         auto wrapper = (instance<void> *) inst.ptr();
         auto wrapper = (instance<void> *) inst.ptr();
 
 
+        wrapper->value = nullptr;
+        wrapper->owned = false;
+
+        switch (policy) {
+        case return_value_policy::automatic:
+        case return_value_policy::take_ownership:
             wrapper->value = src;
             wrapper->value = src;
             wrapper->owned = true;
             wrapper->owned = true;
+            break;
 
 
-        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;
+        case return_value_policy::automatic_reference:
+        case return_value_policy::reference:
+            wrapper->value = src;
+            wrapper->owned = false;
+            break;
 
 
-        if (policy == return_value_policy::copy) {
+        case return_value_policy::copy:
             if (copy_constructor)
             if (copy_constructor)
-                wrapper->value = copy_constructor(wrapper->value);
+                wrapper->value = copy_constructor(src);
             else
             else
                 throw cast_error("return_value_policy = copy, but the object is non-copyable!");
                 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)
             if (move_constructor)
-                wrapper->value = move_constructor(wrapper->value);
+                wrapper->value = move_constructor(src);
             else if (copy_constructor)
             else if (copy_constructor)
-                wrapper->value = copy_constructor(wrapper->value);
+                wrapper->value = copy_constructor(src);
             else
             else
                 throw cast_error("return_value_policy = move, but the object is neither movable nor copyable!");
                 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;
             wrapper->owned = false;
             detail::keep_alive_impl(inst, parent);
             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);
         tinfo->init_holder(inst.ptr(), existing_holder);

tests/CMakeLists.txt

@@ -12,6 +12,7 @@ set(PYBIND11_TEST_FILES
   test_chrono.cpp
   test_chrono.cpp
   test_class_args.cpp
   test_class_args.cpp
   test_constants_and_functions.cpp
   test_constants_and_functions.cpp
+  test_copy_move_policies.cpp
   test_eigen.cpp
   test_eigen.cpp
   test_enum.cpp
   test_enum.cpp
   test_eval.cpp
   test_eval.cpp

tests/test_copy_move_policies.cpp

+/*
+    tests/test_copy_move_policies.cpp -- 'copy' and 'move'
+                                         return value policies
+
+    Copyright (c) 2016 Ben North <ben@redfrontdoor.org>
+
+    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 <typename derived>
+struct empty {
+    static const derived& get_one() { return instance_; }
+    static derived instance_;
+};
+
+struct lacking_copy_ctor : public empty<lacking_copy_ctor> {
+    lacking_copy_ctor() {}
+    lacking_copy_ctor(const lacking_copy_ctor& other) = delete;
+};
+
+template <> lacking_copy_ctor empty<lacking_copy_ctor>::instance_ {};
+
+struct lacking_move_ctor : public empty<lacking_move_ctor> {
+    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<lacking_move_ctor>::instance_ {};
+
+test_initializer copy_move_policies([](py::module &m) {
+    py::class_<lacking_copy_ctor>(m, "lacking_copy_ctor")
+        .def_static("get_one", &lacking_copy_ctor::get_one,
+                    py::return_value_policy::copy);
+    py::class_<lacking_move_ctor>(m, "lacking_move_ctor")
+        .def_static("get_one", &lacking_move_ctor::get_one,
+                    py::return_value_policy::move);
+});

tests/test_copy_move_policies.py

+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)
+
+