Merge pull request eclipse-iceoryx#1573 from ApexAI/iox-#1571-check-f…

…or-nullptr-in-unique-ptr

iox-eclipse-iceoryx#1571 Add check for `nullptr` in `unique_ptr::get()`

doc/website/release-notes/iceoryx-unreleased.md

@@ -44,6 +44,7 @@
   - This bug was not part of a release but introduce during the v3 development
 - Add "inline" keyword to smart_lock method implementation [\#1551](https://github.com/eclipse-iceoryx/iceoryx/issues/1551)
 - Fix RouDi crash due to uninitialized `ServiceRegistry` chunk [\#1575](https://github.com/eclipse-iceoryx/iceoryx/issues/1575)
+- Add check in `cxx::unique_ptr::get` to avoid `nullptr` dereferencing [\#1571](https://github.com/eclipse-iceoryx/iceoryx/issues/1571)
 
 **Refactoring:**
 

iceoryx_hoofs/include/iceoryx_hoofs/cxx/unique_ptr.hpp

@@ -36,11 +36,6 @@ class unique_ptr
   public:
     unique_ptr() = delete;
 
-    ///
-    /// @brief unique_ptr Creates an empty unique ptr that owns nothing. Can be passed ownership later via reset.
-    ///
-    explicit unique_ptr(const function<void(T*)>& deleter) noexcept;
-
     ///
     /// @brief unique_ptr Creates a unique pointer that takes ownership of an object.
     /// @details A deleter must always be provided as no default can be provided given that no heap is used.
@@ -56,7 +51,7 @@ class unique_ptr
     unique_ptr& operator=(unique_ptr&& rhs) noexcept;
 
     ///
-    /// Automatically deletes the managed object on destruction.
+    /// @brief Automatically deletes the managed object on destruction.
     ///
     ~unique_ptr() noexcept;
 
@@ -65,13 +60,13 @@ class unique_ptr
 
     ///
     /// @brief operator -> Transparent access to the managed object.
-    /// @return
+    /// @return Pointer to the stored object
     ///
     T* operator->() noexcept;
 
     ///
     /// @brief operator -> Transparent access to the managed object.
-    /// @return
+    /// @return Const pointer to the stored object
     ///
     const T* operator->() const noexcept;
 
@@ -83,14 +78,14 @@ class unique_ptr
     ///
     /// @brief get Retrieve the underlying raw pointer.
     /// @details The unique_ptr retains ownership, therefore the "borrowed" pointer must not be deleted.
-    /// @return Pointer to managed object or nullptr if none owned.
+    /// @return Pointer to managed object or errorHandler call if none owned.
     ///
     T* get() noexcept;
 
     ///
     /// @brief get Retrieve the underlying raw pointer.
     /// @details The unique_ptr retains ownership, therefore the "borrowed" pointer must not be deleted.
-    /// @return Pointer to managed object or nullptr if none owned.
+    /// @return Const pointer to managed object or errorHandler call if none owned.
     ///
     const T* get() const noexcept;
 

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/unique_ptr.inl

@@ -31,12 +31,6 @@ unique_ptr<T>::unique_ptr(T* const ptr, const function<void(T*)>& deleter) noexc
 {
 }
 
-template <typename T>
-unique_ptr<T>::unique_ptr(const function<void(T*)>& deleter) noexcept
-    : unique_ptr(nullptr, std::move(deleter))
-{
-}
-
 template <typename T>
 unique_ptr<T>& unique_ptr<T>::operator=(std::nullptr_t) noexcept
 {
@@ -84,12 +78,13 @@ const T* unique_ptr<T>::operator->() const noexcept
 template <typename T>
 unique_ptr<T>::operator bool() const noexcept
 {
-    return get() != nullptr;
+    return m_ptr != nullptr;
 }
 
 template <typename T>
 T* unique_ptr<T>::get() noexcept
 {
+    cxx::Expects(m_ptr != nullptr);
     return m_ptr;
 }
 

iceoryx_hoofs/test/moduletests/test_cxx_unique_ptr.cpp

@@ -59,18 +59,6 @@ class UniquePtrTest : public Test
     };
 };
 
-TEST_F(UniquePtrTest, CtorWithOnlyDeleterSetsPtrToNullAndDoesntCallDeleter)
-{
-    ::testing::Test::RecordProperty("TEST_ID", "a562a5d3-c9e1-49db-bf6c-7f9ee702c306");
-    {
-        auto sut = iox::cxx::unique_ptr<Position>(deleter);
-        EXPECT_FALSE(sut);
-        EXPECT_EQ(sut.get(), nullptr);
-    }
-    // SUT is out of scope but shouldn't have called deleter as SUT is NULL
-    EXPECT_FALSE(m_deleterCalled);
-}
-
 TEST_F(UniquePtrTest, CtorWithObjectPtrAndDeleterSetsPtrToObjectAndCallsDeleter)
 {
     ::testing::Test::RecordProperty("TEST_ID", "85a90fc3-e8b1-4c3d-a15c-ee7f64070b57");
@@ -86,18 +74,6 @@ TEST_F(UniquePtrTest, CtorWithObjectPtrAndDeleterSetsPtrToObjectAndCallsDeleter)
     EXPECT_TRUE(m_deleterCalled);
 }
 
-TEST_F(UniquePtrTest, CtorWithObjectPtrToNullAndDeleterSetsPtrToObjectAndDoesntCallsDeleter)
-{
-    ::testing::Test::RecordProperty("TEST_ID", "4b0377db-3db9-4103-870d-a7635d90f5b0");
-    {
-        auto sut = iox::cxx::unique_ptr<Position>(nullptr, deleter);
-        EXPECT_FALSE(sut);
-        EXPECT_EQ(sut.get(), nullptr);
-    }
-    // SUT is out of scope but shouldn't have called deleter as SUT is NULL
-    EXPECT_FALSE(m_deleterCalled);
-}
-
 TEST_F(UniquePtrTest, CtorUsingMoveWithObjectPtrAndDeleterSetsPtrToObjectAndCallsDeleter)
 {
     ::testing::Test::RecordProperty("TEST_ID", "88ae1d4c-d893-4633-9256-766d7e42bcc6");
@@ -224,6 +200,7 @@ TEST_F(UniquePtrTest, ReleaseAnObjectResultsInUniquePtrBeingInvalidAndReturnOfOb
     EXPECT_EQ(sut.release(), object);
     EXPECT_FALSE(sut);
     delete object;
+    EXPECT_FALSE(m_deleterCalled);
 }
 
 TEST_F(UniquePtrTest, ReleaseNullObjectResultsInUniquePtrBeingInvalidAndReturnOfNull)
@@ -235,15 +212,6 @@ TEST_F(UniquePtrTest, ReleaseNullObjectResultsInUniquePtrBeingInvalidAndReturnOf
     EXPECT_FALSE(sut);
 }
 
-TEST_F(UniquePtrTest, ReleaseDeleterOnlyUniquePtrResultsInUniquePtrBeingInvalidAndReturnOfNull)
-{
-    ::testing::Test::RecordProperty("TEST_ID", "2d20f154-7823-4332-a6f2-c56338e2b312");
-    auto sut = iox::cxx::unique_ptr<Position>(deleter);
-
-    EXPECT_EQ(sut.release(), nullptr);
-    EXPECT_FALSE(sut);
-}
-
 TEST_F(UniquePtrTest, ResetToAnExistingObjectPtrResultsInDeleterCalledTwice)
 {
     ::testing::Test::RecordProperty("TEST_ID", "e5da7713-e71d-49b2-8bf6-d6108aab6366");
@@ -298,54 +266,32 @@ TEST_F(UniquePtrTest, SwapTwoValidUniquePtrsWithDifferentDeletersSucceeds)
     EXPECT_TRUE(m_anotherDeleterCalled);
 }
 
-TEST_F(UniquePtrTest, SwapUniquePtrWithADeleterOnlyUniquePtrLeadsToDeletedUniquePtr)
+TEST_F(UniquePtrTest, SwapUniquePtrWithUniquePtrLeadsToCleanupOfBothInReverseOrder)
 {
     ::testing::Test::RecordProperty("TEST_ID", "9017ba22-ff18-41d4-8590-ccb0d7729435");
     {
         // NOLINTJUSTIFICATION no memory leak, object is deleted in the dtor deleter callback
-        // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
+        // NOLINTBEGIN(clang-analyzer-cplusplus.NewDeleteLeaks)
         auto* object = new Position();
+        auto* anotherObject = new Position();
+        // NOLINTEND(clang-analyzer-cplusplus.NewDeleteLeaks)
         auto sut = iox::cxx::unique_ptr<Position>(object, deleter);
         {
-            auto anotherSut = iox::cxx::unique_ptr<Position>(anotherDeleter);
+            auto anotherSut = iox::cxx::unique_ptr<Position>(anotherObject, anotherDeleter);
 
             sut.swap(anotherSut);
 
             // no deleter calls during swap
             EXPECT_FALSE(m_deleterCalled);
-            EXPECT_FALSE(sut);
+            EXPECT_TRUE(sut);
             EXPECT_EQ(anotherSut.get(), object);
         }
         // anotherSUT is out of scope and calls its deleter, which has been swapped and is now 'deleter'
         EXPECT_TRUE(m_deleterCalled);
+        EXPECT_FALSE(m_anotherDeleterCalled);
     }
-    // SUT is out of scope not calling its anotherDeleter as it's NULL
-    EXPECT_FALSE(m_anotherDeleterCalled);
-}
-
-TEST_F(UniquePtrTest, SwapADeleterOnlyUniquePtrWithUniquePtrLeadsToOneValidAndOneInvalidUniquePtrs)
-{
-    ::testing::Test::RecordProperty("TEST_ID", "0e7f9cf8-c240-468e-accf-27415fa0fcb1");
-    {
-        // NOLINTJUSTIFICATION no memory leak, object is deleted in the dtor deleter callback
-        // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
-        auto* anotherObject = new Position();
-        auto anotherSut = iox::cxx::unique_ptr<Position>(anotherObject, anotherDeleter);
-        {
-            auto sut = iox::cxx::unique_ptr<Position>(deleter);
-
-            sut.swap(anotherSut);
-
-            // no deleter calls during swap
-            EXPECT_FALSE(m_anotherDeleterCalled);
-            EXPECT_FALSE(anotherSut);
-            EXPECT_EQ(sut.get(), anotherObject);
-        }
-        // SUT is out of scope and calls its anotherDeleter, which has been swapped
-        EXPECT_TRUE(m_anotherDeleterCalled);
-    }
-    // anotherSUT is out of scope not calling its deleter as it's NULL
-    EXPECT_FALSE(m_deleterCalled);
+    // SUT is out of scope and calling anotherDeleter
+    EXPECT_TRUE(m_anotherDeleterCalled);
 }
 
 TEST_F(UniquePtrTest, CompareAUniquePtrWithItselfIsTrue)
@@ -460,11 +406,15 @@ TEST_F(UniquePtrTest, AssigningUniquePtrToNullptrDeletesTheManagedObject)
 TEST_F(UniquePtrTest, AssigningUniquePtrToNullptrSetsUnderlyingObjectToNullptr)
 {
     ::testing::Test::RecordProperty("TEST_ID", "eacf4bf4-0fa8-42dd-b0a7-c343a1959282");
-    // NOLINTJUSTIFICATION no memory leak, object is deleted in the dtor deleter callback
-    // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
-    auto* object = new Position();
-    auto sut = iox::cxx::unique_ptr<Position>(object, deleter);
-    sut = nullptr;
-    EXPECT_EQ(nullptr, sut.get());
+    EXPECT_DEATH(
+        {
+            // NOLINTJUSTIFICATION no memory leak, object is deleted in the dtor deleter callback
+            // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
+            auto* object = new Position();
+            auto sut = iox::cxx::unique_ptr<Position>(object, deleter);
+            sut = nullptr;
+            EXPECT_EQ(nullptr, sut.get());
+        },
+        "EXPECTS_ENSURES_FAILED");
 }
 } // namespace

iceoryx_posh/include/iceoryx_posh/internal/popo/smart_chunk.inl

@@ -84,7 +84,7 @@ inline const T& SmartChunk<TransmissionInterface, T, H>::operator*() const noexc
 template <typename TransmissionInterface, typename T, typename H>
 inline SmartChunk<TransmissionInterface, T, H>::operator bool() const noexcept
 {
-    return get() != nullptr;
+    return m_members.smartChunkUniquePtr.operator bool();
 }
 
 template <typename TransmissionInterface, typename T, typename H>

iceoryx_posh/test/moduletests/test_popo_request.cpp

@@ -39,7 +39,7 @@ TEST_F(Request_test, SendCallsInterfaceMockWithSuccessResult)
     auto sendResult = sutProducer.send();
 
     EXPECT_FALSE(sendResult.has_error());
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Request_test, SendOnMoveDestinationCallsInterfaceMockWithSuccessResult)
@@ -51,7 +51,7 @@ TEST_F(Request_test, SendOnMoveDestinationCallsInterfaceMockWithSuccessResult)
     auto sendResult = movedSut.send();
 
     EXPECT_FALSE(sendResult.has_error());
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Request_test, SendCallsInterfaceMockWithErrorResult)
@@ -65,7 +65,7 @@ TEST_F(Request_test, SendCallsInterfaceMockWithErrorResult)
 
     ASSERT_TRUE(sendResult.has_error());
     EXPECT_THAT(sendResult.get_error(), Eq(CLIENT_SEND_ERROR));
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Request_test, SendingAlreadySentRequestCallsErrorHandler)

