From 935ab7b639e76d7a7cd44914de9e38ebe4a72172 Mon Sep 17 00:00:00 2001 From: Ben Deane Date: Wed, 1 Jan 2025 15:02:05 -0700 Subject: [PATCH 1/2] :art: Use injected class name in `nexus` --- include/cib/nexus.hpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/include/cib/nexus.hpp b/include/cib/nexus.hpp index e2d9beb7..27b6885f 100644 --- a/include/cib/nexus.hpp +++ b/include/cib/nexus.hpp @@ -16,25 +16,22 @@ namespace cib { * * @see cib::config */ -template struct nexus { - private: - using this_t = nexus; +template struct nexus { // Workaround unfortunate bug in clang where it can't deduce "auto" sometimes #define CIB_BUILD_SERVICE \ initialized::value.template build>() - public: template constexpr static decltype(CIB_BUILD_SERVICE) service = CIB_BUILD_SERVICE; #undef CIB_BUILD_SERVICE static void init() { auto const service = [] { - using from_t = std::remove_cvref_t)>; + using from_t = std::remove_cvref_t)>; using to_t = std::remove_cvref_t)>; - auto &service_impl = this_t::service; + auto &service_impl = nexus::service; if constexpr (std::is_convertible_v) { cib::service = service_impl; } else { From a7db66ed857b7a52ecb1e725c9542f3ea840861c Mon Sep 17 00:00:00 2001 From: Ben Deane Date: Wed, 1 Jan 2025 15:57:10 -0700 Subject: [PATCH 2/2] :safety_vest: Protect against uninitialized services Problem: - If services are uninitialized, either through not being exported properly, or through `nexus.init()` not being called, the service pointers are null and the resulting UB causes hard-to-diagnose effects. Solution: - Initialize services to functions that cause a runtime panic. --- include/cib/builder_meta.hpp | 3 ++ include/cib/built.hpp | 3 +- include/cib/callback.hpp | 8 +++++ include/flow/builder.hpp | 10 ++++++ include/msg/handler_interface.hpp | 35 +++++++++++++++++++ include/msg/indexed_service.hpp | 7 ++++ include/msg/message.hpp | 2 ++ include/msg/service.hpp | 8 ++++- test/cib/CMakeLists.txt | 1 + test/cib/builder_meta.cpp | 15 ++++---- test/cib/callback_uninit.cpp | 35 +++++++++++++++++++ test/flow/CMakeLists.txt | 1 + test/flow/flow.cpp | 1 + test/flow/flow_uninit.cpp | 34 ++++++++++++++++++ test/msg/CMakeLists.txt | 2 ++ test/msg/handler_uninit.cpp | 54 +++++++++++++++++++++++++++++ test/msg/indexed_handler_uninit.cpp | 42 ++++++++++++++++++++++ 17 files changed, 253 insertions(+), 8 deletions(-) create mode 100644 test/cib/callback_uninit.cpp create mode 100644 test/flow/flow_uninit.cpp create mode 100644 test/msg/handler_uninit.cpp create mode 100644 test/msg/indexed_handler_uninit.cpp diff --git a/include/cib/builder_meta.hpp b/include/cib/builder_meta.hpp index 9761fed6..4b2dd85c 100644 --- a/include/cib/builder_meta.hpp +++ b/include/cib/builder_meta.hpp @@ -1,10 +1,13 @@ #pragma once +#include + namespace cib { template concept builder_meta = requires { typename T::builder_t; typename T::interface_t; + { T::uninitialized() } -> std::same_as; }; template using builder_t = typename T::builder_t; diff --git a/include/cib/built.hpp b/include/cib/built.hpp index 3a64c2e4..8391a731 100644 --- a/include/cib/built.hpp +++ b/include/cib/built.hpp @@ -3,5 +3,6 @@ #include namespace cib { -template interface_t service; +template +constinit auto service = ServiceMeta::uninitialized(); } // namespace cib diff --git a/include/cib/callback.hpp b/include/cib/callback.hpp index 21d44514..47e76dd3 100644 --- a/include/cib/callback.hpp +++ b/include/cib/callback.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -119,5 +120,12 @@ template struct builder { template struct service { using builder_t = builder<0, ArgTypes...>; using interface_t = void (*)(ArgTypes...); + + CONSTEVAL static auto uninitialized() -> interface_t { + return [](ArgTypes...) { + stdx::panic< + "Attempting to run callback before it is initialized">(); + }; + } }; } // namespace callback diff --git a/include/flow/builder.hpp b/include/flow/builder.hpp index 059f0536..31852dff 100644 --- a/include/flow/builder.hpp +++ b/include/flow/builder.hpp @@ -4,7 +4,9 @@ #include #include +#include #include +#include namespace flow { template @@ -13,5 +15,13 @@ using builder = graph>; template struct service { using builder_t = builder; using interface_t = FunctionPtr; + + CONSTEVAL static auto uninitialized() -> interface_t { + return [] { + using namespace stdx::literals; + stdx::panic<"Attempting to run flow ("_cts + Name + + ") before it is initialized"_cts>(); + }; + } }; } // namespace flow diff --git a/include/msg/handler_interface.hpp b/include/msg/handler_interface.hpp index 22387e8c..934e3b2f 100644 --- a/include/msg/handler_interface.hpp +++ b/include/msg/handler_interface.hpp @@ -1,5 +1,11 @@ #pragma once +#include +#include +#include +#include +#include + namespace msg { template struct handler_interface { @@ -8,4 +14,33 @@ struct handler_interface { virtual auto handle(MsgBase const &msg, ExtraCallbackArgs... extra_args) const -> bool = 0; }; + +namespace detail { +template +concept named_msg_base = + stdx::is_specialization_of().value; + +template CONSTEVAL auto name_for_msg() { + if constexpr (detail::named_msg_base) { + return M::name; + } else { + constexpr auto name = stdx::type_as_string(); + return stdx::ct_string{name}; + } +} +} // namespace detail + +template +struct uninitialized_handler_t + : handler_interface { + auto is_match(MsgBase const &) const -> bool override { return false; } + + auto handle(MsgBase const &, ExtraCallbackArgs...) const -> bool override { + using namespace stdx::literals; + stdx::panic<"Attempting to handle msg ("_cts + + detail::name_for_msg() + + ") before service is initialized"_cts>(); + return false; + } +}; } // namespace msg diff --git a/include/msg/indexed_service.hpp b/include/msg/indexed_service.hpp index ea9dc3be..1acea01a 100644 --- a/include/msg/indexed_service.hpp +++ b/include/msg/indexed_service.hpp @@ -3,6 +3,7 @@ #include #include +#include #include namespace msg { @@ -12,5 +13,11 @@ struct indexed_service { ExtraCallbackArgs...>; using interface_t = handler_interface const *; + + constexpr static auto uninitialized_v = + uninitialized_handler_t{}; + CONSTEVAL static auto uninitialized() -> interface_t { + return &uninitialized_v; + } }; } // namespace msg diff --git a/include/msg/message.hpp b/include/msg/message.hpp index 53700636..943e90ab 100644 --- a/include/msg/message.hpp +++ b/include/msg/message.hpp @@ -349,6 +349,8 @@ template struct message { (... and Fields::template fits_inside()); template struct base { + constexpr static auto name = Name; + constexpr auto as_derived() const -> T const & { return static_cast(*this); } diff --git a/include/msg/service.hpp b/include/msg/service.hpp index 3aa9bd26..e6895420 100644 --- a/include/msg/service.hpp +++ b/include/msg/service.hpp @@ -3,7 +3,7 @@ #include #include -#include +#include namespace msg { template struct service { @@ -11,5 +11,11 @@ template struct service { handler_builder, MsgBase, ExtraCallbackArgs...>; using interface_t = handler_interface const *; + + constexpr static auto uninitialized_v = + uninitialized_handler_t{}; + CONSTEVAL static auto uninitialized() -> interface_t { + return &uninitialized_v; + } }; } // namespace msg diff --git a/test/cib/CMakeLists.txt b/test/cib/CMakeLists.txt index 4bbbde99..b1fd899f 100644 --- a/test/cib/CMakeLists.txt +++ b/test/cib/CMakeLists.txt @@ -2,6 +2,7 @@ add_tests( FILES builder_meta callback + callback_uninit nexus readme_hello_world LIBRARIES diff --git a/test/cib/builder_meta.cpp b/test/cib/builder_meta.cpp index c0579f4b..445edea4 100644 --- a/test/cib/builder_meta.cpp +++ b/test/cib/builder_meta.cpp @@ -1,16 +1,19 @@ #include +#include + #include #include namespace { -struct TestBuilderTag; -struct TestInterfaceTag; +struct TestBuilder; +struct TestInterface {}; struct test_builder_meta { - using builder_t = TestBuilderTag; - using interface_t = TestInterfaceTag; + using builder_t = TestBuilder; + using interface_t = TestInterface; + CONSTEVAL static auto uninitialized() -> interface_t; }; } // namespace @@ -21,7 +24,7 @@ TEST_CASE("builder_meta concept") { TEST_CASE( "builder_meta builder and interface type traits return correct values") { static_assert( - std::is_same_v>); + std::is_same_v>); static_assert( - std::is_same_v>); + std::is_same_v>); } diff --git a/test/cib/callback_uninit.cpp b/test/cib/callback_uninit.cpp new file mode 100644 index 00000000..e8f74180 --- /dev/null +++ b/test/cib/callback_uninit.cpp @@ -0,0 +1,35 @@ +#include +#include + +#include + +#include + +#include +#include + +namespace { +template +struct TestCallback : public callback::service {}; + +std::string panic_string = {}; +int panics{}; + +struct test_panic_handler { + template + static auto panic(Ts &&...) -> void { + panic_string = std::string_view{Why}; + ++panics; + } +}; +} // namespace + +template <> inline auto stdx::panic_handler<> = test_panic_handler{}; + +TEST_CASE("run callback service when uninitialized", "[callback]") { + panics = 0; + cib::service>(); + CHECK(panics == 1); + CHECK(panic_string == + "Attempting to run callback before it is initialized"); +} diff --git a/test/flow/CMakeLists.txt b/test/flow/CMakeLists.txt index 40da6706..d95f5569 100644 --- a/test/flow/CMakeLists.txt +++ b/test/flow/CMakeLists.txt @@ -11,6 +11,7 @@ add_unit_test( add_tests( FILES flow + flow_uninit graph graph_builder logging diff --git a/test/flow/flow.cpp b/test/flow/flow.cpp index 00ddf806..7523a8dd 100644 --- a/test/flow/flow.cpp +++ b/test/flow/flow.cpp @@ -54,6 +54,7 @@ using alt_builder = flow::graph; template struct alt_flow_service { using builder_t = alt_builder; using interface_t = flow::VizFunctionPtr; + constexpr static auto uninitialized() -> interface_t { return {}; } }; struct VizFlow : public alt_flow_service<"debug"> {}; diff --git a/test/flow/flow_uninit.cpp b/test/flow/flow_uninit.cpp new file mode 100644 index 00000000..70256b00 --- /dev/null +++ b/test/flow/flow_uninit.cpp @@ -0,0 +1,34 @@ +#include +#include + +#include + +#include + +#include +#include + +namespace { +struct TestFlow : flow::service<"test"> {}; + +std::string panic_string = {}; +int panics{}; + +struct test_panic_handler { + template + static auto panic(Ts &&...) -> void { + panic_string = std::string_view{Why}; + ++panics; + } +}; +} // namespace + +template <> inline auto stdx::panic_handler<> = test_panic_handler{}; + +TEST_CASE("run flow when uninitialized", "[flow]") { + panics = 0; + cib::service(); + CHECK(panics == 1); + CHECK(panic_string == + "Attempting to run flow (test) before it is initialized"); +} diff --git a/test/msg/CMakeLists.txt b/test/msg/CMakeLists.txt index b03bf0d1..61c06d4a 100644 --- a/test/msg/CMakeLists.txt +++ b/test/msg/CMakeLists.txt @@ -6,9 +6,11 @@ add_tests( field_matchers handler handler_builder + handler_uninit indexed_builder indexed_callback indexed_handler + indexed_handler_uninit message send LIBRARIES diff --git a/test/msg/handler_uninit.cpp b/test/msg/handler_uninit.cpp new file mode 100644 index 00000000..bbe7df92 --- /dev/null +++ b/test/msg/handler_uninit.cpp @@ -0,0 +1,54 @@ +#include +#include +#include + +#include + +#include + +#include +#include +#include + +struct raw_msg_t {}; + +namespace { +using namespace msg; + +using msg_defn = message<"msg">; +using test_msg_t = msg::owning; +using msg_view_t = msg::const_view; +struct test_service : msg::service {}; +struct raw_service : msg::service {}; + +std::string panic_string = {}; +int panics{}; + +struct test_panic_handler { + template + static auto panic(Ts &&...) -> void { + panic_string = std::string_view{Why}; + ++panics; + } +}; +} // namespace + +template <> inline auto stdx::panic_handler<> = test_panic_handler{}; + +TEST_CASE("invoke handle on service when uninitialized (named msg type)", + "[handler]") { + panics = 0; + cib::service->handle(test_msg_t{}); + CHECK(panics == 1); + CHECK(panic_string == + "Attempting to handle msg (msg) before service is initialized"); +} + +TEST_CASE("invoke handle on service when uninitialized (raw msg type)", + "[handler]") { + panics = 0; + cib::service->handle(raw_msg_t{}); + CHECK(panics == 1); + CHECK(panic_string == + "Attempting to handle msg (raw_msg_t) before service is initialized"); +} diff --git a/test/msg/indexed_handler_uninit.cpp b/test/msg/indexed_handler_uninit.cpp new file mode 100644 index 00000000..8a66d48e --- /dev/null +++ b/test/msg/indexed_handler_uninit.cpp @@ -0,0 +1,42 @@ +#include +#include +#include + +#include + +#include + +#include +#include + +namespace { +using namespace msg; + +using msg_defn = message<"msg">; +using test_msg_t = msg::owning; +using msg_view_t = msg::const_view; + +using index_spec = msg::index_spec<>; +struct test_service : msg::indexed_service {}; + +std::string panic_string = {}; +int panics{}; + +struct test_panic_handler { + template + static auto panic(Ts &&...) -> void { + panic_string = std::string_view{Why}; + ++panics; + } +}; +} // namespace + +template <> inline auto stdx::panic_handler<> = test_panic_handler{}; + +TEST_CASE("invoke handle on service when uninitialized", "[indexed_handler]") { + panics = 0; + cib::service->handle(test_msg_t{}); + CHECK(panics == 1); + CHECK(panic_string == + "Attempting to handle msg (msg) before service is initialized"); +}