From f277cf2f0094f58e2b2de2f56bc7735f0b4c20f3 Mon Sep 17 00:00:00 2001 From: Robert Norton Date: Wed, 27 Apr 2022 11:56:16 +0100 Subject: [PATCH] Refactor capptr_domesticate SFINAE to make more statically safe. This refactoring was provided by David. Previously if a backend provided a capptr_domesticate function with the wrong type it would be silently ignored. This change requires backends to explicitly opt in to domestication via a new Backend::Option and ensures the compiler will loudly complain if there is a mismatch. --- src/snmalloc/backend/fixedglobalconfig.h | 21 ++++++- src/snmalloc/backend_helpers/commonconfig.h | 7 +++ src/snmalloc/mem/backend_wrappers.h | 62 ++++++++++++++------ src/test/func/domestication/domestication.cc | 1 + 4 files changed, 72 insertions(+), 19 deletions(-) diff --git a/src/snmalloc/backend/fixedglobalconfig.h b/src/snmalloc/backend/fixedglobalconfig.h index f0e8a57..41b5026 100644 --- a/src/snmalloc/backend/fixedglobalconfig.h +++ b/src/snmalloc/backend/fixedglobalconfig.h @@ -24,7 +24,26 @@ namespace snmalloc return alloc_pool; } - static constexpr Flags Options{}; + /* + * The obvious + * `static constexpr Flags Options{.HasDomesticate = true};` fails on + * Ubuntu 18.04 with an error "sorry, unimplemented: non-trivial + * designated initializers not supported". + * The following was copied from domestication.cc test with the following + * comment: + * C++, even as late as C++20, has some really quite strict limitations on + * designated initializers. However, as of C++17, we can have constexpr + * lambdas and so can use more of the power of the statement fragment of + * C++, and not just its initializer fragment, to initialize a non-prefix + * subset of the flags (in any order, at that). + */ + static constexpr Flags Options = []() constexpr + { + Flags opts = {}; + opts.HasDomesticate = true; + return opts; + } + (); // This needs to be a forward reference as the // thread local state will need to know about this. diff --git a/src/snmalloc/backend_helpers/commonconfig.h b/src/snmalloc/backend_helpers/commonconfig.h index c8e55e1..aca6103 100644 --- a/src/snmalloc/backend_helpers/commonconfig.h +++ b/src/snmalloc/backend_helpers/commonconfig.h @@ -86,6 +86,13 @@ namespace snmalloc * the queue nodes themselves (which are always considered Wild)? */ bool QueueHeadsAreTame = true; + + /** + * Does the backend provide a capptr_domesticate function to sanity check + * pointers? If so it will be called when untrusted pointers are consumed + * (on dealloc and in freelists) otherwise a no-op version is provided. + */ + bool HasDomesticate = false; }; /** diff --git a/src/snmalloc/mem/backend_wrappers.h b/src/snmalloc/mem/backend_wrappers.h index 36657df..a16b7e8 100644 --- a/src/snmalloc/mem/backend_wrappers.h +++ b/src/snmalloc/mem/backend_wrappers.h @@ -36,41 +36,46 @@ namespace snmalloc namespace detail { /** - * SFINAE helper, calls capptr_domesticate in the backend if it exists. + * SFINAE helper to detect the presence of capptr_domesticate function in + * backend. Returns true if there is a function with correct name and type. */ template< SNMALLOC_CONCEPT(ConceptBackendDomestication) Backend, typename T, SNMALLOC_CONCEPT(capptr::ConceptBound) B> - SNMALLOC_FAST_PATH_INLINE auto - capptr_domesticate(typename Backend::LocalState* ls, CapPtr p, int) - -> decltype(Backend::capptr_domesticate(ls, p)) + constexpr SNMALLOC_FAST_PATH_INLINE auto has_domesticate(int) + -> std::enable_if_t< + std::is_same_v< + decltype(Backend::capptr_domesticate( + std::declval(), + std::declval>())), + CapPtr< + T, + typename B::template with_wildness< + capptr::dimension::Wildness::Tame>>>, + bool> { - return Backend::capptr_domesticate(ls, p); + return true; } /** - * SFINAE helper. If the back end does not provide special handling for - * domestication then assume all wild pointers can be domesticated. + * SFINAE helper to detect the presence of capptr_domesticate function in + * backend. Returns false in case where above template does not match. */ template< SNMALLOC_CONCEPT(ConceptBackendGlobals) Backend, typename T, SNMALLOC_CONCEPT(capptr::ConceptBound) B> - SNMALLOC_FAST_PATH_INLINE auto - capptr_domesticate(typename Backend::LocalState*, CapPtr p, long) + constexpr SNMALLOC_FAST_PATH_INLINE bool has_domesticate(long) { - return CapPtr< - T, - typename B::template with_wildness>( - p.unsafe_ptr()); + return false; } } // namespace detail /** - * Wrapper that calls `Backend::capptr_domesticate` if and only if it is - * implemented. If it is not implemented then this assumes that any wild - * pointer can be domesticated. + * Wrapper that calls `Backend::capptr_domesticate` if and only if + * Backend::Options.HasDomesticate is true. If it is not implemented then + * this assumes that any wild pointer can be domesticated. */ template< SNMALLOC_CONCEPT(ConceptBackendGlobals) Backend, @@ -79,7 +84,28 @@ namespace snmalloc SNMALLOC_FAST_PATH_INLINE auto capptr_domesticate(typename Backend::LocalState* ls, CapPtr p) { - return detail::capptr_domesticate(ls, p, 0); - } + static_assert( + !detail::has_domesticate(0) || + (detail::has_domesticate(0) && + Backend::Options.HasDomesticate), + "Back end provides domesticate function but opts out of using it "); + static_assert( + detail::has_domesticate(0) || + !(detail::has_domesticate(0) && + Backend::Options.HasDomesticate), + "Back end does not provide capptr_domesticate and requests its use"); + if constexpr (Backend::Options.HasDomesticate) + { + return Backend::capptr_domesticate(ls, p); + } + else + { + UNUSED(ls); + return CapPtr< + T, + typename B::template with_wildness>( + p.unsafe_ptr()); + } + } } // namespace snmalloc diff --git a/src/test/func/domestication/domestication.cc b/src/test/func/domestication/domestication.cc index 26c033a..726a2f3 100644 --- a/src/test/func/domestication/domestication.cc +++ b/src/test/func/domestication/domestication.cc @@ -40,6 +40,7 @@ namespace snmalloc { Flags opts = {}; opts.QueueHeadsAreTame = false; + opts.HasDomesticate = true; return opts; } ();