iceoryx_posh/test/moduletests/test_popo_response.cpp

@@ -39,7 +39,7 @@ TEST_F(Response_test, SendCallsInterfaceMockWithSuccessResult)
     auto sendResult = sutProducer.send();
 
     EXPECT_FALSE(sendResult.has_error());
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Response_test, SendOnMoveDestinationCallsInterfaceMockWithSuccessResult)
@@ -51,7 +51,7 @@ TEST_F(Response_test, SendOnMoveDestinationCallsInterfaceMockWithSuccessResult)
     auto sendResult = movedSut.send();
 
     EXPECT_FALSE(sendResult.has_error());
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Response_test, SendCallsInterfaceMockWithErrorResult)
@@ -65,7 +65,7 @@ TEST_F(Response_test, SendCallsInterfaceMockWithErrorResult)
 
     ASSERT_TRUE(sendResult.has_error());
     EXPECT_THAT(sendResult.get_error(), Eq(SERVER_SEND_ERROR));
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Response_test, SendingAlreadySentResponseCallsErrorHandler)

iceoryx_posh/test/moduletests/test_popo_sample.cpp

@@ -38,7 +38,7 @@ TEST_F(Sample_test, SendCallsInterfaceMockWithSuccessResult)
 
     sutProducer.publish();
 
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Sample_test, SendOnMoveDestinationCallsInterfaceMock)
@@ -49,7 +49,7 @@ TEST_F(Sample_test, SendOnMoveDestinationCallsInterfaceMock)
     auto movedSut = std::move(sutProducer);
     movedSut.publish();
 
