iox-eclipse-iceoryx#932 Handle nullptr callbacks in Listener

Includes the C-Binding
ApexAI · Oct 7, 2021 · 4eea032 · 4eea032
1 parent 0cd82bb
commit 4eea032
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 11 deletions.
diff --git a/iceoryx_binding_c/include/iceoryx_binding_c/enums.h b/iceoryx_binding_c/include/iceoryx_binding_c/enums.h
@@ -88,6 +88,7 @@ enum iox_ListenerResult
 {
     ListenerResult_LISTENER_FULL,
     ListenerResult_EVENT_ALREADY_ATTACHED,
+    ListenerResult_EMPTY_EVENT_CALLBACK,
     ListenerResult_EMPTY_INVALIDATION_CALLBACK,
     ListenerResult_UNDEFINED_ERROR,
     ListenerResult_SUCCESS

diff --git a/iceoryx_binding_c/source/c_listener.cpp b/iceoryx_binding_c/source/c_listener.cpp
@@ -48,8 +48,9 @@ ENUM iox_ListenerResult iox_listener_attach_subscriber_event(iox_listener_t cons
                                                              const ENUM iox_SubscriberEvent subscriberEvent,
                                                              void (*callback)(iox_sub_t))
 {
-    auto result =
-        self->attachEvent(*subscriber, c2cpp::subscriberEvent(subscriberEvent), createNotificationCallback(*callback));
+    auto result = self->attachEvent(*subscriber,
+                                    c2cpp::subscriberEvent(subscriberEvent),
+                                    NotificationCallback<cpp2c_Subscriber, internal::NoType_t>{callback, nullptr});
     if (result.has_error())
     {
         return cpp2c::listenerResult(result.get_error());
@@ -68,7 +69,9 @@ iox_listener_attach_subscriber_event_with_context_data(iox_listener_t const self
     notificationCallback.m_callback = callback;
     notificationCallback.m_contextData = contextData;
 
-    auto result = self->attachEvent(*subscriber, c2cpp::subscriberEvent(subscriberEvent), notificationCallback);
+    auto result = self->attachEvent(*subscriber,
+                                    c2cpp::subscriberEvent(subscriberEvent),
+                                    NotificationCallback<cpp2c_Subscriber, void>{callback, contextData});
     if (result.has_error())
     {
         return cpp2c::listenerResult(result.get_error());
@@ -80,7 +83,8 @@ ENUM iox_ListenerResult iox_listener_attach_user_trigger_event(iox_listener_t co
                                                                iox_user_trigger_t const userTrigger,
                                                                void (*callback)(iox_user_trigger_t))
 {
-    auto result = self->attachEvent(*userTrigger, createNotificationCallback(*callback));
+    auto result =
+        self->attachEvent(*userTrigger, NotificationCallback<UserTrigger, internal::NoType_t>{callback, nullptr});
     if (result.has_error())
     {
         return cpp2c::listenerResult(result.get_error());
@@ -98,7 +102,7 @@ ENUM iox_ListenerResult iox_listener_attach_user_trigger_event_with_context_data
     notificationCallback.m_callback = callback;
     notificationCallback.m_contextData = contextData;
 
-    auto result = self->attachEvent(*userTrigger, notificationCallback);
+    auto result = self->attachEvent(*userTrigger, NotificationCallback<UserTrigger, void>{callback, contextData});
     if (result.has_error())
     {
         return cpp2c::listenerResult(result.get_error());

diff --git a/iceoryx_binding_c/source/cpp2c_enum_translation.cpp b/iceoryx_binding_c/source/cpp2c_enum_translation.cpp
@@ -95,6 +95,8 @@ iox_ListenerResult listenerResult(const iox::popo::ListenerError value) noexcept
         return ListenerResult_LISTENER_FULL;
     case ListenerError::INVALID_STATE:
         return ListenerResult_UNDEFINED_ERROR;
+    case ListenerError::EMPTY_EVENT_CALLBACK:
+        return ListenerResult_EMPTY_EVENT_CALLBACK;
     case ListenerError::EMPTY_INVALIDATION_CALLBACK:
         return ListenerResult_EMPTY_INVALIDATION_CALLBACK;
     }

diff --git a/iceoryx_binding_c/test/moduletests/test_cpp2c_enum_translation.cpp b/iceoryx_binding_c/test/moduletests/test_cpp2c_enum_translation.cpp
@@ -181,6 +181,7 @@ TEST(cpp2c_enum_translation_test, ListenerResult)
     constexpr EnumMapping<iox::popo::ListenerError, iox_ListenerResult> LISTENER_ERRORS[]{
         {iox::popo::ListenerError::LISTENER_FULL, ListenerResult_LISTENER_FULL},
         {iox::popo::ListenerError::EVENT_ALREADY_ATTACHED, ListenerResult_EVENT_ALREADY_ATTACHED},
+        {iox::popo::ListenerError::EMPTY_EVENT_CALLBACK, ListenerResult_EMPTY_EVENT_CALLBACK},
         {iox::popo::ListenerError::EMPTY_INVALIDATION_CALLBACK, ListenerResult_EMPTY_INVALIDATION_CALLBACK},
         {iox::popo::ListenerError::INVALID_STATE, ListenerResult_UNDEFINED_ERROR}};
 
@@ -194,6 +195,9 @@ TEST(cpp2c_enum_translation_test, ListenerResult)
         case iox::popo::ListenerError::EVENT_ALREADY_ATTACHED:
             EXPECT_EQ(cpp2c::listenerResult(listenerError.cpp), listenerError.c);
             break;
+        case iox::popo::ListenerError::EMPTY_EVENT_CALLBACK:
+            EXPECT_EQ(cpp2c::listenerResult(listenerError.cpp), listenerError.c);
+            break;
         case iox::popo::ListenerError::EMPTY_INVALIDATION_CALLBACK:
             EXPECT_EQ(cpp2c::listenerResult(listenerError.cpp), listenerError.c);
             break;

diff --git a/iceoryx_binding_c/test/moduletests/test_listener.cpp b/iceoryx_binding_c/test/moduletests/test_listener.cpp
@@ -233,6 +233,19 @@ TEST_F(iox_listener_test, AttachingSubscriberEventWorks)
                 Eq(iox_ListenerResult::ListenerResult_SUCCESS));
 }
 
+TEST_F(iox_listener_test, AttachingSubscriberEventWithNullptrCallbackFails)
+{
+    EXPECT_THAT(iox_listener_attach_subscriber_event(
+                    &m_sut, &m_subscriber[0U], iox_SubscriberEvent::SubscriberEvent_DATA_RECEIVED, NULL),
+                Eq(iox_ListenerResult::ListenerResult_EMPTY_EVENT_CALLBACK));
+}
+
+TEST_F(iox_listener_test, AttachingUserTriggerEventWithNullptrCallbackFails)
+{
+    EXPECT_THAT(iox_listener_attach_user_trigger_event(&m_sut, m_userTrigger[0U], NULL),
+                Eq(iox_ListenerResult::ListenerResult_EMPTY_EVENT_CALLBACK));
+}
+
 TEST_F(iox_listener_test, AttachingSubscriberTillListenerFullWorks)
 {
     AttachAllSubscriber();

diff --git a/iceoryx_posh/include/iceoryx_posh/internal/popo/listener.inl b/iceoryx_posh/include/iceoryx_posh/internal/popo/listener.inl
@@ -20,33 +20,43 @@ namespace iox
 {
 namespace popo
 {
-template <typename T, typename UserType>
+template <typename T, typename ContextDataType>
 inline cxx::expected<ListenerError>
-Listener::attachEvent(T& eventOrigin, const NotificationCallback<T, UserType>& eventCallback) noexcept
+Listener::attachEvent(T& eventOrigin, const NotificationCallback<T, ContextDataType>& eventCallback) noexcept
 {
+    if (eventCallback.m_callback == nullptr)
+    {
+        return cxx::error<ListenerError>(ListenerError::EMPTY_EVENT_CALLBACK);
+    }
+
     return addEvent(&eventOrigin,
                     eventCallback.m_contextData,
                     static_cast<uint64_t>(NoEnumUsed::PLACEHOLDER),
                     typeid(NoEnumUsed).hash_code(),
                     reinterpret_cast<internal::GenericCallbackRef_t>(*eventCallback.m_callback),
-                    internal::TranslateAndCallTypelessCallback<T, UserType>::call,
+                    internal::TranslateAndCallTypelessCallback<T, ContextDataType>::call,
                     NotificationAttorney::getInvalidateTriggerMethod(eventOrigin))
         .and_then([&](auto& eventId) {
             NotificationAttorney::enableEvent(
                 eventOrigin, TriggerHandle(*m_conditionVariableData, {*this, &Listener::removeTrigger}, eventId));
         });
 }
 
-template <typename T, typename EventType, typename UserType, typename>
+template <typename T, typename EventType, typename ContextDataType, typename>
 inline cxx::expected<ListenerError> Listener::attachEvent(
-    T& eventOrigin, const EventType eventType, const NotificationCallback<T, UserType>& eventCallback) noexcept
+    T& eventOrigin, const EventType eventType, const NotificationCallback<T, ContextDataType>& eventCallback) noexcept
 {
+    if (eventCallback.m_callback == nullptr)
+    {
+        return cxx::error<ListenerError>(ListenerError::EMPTY_EVENT_CALLBACK);
+    }
+
     return addEvent(&eventOrigin,
                     eventCallback.m_contextData,
                     static_cast<uint64_t>(eventType),
                     typeid(EventType).hash_code(),
                     reinterpret_cast<internal::GenericCallbackRef_t>(*eventCallback.m_callback),
-                    internal::TranslateAndCallTypelessCallback<T, UserType>::call,
+                    internal::TranslateAndCallTypelessCallback<T, ContextDataType>::call,
                     NotificationAttorney::getInvalidateTriggerMethod(eventOrigin))
         .and_then([&](auto& eventId) {
             NotificationAttorney::enableEvent(

diff --git a/iceoryx_posh/include/iceoryx_posh/popo/listener.hpp b/iceoryx_posh/include/iceoryx_posh/popo/listener.hpp
@@ -39,6 +39,7 @@ enum class ListenerError
     INVALID_STATE,
     LISTENER_FULL,
     EVENT_ALREADY_ATTACHED,
+    EMPTY_EVENT_CALLBACK,
     EMPTY_INVALIDATION_CALLBACK
 };
 

diff --git a/iceoryx_posh/test/moduletests/test_popo_listener.cpp b/iceoryx_posh/test/moduletests/test_popo_listener.cpp
@@ -465,6 +465,28 @@ TEST_F(Listener_test, AttachingSameClassWithTwoDifferentEventsWorks)
                      .has_error());
 }
 
+TEST_F(Listener_test, AttachingNullptrCallbackFails)
+{
+    auto empty_callback = createNotificationCallback(attachCallback);
+    empty_callback.m_callback = nullptr;
+    empty_callback.m_contextData = nullptr;
+
+    auto result = m_sut->attachEvent(m_simpleEvents[0U], empty_callback);
+    ASSERT_TRUE(result.has_error());
+    EXPECT_THAT(result.get_error(), Eq(ListenerError::EMPTY_EVENT_CALLBACK));
+}
+
+TEST_F(Listener_test, AttachingNullptrCallbackWithEventFails)
+{
+    auto empty_callback = createNotificationCallback(attachCallback);
+    empty_callback.m_callback = nullptr;
+    empty_callback.m_contextData = nullptr;
+
+    auto result = m_sut->attachEvent(m_simpleEvents[0U], SimpleEvent::StoepselBachelorParty, empty_callback);
+    ASSERT_TRUE(result.has_error());
+    EXPECT_THAT(result.get_error(), Eq(ListenerError::EMPTY_EVENT_CALLBACK));
+}
+
 TEST_F(Listener_test, DetachingSameClassWithDifferentEventEnumChangesNothing)
 {
     ASSERT_FALSE(m_sut