From c36584e577686500842e0cf1ac04f45608bd73de Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 27 Mar 2024 20:40:14 -0600 Subject: [PATCH] Fix several sources of undefined behavior in type casters --- docs/changelog.rst | 38 ++++++ include/nanobind/eigen/dense.h | 3 + include/nanobind/eigen/sparse.h | 2 + include/nanobind/nb_cast.h | 156 +++++++++++++++++++------ include/nanobind/nb_lib.h | 9 +- include/nanobind/stl/detail/nb_array.h | 6 +- include/nanobind/stl/detail/nb_dict.h | 14 +-- include/nanobind/stl/detail/nb_list.h | 6 +- include/nanobind/stl/detail/nb_set.h | 6 +- include/nanobind/stl/optional.h | 10 +- include/nanobind/stl/pair.h | 5 + include/nanobind/stl/tuple.h | 7 ++ include/nanobind/stl/unique_ptr.h | 33 +++++- include/nanobind/stl/variant.h | 13 +-- src/nb_type.cpp | 63 +++++++--- tests/test_classes.cpp | 35 ++++++ tests/test_classes.py | 25 +++- tests/test_classes_ext.pyi.ref | 6 + tests/test_holders.cpp | 14 ++- tests/test_holders.py | 49 ++++++++ tests/test_stl.cpp | 6 + tests/test_stl.py | 11 ++ tests/test_stl_ext.pyi.ref | 2 + 23 files changed, 423 insertions(+), 96 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 280e756e..91cc013e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -195,6 +195,44 @@ noteworthy: specify the owner explicitly. The previous default (``nb::handle()``) continues to be a valid argument. +* There have been some changes to the API for type casters in order to + avoid undefined behavior in certain cases. (PR `#549 + `__). + + * Type casters that implement custom cast operators must now define a + member function template ``can_cast()``, which returns false if + ``operator cast_t()`` would raise an exception and true otherwise. + ``can_cast()`` will be called only after a successful call to + ``from_python()``, and might not be called at all if the caller of + ``operator cast_t()`` can cope with a raised exception. + (Users of the ``NB_TYPE_CASTER()`` convenience macro need not worry + about this; it produces cast operators that never raise exceptions, + and therefore provides a ``can_cast()`` that always returns true.) + + * Many type casters for container types (``std::vector``, + ``std::optional``, etc) implement their ``from_python()`` methods + by delegating to another, "inner" type caster (``T`` in these examples) + that is allocated on the stack inside ``from_python()``. Container casters + implemented in this way should make two changes in order to take advantage + of the new safety features: + + * Wrap your ``flags`` (received as an argument of the outer caster's + ``from_python`` method) in ``flags_for_local_caster()`` before + passing them to ``inner_caster.from_python()``. This allows nanobind + to prevent some casts that would produce dangling pointers or references. + + * If ``inner_caster.from_python()`` succeeds, then also verify + ``inner_caster.template can_cast()`` before you execute + ``inner_caster.operator cast_t()``. A failure of + ``can_cast()`` should be treated the same as a failure of + ``from_python()``. This avoids the possibility of an exception + being raised through the noexcept ``load_python()`` method, + which would crash the interpreter. + + The previous ``cast_flags::none_disallowed`` flag has been removed; + it existed to avoid one particular source of exceptions from a cast + operator, but ``can_cast()`` now handles that problem more generally. + * ABI version 14. .. rubric:: Footnote diff --git a/include/nanobind/eigen/dense.h b/include/nanobind/eigen/dense.h index b61abfae..c0f8dafd 100644 --- a/include/nanobind/eigen/dense.h +++ b/include/nanobind/eigen/dense.h @@ -220,6 +220,7 @@ struct type_caster && using Caster = make_caster; static constexpr auto Name = Caster::Name; template using Cast = T; + template static constexpr bool can_cast() { return true; } /// Generating an expression template from a Python object is, of course, not possible bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept = delete; @@ -250,6 +251,7 @@ struct type_caster, using NDArrayCaster = type_caster; static constexpr auto Name = NDArrayCaster::Name; template using Cast = Map; + template static constexpr bool can_cast() { return true; } NDArrayCaster caster; @@ -403,6 +405,7 @@ struct type_caster, const_name(DMapCaster::Name, MapCaster::Name); template using Cast = Ref; + template static constexpr bool can_cast() { return true; } MapCaster caster; struct Empty { }; diff --git a/include/nanobind/eigen/sparse.h b/include/nanobind/eigen/sparse.h index f0a85335..718fef2f 100644 --- a/include/nanobind/eigen/sparse.h +++ b/include/nanobind/eigen/sparse.h @@ -150,6 +150,7 @@ struct type_caster, enable_if_t>> { using SparseMatrixCaster = type_caster; static constexpr auto Name = SparseMatrixCaster::Name; template using Cast = Map; + template static constexpr bool can_cast() { return true; } bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept = delete; @@ -165,6 +166,7 @@ struct type_caster, enable_if_t; static constexpr auto Name = MapCaster::Name; template using Cast = Ref; + template static constexpr bool can_cast() { return true; } bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept = delete; diff --git a/include/nanobind/nb_cast.h b/include/nanobind/nb_cast.h index a9d49d4d..6a85272a 100644 --- a/include/nanobind/nb_cast.h +++ b/include/nanobind/nb_cast.h @@ -11,6 +11,7 @@ using Value = Value_; \ static constexpr auto Name = descr; \ template using Cast = movable_cast_t; \ + template static constexpr bool can_cast() { return true; } \ template , Value>> = 0> \ static handle from_cpp(T_ *p, rv_policy policy, cleanup_list *list) { \ @@ -38,11 +39,11 @@ enum cast_flags : uint8_t { // Passed to the 'self' argument in a constructor call (__init__) construct = (1 << 1), - // Don't accept 'None' Python objects in the base class caster - none_disallowed = (1 << 2), - - // Indicates that this cast is performed by nb::cast or nb::try_cast - manual = (1 << 3) + // Indicates that this cast is performed by nb::cast or nb::try_cast. + // This implies that objects added to the cleanup list may be + // released immediately after the caster's final output value is + // obtained, i.e., before it is used. + manual = (1 << 2), }; /** @@ -88,6 +89,51 @@ using precise_cast_t = std::conditional_t, intrinsic_t &&, intrinsic_t &>>; +/// Many type casters delegate to another caster using the pattern: +/// ~~~ .cc +/// bool from_python(handle src, uint8_t flags, cleanup_list *cl) noexcept { +/// SomeCaster c; +/// if (!c.from_python(src, flags, cl)) return false; +/// /* do something with */ c.operator T(); +/// return true; +/// } +/// ~~~ +/// This function adjusts the flags to avoid issues where the resulting T object +/// refers into storage that will dangle after SomeCaster is destroyed, and +/// causes a static assertion failure if that's not sufficient. Use it like: +/// ~~~ .cc +/// if (!c.from_python(src, flags_for_local_caster(flags), cl)) +/// return false; +/// ~~~ +/// where the template argument T is the type you plan to extract. +template +NB_INLINE uint8_t flags_for_local_caster(uint8_t flags) noexcept { + constexpr bool is_ref = std::is_pointer_v || std::is_reference_v; + if constexpr (is_base_caster_v>) { + if constexpr (is_ref) { + /* References/pointers to a type produced by implicit conversions + refer to storage owned by the cleanup_list. In a nb::cast() call, + that storage will be released before the reference can be used; + to prevent dangling, don't allow implicit conversions there. */ + if (flags & ((uint8_t) cast_flags::manual)) + flags &= ~((uint8_t) cast_flags::convert); + } + } else { + /* Any pointer produced by a non-base caster will generally point + into storage owned by the caster, which won't live long enough. + Exception: the 'char' caster produces a result that points to + storage owned by the incoming Python 'str' object, so it's OK. */ + static_assert(!is_ref || std::is_same_v, + "nanobind generally cannot produce objects that " + "contain interior pointers T* (or references T&) if " + "the pointee T is not handled by nanobind's regular " + "class binding mechanism. For example, you can write " + "a function that accepts int*, or std::vector, " + "but not std::vector."); + } + return flags; +} + template struct type_caster && !is_std_char_v>> { NB_INLINE bool from_python(handle src, uint8_t flags, cleanup_list *) noexcept { @@ -162,6 +208,7 @@ template <> struct type_caster { template <> struct type_caster { template using Cast = void *; + template static constexpr bool can_cast() { return true; } using Value = void*; static constexpr auto Name = const_name("types.CapsuleType"); explicit operator void *() { return value; } @@ -253,10 +300,15 @@ template <> struct type_caster { return PyUnicode_FromStringAndSize(&value, 1); } + template + NB_INLINE bool can_cast() const noexcept { + return std::is_pointer_v || (value && value[0] && value[1] == '\0'); + } + explicit operator const char *() { return value; } explicit operator char() { - if (value && value[0] && value[1] == '\0') + if (can_cast()) return value[0]; else throw next_overload(); @@ -270,7 +322,8 @@ template struct type_caster> { bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept { Caster c; - if (!c.from_python(src, flags, cleanup)) + if (!c.from_python(src, flags_for_local_caster(flags), cleanup) || + !c.template can_cast()) return false; value.h = src; value.p = c.operator T*(); @@ -305,13 +358,10 @@ template struct type_caster> { bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept { Caster caster; - if (!caster.from_python(src, flags, cleanup)) - return false; - try { - value = caster.operator cast_t(); - } catch (...) { + if (!caster.from_python(src, flags_for_local_caster(flags), cleanup) || + !caster.template can_cast()) return false; - } + value = caster.operator cast_t(); return true; } @@ -410,6 +460,11 @@ template struct type_caster_base : type_caster_base_tag { } } + template + bool can_cast() const noexcept { + return std::is_pointer_v || (value != nullptr); + } + operator Type*() { return value; } operator Type&() { @@ -433,58 +488,87 @@ template T cast_impl(handle h) { using Caster = detail::make_caster; + // A returned reference/pointer would usually refer into the type_caster + // object, which will be destroyed before the returned value can be used, + // so we prohibit it by default, with two exceptions that we know are safe: + // + // - If we're casting to a bound object type, the returned pointer points + // into storage owned by that object, not the type caster. Note this is + // only safe if we don't allow implicit conversions, because the pointer + // produced after an implicit conversion points into storage owned by + // a temporary object in the cleanup list, and we have to release those + // temporaries before we return. + // + // - If we're casting to const char*, the caster was provided by nanobind, + // and we know it will only accept Python 'str' objects, producing + // a pointer to storage owned by that object. + + constexpr bool is_ref = std::is_reference_v || std::is_pointer_v; static_assert( - !(std::is_reference_v || std::is_pointer_v) || - detail::is_base_caster_v || + !is_ref || + is_base_caster_v || std::is_same_v, "nanobind::cast(): cannot return a reference to a temporary."); Caster caster; bool rv; - if constexpr (Convert) { - cleanup_list cleanup(nullptr); + if constexpr (Convert && !is_ref) { + // Release the values in the cleanup list only after we + // initialize the return object, since the initialization + // might access those temporaries. + struct raii_cleanup { + cleanup_list list{nullptr}; + ~raii_cleanup() { list.release(); } + } cleanup; rv = caster.from_python(h.ptr(), ((uint8_t) cast_flags::convert) | ((uint8_t) cast_flags::manual), - &cleanup); - cleanup.release(); // 'from_python' is 'noexcept', so this always runs + &cleanup.list); + if (!rv) + detail::raise_cast_error(); + return caster.operator cast_t(); } else { rv = caster.from_python(h.ptr(), (uint8_t) cast_flags::manual, nullptr); + if (!rv) + detail::raise_cast_error(); + return caster.operator cast_t(); } - - if (!rv) - detail::raise_cast_error(); - return caster.operator detail::cast_t(); } template bool try_cast_impl(handle h, T &out) noexcept { using Caster = detail::make_caster; - static_assert(!std::is_same_v, - "nanobind::try_cast(): cannot return a reference to a temporary."); + // See comments in cast_impl above + constexpr bool is_ref = std::is_reference_v || std::is_pointer_v; + static_assert( + !is_ref || + is_base_caster_v || + std::is_same_v, + "nanobind::try_cast(): cannot return a reference to a temporary."); Caster caster; bool rv; - if constexpr (Convert) { + if constexpr (Convert && !is_ref) { cleanup_list cleanup(nullptr); rv = caster.from_python(h.ptr(), ((uint8_t) cast_flags::convert) | ((uint8_t) cast_flags::manual), - &cleanup); + &cleanup) && + caster.template can_cast(); + if (rv) { + out = caster.operator cast_t(); + } cleanup.release(); // 'from_python' is 'noexcept', so this always runs } else { - rv = caster.from_python(h.ptr(), (uint8_t) cast_flags::manual, nullptr); - } - - if (rv) { - try { - out = caster.operator detail::cast_t(); - return true; - } catch (const builtin_exception&) { } + rv = caster.from_python(h.ptr(), (uint8_t) cast_flags::manual, nullptr) && + caster.template can_cast(); + if (rv) { + out = caster.operator cast_t(); + } } - return false; + return rv; } NAMESPACE_END(detail) diff --git a/include/nanobind/nb_lib.h b/include/nanobind/nb_lib.h index d18ba6dd..5ae08a16 100644 --- a/include/nanobind/nb_lib.h +++ b/include/nanobind/nb_lib.h @@ -298,8 +298,13 @@ NB_CORE PyObject *nb_type_put_unique_p(const std::type_info *cpp_type, void *value, cleanup_list *cleanup, bool cpp_delete) noexcept; -/// Try to reliquish ownership from Python object to a unique_ptr -NB_CORE void nb_type_relinquish_ownership(PyObject *o, bool cpp_delete); +/// Try to reliquish ownership from Python object to a unique_ptr; +/// return true if successful, false if not. (Failure is only +/// possible if `cpp_delete` is true.) +NB_CORE bool nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) noexcept; + +/// Reverse the effects of nb_type_relinquish_ownership(). +NB_CORE void nb_type_restore_ownership(PyObject *o, bool cpp_delete) noexcept; /// Get a pointer to a user-defined 'extra' value associated with the nb_type t. NB_CORE void *nb_type_supplement(PyObject *t) noexcept; diff --git a/include/nanobind/stl/detail/nb_array.h b/include/nanobind/stl/detail/nb_array.h index fc7d3e49..728f9c56 100644 --- a/include/nanobind/stl/detail/nb_array.h +++ b/include/nanobind/stl/detail/nb_array.h @@ -21,12 +21,12 @@ template struct array_caster { Caster caster; bool success = o != nullptr; - if constexpr (is_base_caster_v && !std::is_pointer_v) - flags |= (uint8_t) cast_flags::none_disallowed; + flags = flags_for_local_caster(flags); if (success) { for (size_t i = 0; i < Size; ++i) { - if (!caster.from_python(o[i], flags, cleanup)) { + if (!caster.from_python(o[i], flags, cleanup) || + !caster.template can_cast()) { success = false; break; } diff --git a/include/nanobind/stl/detail/nb_dict.h b/include/nanobind/stl/detail/nb_dict.h index 42d8eff0..eb0e2dea 100644 --- a/include/nanobind/stl/detail/nb_dict.h +++ b/include/nanobind/stl/detail/nb_dict.h @@ -35,12 +35,8 @@ template struct dict_caster { Py_ssize_t size = NB_LIST_GET_SIZE(items); bool success = size >= 0; - uint8_t flags_key = flags, flags_val = flags; - - if constexpr (is_base_caster_v && !std::is_pointer_v) - flags_key |= (uint8_t) cast_flags::none_disallowed; - if constexpr (is_base_caster_v && !std::is_pointer_v) - flags_val |= (uint8_t) cast_flags::none_disallowed; + uint8_t flags_key = flags_for_local_caster(flags), + flags_val = flags_for_local_caster(flags); KeyCaster key_caster; ValCaster val_caster; @@ -49,12 +45,14 @@ template struct dict_caster { PyObject *key = NB_TUPLE_GET_ITEM(item, 0); PyObject *val = NB_TUPLE_GET_ITEM(item, 1); - if (!key_caster.from_python(key, flags_key, cleanup)) { + if (!key_caster.from_python(key, flags_key, cleanup) || + !key_caster.template can_cast()) { success = false; break; } - if (!val_caster.from_python(val, flags_val, cleanup)) { + if (!val_caster.from_python(val, flags_val, cleanup) || + !val_caster.template can_cast()) { success = false; break; } diff --git a/include/nanobind/stl/detail/nb_list.h b/include/nanobind/stl/detail/nb_list.h index bf3e0e6c..5874f7d0 100644 --- a/include/nanobind/stl/detail/nb_list.h +++ b/include/nanobind/stl/detail/nb_list.h @@ -39,11 +39,11 @@ template struct list_caster { Caster caster; bool success = o != nullptr; - if constexpr (is_base_caster_v && !std::is_pointer_v) - flags |= (uint8_t) cast_flags::none_disallowed; + flags = flags_for_local_caster(flags); for (size_t i = 0; i < size; ++i) { - if (!caster.from_python(o[i], flags, cleanup)) { + if (!caster.from_python(o[i], flags, cleanup) || + !caster.template can_cast()) { success = false; break; } diff --git a/include/nanobind/stl/detail/nb_set.h b/include/nanobind/stl/detail/nb_set.h index 435ccc60..936b75ee 100644 --- a/include/nanobind/stl/detail/nb_set.h +++ b/include/nanobind/stl/detail/nb_set.h @@ -34,11 +34,11 @@ template struct set_caster { Caster key_caster; PyObject *key; - if constexpr (is_base_caster_v && !std::is_pointer_v) - flags |= (uint8_t) cast_flags::none_disallowed; + flags = flags_for_local_caster(flags); while ((key = PyIter_Next(iter)) != nullptr) { - success &= key_caster.from_python(key, flags, cleanup); + success &= (key_caster.from_python(key, flags, cleanup) && + key_caster.template can_cast()); Py_DECREF(key); if (!success) diff --git a/include/nanobind/stl/optional.h b/include/nanobind/stl/optional.h index 3b5f9aa3..eb389448 100644 --- a/include/nanobind/stl/optional.h +++ b/include/nanobind/stl/optional.h @@ -33,16 +33,10 @@ struct type_caster> { } Caster caster; - if (!caster.from_python(src, flags, cleanup)) + if (!caster.from_python(src, flags_for_local_caster(flags), cleanup) || + !caster.template can_cast()) return false; - static_assert( - !std::is_pointer_v || is_base_caster_v, - "Binding ``optional`` requires that ``T`` is handled " - "by nanobind's regular class binding mechanism. However, a " - "type caster was registered to intercept this particular " - "type, which is not allowed."); - value.emplace(caster.operator cast_t()); return true; diff --git a/include/nanobind/stl/pair.h b/include/nanobind/stl/pair.h index 0a9a0749..596b3d3a 100644 --- a/include/nanobind/stl/pair.h +++ b/include/nanobind/stl/pair.h @@ -73,6 +73,11 @@ template struct type_caster> { return r; } + template + bool can_cast() const noexcept { + return caster1.template can_cast() && caster2.template can_cast(); + } + /// Return the constructed tuple by copying from the sub-casters explicit operator Value() { return Value(caster1.operator cast_t(), diff --git a/include/nanobind/stl/tuple.h b/include/nanobind/stl/tuple.h index c1e28252..ed996027 100644 --- a/include/nanobind/stl/tuple.h +++ b/include/nanobind/stl/tuple.h @@ -89,8 +89,15 @@ template struct type_caster> { return r; } + template + bool can_cast() const noexcept { return can_cast_impl(Indices{}); } + explicit operator Value() { return cast_impl(Indices{}); } + template + bool can_cast_impl(std::index_sequence) const noexcept { + return (std::get(casters).template can_cast() && ...); + } template Value cast_impl(std::index_sequence) { return Value(std::get(casters).operator cast_t()...); } diff --git a/include/nanobind/stl/unique_ptr.h b/include/nanobind/stl/unique_ptr.h index ec1433ff..f07e8eec 100644 --- a/include/nanobind/stl/unique_ptr.h +++ b/include/nanobind/stl/unique_ptr.h @@ -71,6 +71,24 @@ struct type_caster> { Caster caster; handle src; + /* If true, the Python object has relinquished ownership but we have + not yet yielded a unique_ptr that holds ownership on the C++ side. + + `nb_type_relinquish_ownership()` can fail, so we must check it in + `can_cast()`. If we do so, but then wind up not executing the cast + operator, we must remember to undo our relinquishment and push the + ownership back onto the Python side. For example, this might be + necessary the Python object `[(foo, foo)]` is converted to + `std::vector, std::unique_ptr>>`; + the pair caster won't know that it can't cast the second element + until after it's verified that it can cast the first one. */ + mutable bool inflight = false; + + ~type_caster() { + if (inflight) + nb_type_restore_ownership(src.ptr(), IsDefaultDeleter); + } + bool from_python(handle src_, uint8_t, cleanup_list *) noexcept { // Stash source python object src = src_; @@ -126,8 +144,21 @@ struct type_caster> { return result; } + template + bool can_cast() const noexcept { + if (src.is_none() || inflight) + return true; + else if (!nb_type_relinquish_ownership(src.ptr(), IsDefaultDeleter)) + return false; + inflight = true; + return true; + } + explicit operator Value() { - nb_type_relinquish_ownership(src.ptr(), IsDefaultDeleter); + if (inflight) + inflight = false; + else if (!nb_type_relinquish_ownership(src.ptr(), IsDefaultDeleter)) + throw next_overload(); T *value = caster.operator T *(); if constexpr (IsNanobindDeleter) diff --git a/include/nanobind/stl/variant.h b/include/nanobind/stl/variant.h index de70ffd9..03686cb6 100644 --- a/include/nanobind/stl/variant.h +++ b/include/nanobind/stl/variant.h @@ -44,19 +44,10 @@ template struct type_caster> { bool try_variant(const handle &src, uint8_t flags, cleanup_list *cleanup) { using CasterT = make_caster; - static_assert( - !std::is_pointer_v || is_base_caster_v, - "Binding ``variant`` requires that ``T`` is handled " - "by nanobind's regular class binding mechanism. However, a " - "type caster was registered to intercept this particular " - "type, which is not allowed."); - - if constexpr (is_base_caster_v && !std::is_pointer_v) - flags |= (uint8_t) cast_flags::none_disallowed; - CasterT caster; - if (!caster.from_python(src, flags, cleanup)) + if (!caster.from_python(src, flags_for_local_caster(flags), cleanup) || + !caster.template can_cast()) return false; value = caster.operator cast_t(); diff --git a/src/nb_type.cpp b/src/nb_type.cpp index 36570bb6..82e03e5d 100644 --- a/src/nb_type.cpp +++ b/src/nb_type.cpp @@ -1213,7 +1213,7 @@ bool nb_type_get(const std::type_info *cpp_type, PyObject *src, uint8_t flags, // Convert None -> nullptr if (src == Py_None) { *out = nullptr; - return (flags & (uint8_t) cast_flags::none_disallowed) == 0; + return true; } PyTypeObject *src_type = Py_TYPE(src); @@ -1662,30 +1662,45 @@ PyObject *nb_type_put_unique_p(const std::type_info *cpp_type, return o; } -void nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) { +static void warn_relinquish_failed(const char *why, PyObject *o) noexcept { + PyObject *name = nb_inst_name(o); + int rc = PyErr_WarnFormat( + PyExc_RuntimeWarning, 1, + "nanobind::detail::nb_relinquish_ownership(): could not " + "transfer ownership of a Python instance of type '%U' to C++. %s", + name, why); + if (rc != 0) // user has configured warnings-as-errors + PyErr_WriteUnraisable(o); + Py_DECREF(name); +} + +bool nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) noexcept { nb_inst *inst = (nb_inst *) o; - // This function is called to indicate ownership *changes* - check(inst->ready, - "nanobind::detail::nb_relinquish_ownership('%s'): ownership " - "status has become corrupted.", - PyUnicode_AsUTF8AndSize(nb_inst_name(o), nullptr)); + /* This function is called after nb_type_get() succeeds, so the + instance should be ready; but the !ready case is possible if + an attempt is made to transfer ownership of the same object + to C++ multiple times as part of the same data structure. + For example, converting Python (foo, foo) to C++ + std::pair, std::unique_ptr>. */ + + if (!inst->ready) { + warn_relinquish_failed( + "The resulting data structure would have had multiple " + "std::unique_ptrs that each thought they owned the same " + "Python instance's data, which is not allowed.", o); + return false; + } if (cpp_delete) { if (!inst->cpp_delete || !inst->destruct || inst->internal) { - PyObject *name = nb_inst_name(o); - PyErr_WarnFormat( - PyExc_RuntimeWarning, 1, - "nanobind::detail::nb_relinquish_ownership(): could not " - "transfer ownership of a Python instance of type '%U' to C++. " + warn_relinquish_failed( "This is only possible when the instance was previously " "constructed on the C++ side and is now owned by Python, which " "was not the case here. You could change the unique pointer " "signature to std::unique_ptr> to work around " - "this issue.", name); - - Py_DECREF(name); - throw next_overload(); + "this issue.", o); + return false; } inst->cpp_delete = false; @@ -1693,6 +1708,22 @@ void nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) { } inst->ready = false; + return true; +} + +void nb_type_restore_ownership(PyObject *o, bool cpp_delete) noexcept { + nb_inst *inst = (nb_inst *) o; + + check(!inst->ready, + "nanobind::detail::nb_type_restore_ownership('%s'): ownership " + "status has become corrupted.", + PyUnicode_AsUTF8AndSize(nb_inst_name(o), nullptr)); + + inst->ready = true; + if (cpp_delete) { + inst->cpp_delete = true; + inst->destruct = true; + } } bool nb_type_isinstance(PyObject *o, const std::type_info *t) noexcept { diff --git a/tests/test_classes.cpp b/tests/test_classes.cpp index 0db444c2..e6530cc1 100644 --- a/tests/test_classes.cpp +++ b/tests/test_classes.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -306,6 +307,9 @@ NB_MODULE(test_classes_ext, m) { D(C c) : value(c.c + 1000) { } D(int d) : value(d + 10000) { } D(float) : value(0) { throw std::runtime_error("Fail!"); } + D(std::nullptr_t) : value(0) {} + // notice dangling access: + ~D() { static_cast(value) = -100; } int value; }; @@ -329,6 +333,37 @@ NB_MODULE(test_classes_ext, m) { .def_rw("value", &D::value); m.def("get_d", [](const D &d) { return d.value; }); + m.def("get_optional_d", [](std::optional arg) { + return arg ? arg.value()->value : -1; + }, nb::arg().none()); + m.def("get_d_via_cast", [](nb::object obj) { + int by_val = -1, by_ptr = -1, by_opt_val = -1, by_opt_ptr = -1; + try { + by_val = nb::cast(obj).value; + } catch (const nb::cast_error&) {} + try { + by_ptr = nb::cast(obj)->value; + } catch (const nb::cast_error&) {} + try { + by_opt_val = nb::cast>(obj)->value; + } catch (const nb::cast_error&) {} + try { + by_opt_ptr = nb::cast>(obj).value()->value; + } catch (const nb::cast_error&) {} + return nb::make_tuple(by_val, by_ptr, by_opt_val, by_opt_ptr); + }); + m.def("get_d_via_try_cast", [](nb::object obj) { + int by_val = -1, by_ptr = -1, by_opt_val = -1, by_opt_ptr = -1; + if (D val(nullptr); nb::try_cast(obj, val)) + by_val = val.value; + if (D* ptr; nb::try_cast(obj, ptr)) + by_ptr = ptr->value; + if (std::optional opt_val; nb::try_cast(obj, opt_val)) + by_opt_val = opt_val->value; + if (std::optional opt_ptr; nb::try_cast(obj, opt_ptr)) + by_opt_ptr = opt_ptr.value()->value; + return nb::make_tuple(by_val, by_ptr, by_opt_val, by_opt_ptr); + }); struct Int { int i; diff --git a/tests/test_classes.py b/tests/test_classes.py index 72d28a5e..5d73ef67 100644 --- a/tests/test_classes.py +++ b/tests/test_classes.py @@ -285,7 +285,8 @@ def test13_implicitly_convertible(): b = t.B(2) b2 = t.B2(3) c = t.C(4) - d = 5 + i = 5 + with pytest.raises(TypeError) as excinfo: t.get_d(c) assert str(excinfo.value) == ( @@ -294,10 +295,26 @@ def test13_implicitly_convertible(): "\n" "Invoked with types: test_classes_ext.C" ) - assert t.get_d(a) == 11 - assert t.get_d(b) == 102 - assert t.get_d(b2) == 103 + with pytest.raises(TypeError): + t.get_optional_d(c) + + for obj, expected in ((a, 11), (b, 102), (b2, 103), (i, 10005)): + assert t.get_d(obj) == expected + assert t.get_optional_d(obj) == expected + # The -1's here are because nb::cast() won't implicit-convert to a + # pointer because it would dangle + assert t.get_d_via_cast(obj) == (expected, -1, expected, -1) + assert t.get_d_via_try_cast(obj) == (expected, -1, expected, -1) + + d = t.D(5) assert t.get_d(d) == 10005 + assert t.get_optional_d(d) == 10005 + assert t.get_d_via_cast(d) == (10005, 10005, 10005, 10005) + assert t.get_d_via_try_cast(d) == (10005, 10005, 10005, 10005) + + assert t.get_optional_d(None) == -1 + assert t.get_d_via_cast(c) == (-1, -1, -1, -1) + assert t.get_d_via_try_cast(c) == (-1, -1, -1, -1) def test14_operators(): diff --git a/tests/test_classes_ext.pyi.ref b/tests/test_classes_ext.pyi.ref index 2c8bbb32..1edf4ab1 100644 --- a/tests/test_classes_ext.pyi.ref +++ b/tests/test_classes_ext.pyi.ref @@ -261,10 +261,16 @@ def factory_2() -> Base: ... def get_d(arg: D, /) -> int: ... +def get_d_via_cast(arg: object, /) -> tuple: ... + +def get_d_via_try_cast(arg: object, /) -> tuple: ... + def get_destructed() -> list: ... def get_incrementing_struct_value(arg: IncrementingStruct, /) -> Struct: ... +def get_optional_d(arg: D | None) -> int: ... + def go(arg: Animal, /) -> str: ... def i2p(arg: int, /) -> Foo: ... diff --git a/tests/test_holders.cpp b/tests/test_holders.cpp index 8c314403..cb2bd983 100644 --- a/tests/test_holders.cpp +++ b/tests/test_holders.cpp @@ -6,6 +6,7 @@ #include #include #include +#include namespace nb = nanobind; @@ -153,7 +154,8 @@ NB_MODULE(test_holders_ext, m) { // ------- unique_ptr ------- m.def("unique_from_cpp", - []() { return std::make_unique(1); }); + [](int val) { return std::make_unique(val); }, + nb::arg() = 1); m.def("unique_from_cpp_2", []() { return std::unique_ptr>(new Example(2)); }); @@ -171,6 +173,16 @@ NB_MODULE(test_holders_ext, m) { m.def("passthrough_unique_2", [](std::unique_ptr> unique) { return unique; }); + m.def("passthrough_unique_pairs", + [](std::vector, + std::unique_ptr>> v, + bool clear) { + if (clear) { + v.clear(); + } + return v; + }, nb::arg("v"), nb::arg("clear") = false); + m.def("stats", []{ return std::make_pair(created, deleted); }); m.def("reset", []{ created = deleted = 0; }); diff --git a/tests/test_holders.py b/tests/test_holders.py index 7bc59c46..697ae68c 100644 --- a/tests/test_holders.py +++ b/tests/test_holders.py @@ -121,6 +121,34 @@ def test05_uniqueptr_from_cpp(clean): collect() assert t.stats() == (2, 2) + # Test ownership exchange as part of a larger data structure + k = t.unique_from_cpp(1) + v = t.unique_from_cpp(2) + res = t.passthrough_unique_pairs([(k, v)]) + assert res == [(k, v)] + assert k.value == 1 and v.value == 2 + + res = t.passthrough_unique_pairs([(k, v)], clear=True) + assert res == [] + for obj in (k, v): + with pytest.warns(RuntimeWarning, match='nanobind: attempted to access an uninitialized instance of type \'test_holders_ext.Example\'!'): + with pytest.raises(TypeError) as excinfo: + obj.value + + # Test ownership exchange that fails partway through + # (can't take ownershp from k twice) + k = t.unique_from_cpp(3) + with pytest.warns(RuntimeWarning, match=r'nanobind::detail::nb_relinquish_ownership()'): + with pytest.raises(TypeError): + t.passthrough_unique_pairs([(k, k)]) + # Ownership passes back to Python + assert k.value == 3 + + del k, v, res + collect() + collect() + assert t.stats() == (5, 5) + def test06_uniqueptr_from_py(clean): # Test ownership exchange when the object has been created on the Python side @@ -139,6 +167,27 @@ def test06_uniqueptr_from_py(clean): collect() assert t.stats() == (1, 1) + # Test that ownership exchange as part of a larger data structure fails + # gracefully rather than crashing + k = t.Example(1) + v = t.Example(2) + with pytest.warns(RuntimeWarning, match=r'nanobind::detail::nb_relinquish_ownership()'): + with pytest.raises(TypeError) as excinfo: + t.passthrough_unique_pairs([(k, v)]) + assert k.value == 1 and v.value == 2 + + # Test the case where the key relinquishes ownership successfully and + # then the value can't do + v = t.unique_from_cpp(3) + with pytest.warns(RuntimeWarning, match=r'nanobind::detail::nb_relinquish_ownership()'): + with pytest.raises(TypeError) as excinfo: + t.passthrough_unique_pairs([(k, v)]) + assert k.value == 1 and v.value == 3 + + del k, v + collect() + assert t.stats() == (4, 4) + def test07_uniqueptr_passthrough(clean): assert t.passthrough_unique(t.unique_from_cpp()).value == 1 diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 82bb9bf9..05f88df6 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -462,4 +462,10 @@ NB_MODULE(test_stl_ext, m) { }); m.def("pass_wstr", [](std::wstring ws) { return ws; }); + + // uncomment to see compiler error: + // m.def("optional_intptr", [](std::optional) {}); + m.def("optional_cstr", [](std::optional arg) { + return arg.value_or("none"); + }, nb::arg().none()); } diff --git a/tests/test_stl.py b/tests/test_stl.py index 7d59b9b0..965f729e 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -234,6 +234,10 @@ def test24_vec_movable_in_value(clean): t.vec_movable_in_value([t.Movable(i) for i in range(10)]) assert_stats(value_constructed=10, copy_constructed=10, destructed=20) + # Test that None values don't cause a crash + with pytest.raises(TypeError): + t.vec_movable_in_value([None]) + def test25_vec_movable_in_value(clean): t.vec_copyable_in_value([t.Copyable(i) for i in range(10)]) @@ -249,6 +253,11 @@ def test27_vec_movable_in_ptr_2(clean): t.vec_movable_in_ptr_2([t.Movable(i) for i in range(10)]) assert_stats(value_constructed=10, destructed=10) + # Test that None values are permitted when casting to pointer; + # instead we reach 'if (x.size() != 10) fail();' in the bound function + with pytest.raises(RuntimeError): + t.vec_movable_in_ptr_2([None]) + def test28_vec_movable_in_rvalue_ref(clean): t.vec_movable_in_rvalue_ref([t.Movable(i) for i in range(10)]) @@ -373,6 +382,8 @@ def test37_std_optional_copyable_ptr(clean): def test38_std_optional_none(): t.optional_none(None) + assert t.optional_cstr(None) == "none" + assert t.optional_cstr("hi") == "hi" def test39_std_optional_ret_opt_movable(clean): diff --git a/tests/test_stl_ext.pyi.ref b/tests/test_stl_ext.pyi.ref index 731d1419..72a0f5fe 100644 --- a/tests/test_stl_ext.pyi.ref +++ b/tests/test_stl_ext.pyi.ref @@ -133,6 +133,8 @@ def optional_copyable(x: Copyable | None) -> None: ... def optional_copyable_ptr(x: Copyable | None) -> None: ... +def optional_cstr(arg: str | None) -> str: ... + def optional_non_assignable(arg: NonAssignable, /) -> NonAssignable | None: ... def optional_none(x: Copyable | None) -> None: ...