-    EXPECT_THAT(sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(sutProducer);
 }
 
 TEST_F(Sample_test, PublishingAlreadyPublishedSampleCallsErrorHandler)

iceoryx_posh/test/moduletests/test_popo_smart_chunk.cpp

@@ -58,11 +58,11 @@ TYPED_TEST(SmartChunkTest, SmartChunkIsInvalidatedAfterMoveConstruction)
     ::testing::Test::RecordProperty("TEST_ID", "90c8db15-6cf2-4dfc-a9ca-cb1333081fe3");
 
     typename TestFixture::SutProducerType producer(std::move(this->variation.sutProducer));
-    EXPECT_THAT(this->variation.sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(this->variation.sutProducer);
     EXPECT_THAT(producer.get(), Eq(this->variation.chunkMock.sample()));
 
     typename TestFixture::SutConsumerType consumer(std::move(this->variation.sutConsumer));
-    EXPECT_THAT(this->variation.sutConsumer.get(), Eq(nullptr));
+    EXPECT_FALSE(this->variation.sutConsumer);
     EXPECT_THAT(consumer.get(), Eq(this->variation.chunkMock.sample()));
 }
 
@@ -73,13 +73,13 @@ TYPED_TEST(SmartChunkTest, SmartChunkIsInvalidatedAfterMoveAssignment)
     this->variation.sutProducerForMove = std::move(this->variation.sutProducer);
     EXPECT_FALSE(this->variation.sutProducer);
     EXPECT_TRUE(this->variation.sutProducerForMove);
-    EXPECT_THAT(this->variation.sutProducer.get(), Eq(nullptr));
+    EXPECT_FALSE(this->variation.sutProducer);
     EXPECT_THAT(this->variation.sutProducerForMove.get(), Eq(this->variation.chunkMock.sample()));
 
     this->variation.sutConsumerForMove = std::move(this->variation.sutConsumer);
     EXPECT_FALSE(this->variation.sutConsumer);
     EXPECT_TRUE(this->variation.sutConsumerForMove);
-    EXPECT_THAT(this->variation.sutConsumer.get(), Eq(nullptr));
+    EXPECT_FALSE(this->variation.sutConsumer);
     EXPECT_THAT(this->variation.sutConsumerForMove.get(), Eq(this->variation.chunkMock.sample()));
 }