From 390ab6b47d1ab8f8040851190585604362e03647 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Fri, 12 Aug 2022 15:19:13 +0200 Subject: [PATCH 1/3] iox-#1571 Removed unused c'tor and check for nullptr in unique_ptr::get() Signed-off-by: Simon Hoinkis --- .../include/iceoryx_hoofs/cxx/unique_ptr.hpp | 15 ++-- .../iceoryx_hoofs/internal/cxx/unique_ptr.inl | 9 +- .../test/moduletests/test_cxx_unique_ptr.cpp | 90 +++++-------------- 3 files changed, 27 insertions(+), 87 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/unique_ptr.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/unique_ptr.hpp index 08c2f36f89..494ecc9b5e 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/unique_ptr.hpp +++ b/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& 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; diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/unique_ptr.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/unique_ptr.inl index 258c9a1a0f..9309621c0f 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/unique_ptr.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/unique_ptr.inl @@ -31,12 +31,6 @@ unique_ptr::unique_ptr(T* const ptr, const function& deleter) noexc { } -template -unique_ptr::unique_ptr(const function& deleter) noexcept - : unique_ptr(nullptr, std::move(deleter)) -{ -} - template unique_ptr& unique_ptr::operator=(std::nullptr_t) noexcept { @@ -84,12 +78,13 @@ const T* unique_ptr::operator->() const noexcept template unique_ptr::operator bool() const noexcept { - return get() != nullptr; + return m_ptr != nullptr; } template T* unique_ptr::get() noexcept { + cxx::Expects(m_ptr != nullptr); return m_ptr; } diff --git a/iceoryx_hoofs/test/moduletests/test_cxx_unique_ptr.cpp b/iceoryx_hoofs/test/moduletests/test_cxx_unique_ptr.cpp index d27e0db338..c0a3a9f6e6 100644 --- a/iceoryx_hoofs/test/moduletests/test_cxx_unique_ptr.cpp +++ b/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(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(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(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(object, deleter); { - auto anotherSut = iox::cxx::unique_ptr(anotherDeleter); + auto anotherSut = iox::cxx::unique_ptr(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(anotherObject, anotherDeleter); - { - auto sut = iox::cxx::unique_ptr(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(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(object, deleter); + sut = nullptr; + EXPECT_EQ(nullptr, sut.get()); + }, + "EXPECTS_ENSURES_FAILED"); } } // namespace From 4a83834774ae7f7e2daa413f5b336158c0a8b502 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 17 Aug 2022 20:12:04 +0200 Subject: [PATCH 2/3] iox-#1571 Adapt tests to new behaviour of cxx::unique_ptr Signed-off-by: Simon Hoinkis --- .../include/iceoryx_posh/internal/popo/smart_chunk.inl | 2 +- iceoryx_posh/test/moduletests/test_popo_request.cpp | 6 +++--- iceoryx_posh/test/moduletests/test_popo_response.cpp | 6 +++--- iceoryx_posh/test/moduletests/test_popo_sample.cpp | 4 ++-- iceoryx_posh/test/moduletests/test_popo_smart_chunk.cpp | 8 ++++---- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/iceoryx_posh/include/iceoryx_posh/internal/popo/smart_chunk.inl b/iceoryx_posh/include/iceoryx_posh/internal/popo/smart_chunk.inl index db523e3d75..51fdec8a2b 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/popo/smart_chunk.inl +++ b/iceoryx_posh/include/iceoryx_posh/internal/popo/smart_chunk.inl @@ -84,7 +84,7 @@ inline const T& SmartChunk::operator*() const noexc template inline SmartChunk::operator bool() const noexcept { - return get() != nullptr; + return m_members.smartChunkUniquePtr.operator bool(); } template diff --git a/iceoryx_posh/test/moduletests/test_popo_request.cpp b/iceoryx_posh/test/moduletests/test_popo_request.cpp index 4fd38ce121..3d5e31a524 100644 --- a/iceoryx_posh/test/moduletests/test_popo_request.cpp +++ b/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) diff --git a/iceoryx_posh/test/moduletests/test_popo_response.cpp b/iceoryx_posh/test/moduletests/test_popo_response.cpp index 903ab48939..8318d87e0d 100644 --- a/iceoryx_posh/test/moduletests/test_popo_response.cpp +++ b/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) diff --git a/iceoryx_posh/test/moduletests/test_popo_sample.cpp b/iceoryx_posh/test/moduletests/test_popo_sample.cpp index 3fcdfb2337..28c030eceb 100644 --- a/iceoryx_posh/test/moduletests/test_popo_sample.cpp +++ b/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) diff --git a/iceoryx_posh/test/moduletests/test_popo_smart_chunk.cpp b/iceoryx_posh/test/moduletests/test_popo_smart_chunk.cpp index 3f453510e6..907267ac8a 100644 --- a/iceoryx_posh/test/moduletests/test_popo_smart_chunk.cpp +++ b/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())); } From c221852c2c45d0fba45be8c31e6542b05375f034 Mon Sep 17 00:00:00 2001 From: Simon Hoinkis Date: Wed, 17 Aug 2022 20:21:25 +0200 Subject: [PATCH 3/3] iox-#1571 Add release note entry for nullptr check in cxx::unique_ptr Signed-off-by: Simon Hoinkis --- doc/website/release-notes/iceoryx-unreleased.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/website/release-notes/iceoryx-unreleased.md b/doc/website/release-notes/iceoryx-unreleased.md index cdd2074c08..ab9893b2a5 100644 --- a/doc/website/release-notes/iceoryx-unreleased.md +++ b/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:**