From c414d5214bb0548b3c4c30ea12247d95aea496f0 Mon Sep 17 00:00:00 2001 From: Kerstin Keller Date: Mon, 22 Jan 2024 12:36:34 +0100 Subject: [PATCH 1/3] [Core] Fix Race Conditions in CMsgSubscriber - Properly implement destructor (in inherited classes)! -> Destroy may not be called in base CMsgSubscriber class as this can lead to stack unwinding (most likely causes an exception in destructor, or leads to already deleted functions of derived class being called when they are already deleted) - Correct order of RemReceiveCallback and resetting internal pointer - protect function pointer with mutex. - Make testcase "harder" to more easily trigger race conditions. --- .../include/ecal/msg/capnproto/subscriber.h | 8 ++ .../include/ecal/msg/flatbuffers/subscriber.h | 8 ++ .../include/ecal/msg/messagepack/subscriber.h | 8 ++ .../include/ecal/msg/protobuf/subscriber.h | 8 ++ .../core/include/ecal/msg/string/subscriber.h | 8 ++ ecal/core/include/ecal/msg/subscriber.h | 20 +++- testing/ecal/core_test/src/core_test.cpp | 95 ++++++++++--------- 7 files changed, 104 insertions(+), 51 deletions(-) diff --git a/ecal/core/include/ecal/msg/capnproto/subscriber.h b/ecal/core/include/ecal/msg/capnproto/subscriber.h index d330700137..3573f027d7 100644 --- a/ecal/core/include/ecal/msg/capnproto/subscriber.h +++ b/ecal/core/include/ecal/msg/capnproto/subscriber.h @@ -68,6 +68,14 @@ namespace eCAL { } + /** + * @brief Destructor + **/ + ~CBuilderSubscriber() override + { + Destroy(); + } + /** * @brief Copy Constructor is not available. **/ diff --git a/ecal/core/include/ecal/msg/flatbuffers/subscriber.h b/ecal/core/include/ecal/msg/flatbuffers/subscriber.h index c0c0acf973..619e429122 100644 --- a/ecal/core/include/ecal/msg/flatbuffers/subscriber.h +++ b/ecal/core/include/ecal/msg/flatbuffers/subscriber.h @@ -56,6 +56,14 @@ namespace eCAL { } + /** + * @brief Destructor + **/ + ~CSubscriber() override + { + Destroy(); + } + /** * @brief Copy Constructor is not available. **/ diff --git a/ecal/core/include/ecal/msg/messagepack/subscriber.h b/ecal/core/include/ecal/msg/messagepack/subscriber.h index d2334c3608..3367399788 100644 --- a/ecal/core/include/ecal/msg/messagepack/subscriber.h +++ b/ecal/core/include/ecal/msg/messagepack/subscriber.h @@ -59,6 +59,14 @@ namespace eCAL { } + /** + * @brief Destructor + **/ + ~CSubscriber() override + { + Destroy(); + } + /** * @brief Copy Constructor is not available. **/ diff --git a/ecal/core/include/ecal/msg/protobuf/subscriber.h b/ecal/core/include/ecal/msg/protobuf/subscriber.h index 3596f3060b..ce2a987eb2 100644 --- a/ecal/core/include/ecal/msg/protobuf/subscriber.h +++ b/ecal/core/include/ecal/msg/protobuf/subscriber.h @@ -74,6 +74,14 @@ namespace eCAL { } + /** + * @brief Destructor + **/ + ~CSubscriber() override + { + Destroy(); + } + /** * @brief Copy Constructor is not available. **/ diff --git a/ecal/core/include/ecal/msg/string/subscriber.h b/ecal/core/include/ecal/msg/string/subscriber.h index 694d6025b3..1662999040 100644 --- a/ecal/core/include/ecal/msg/string/subscriber.h +++ b/ecal/core/include/ecal/msg/string/subscriber.h @@ -62,6 +62,14 @@ namespace eCAL { } + /** + * @brief Destructor + **/ + ~CSubscriber() override + { + Destroy(); + } + /** * @brief Copy Constructor is not available. **/ diff --git a/ecal/core/include/ecal/msg/subscriber.h b/ecal/core/include/ecal/msg/subscriber.h index 3f6980f795..b0ffec1e1b 100644 --- a/ecal/core/include/ecal/msg/subscriber.h +++ b/ecal/core/include/ecal/msg/subscriber.h @@ -125,7 +125,8 @@ namespace eCAL return *this; } - virtual ~CMsgSubscriber() {} + virtual ~CMsgSubscriber() + {} /** * @brief Creates this object. @@ -207,7 +208,10 @@ namespace eCAL assert(IsCreated()); RemReceiveCallback(); - m_cb_callback = callback_; + { + std::lock_guard callback_lock(m_cb_callback_mutex); + m_cb_callback = callback_; + } auto callback = std::bind(&CMsgSubscriber::ReceiveCallback, this, std::placeholders::_1, std::placeholders::_2); return(CSubscriber::AddReceiveCallback(callback)); } @@ -219,9 +223,12 @@ namespace eCAL **/ bool RemReceiveCallback() { + bool ret = CSubscriber::RemReceiveCallback(); + + std::lock_guard callback_lock(m_cb_callback_mutex); if (m_cb_callback == nullptr) return(false); m_cb_callback = nullptr; - return(CSubscriber::RemReceiveCallback()); + return(ret); } protected: @@ -245,7 +252,11 @@ namespace eCAL private: void ReceiveCallback(const char* topic_name_, const struct eCAL::SReceiveCallbackData* data_) { - MsgReceiveCallbackT fn_callback(m_cb_callback); + MsgReceiveCallbackT fn_callback; + { + std::lock_guard callback_lock(m_cb_callback_mutex); + fn_callback = m_cb_callback; + } if(fn_callback == nullptr) return; @@ -256,6 +267,7 @@ namespace eCAL } } + std::mutex m_cb_callback_mutex; MsgReceiveCallbackT m_cb_callback; }; } diff --git a/testing/ecal/core_test/src/core_test.cpp b/testing/ecal/core_test/src/core_test.cpp index 32f22ebc85..73f4af2a1b 100644 --- a/testing/ecal/core_test/src/core_test.cpp +++ b/testing/ecal/core_test/src/core_test.cpp @@ -5,9 +5,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -96,7 +96,7 @@ TEST(Core, LeakedPubSub) std::this_thread::sleep_for(std::chrono::milliseconds(100)); #endif } - }); + }); // let them work together std::this_thread::sleep_for(std::chrono::seconds(2)); @@ -111,66 +111,67 @@ TEST(Core, LeakedPubSub) TEST(Core, CallbackDestruction) { - // initialize eCAL API - EXPECT_EQ(0, eCAL::Initialize(0, nullptr, "callback destruction")); + for (int i = 0; i < 10; ++i) + { + // initialize eCAL API + EXPECT_EQ(0, eCAL::Initialize(0, nullptr, "callback destruction")); - // enable loop back communication in the same thread - eCAL::Util::EnableLoopback(true); + // enable loop back communication in the same thread + eCAL::Util::EnableLoopback(true); - // create subscriber and register a callback - std::shared_ptr< eCAL::string::CSubscriber> sub; + // create subscriber and register a callback + std::shared_ptr> sub; - // create publisher - eCAL::string::CPublisher pub("foo"); + // create publisher + eCAL::string::CPublisher pub("foo"); - // start publishing thread - std::atomic pub_stop(false); - std::thread pub_t([&]() { - while (!pub_stop) - { - pub.Send("Hello World"); + // start publishing thread + std::atomic pub_stop(false); + std::thread pub_t([&]() { + while (!pub_stop) { + pub.Send("Hello World"); #if 0 - // some kind of busy waiting.... - int y = 0; - for (int i = 0; i < 100000; i++) - { - y += i; - } + // some kind of busy waiting.... + int y = 0; + for (int i = 0; i < 100000; i++) + { + y += i; + } #else - std::this_thread::sleep_for(std::chrono::milliseconds(100)); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); #endif - } - }); - - std::atomic sub_stop(false); - std::thread sub_t([&]() { - while (!sub_stop) - { - sub = std::make_shared>("foo"); - sub->AddReceiveCallback(std::bind(OnReceive, std::placeholders::_4)); - std::this_thread::sleep_for(std::chrono::seconds(2)); - } - }); + } + }); + + std::atomic sub_stop(false); + std::thread sub_t([&]() { + while (!sub_stop) { + sub = std::make_shared>("foo"); + sub->AddReceiveCallback(std::bind(OnReceive, std::placeholders::_4)); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + }); - // let them work together - std::this_thread::sleep_for(std::chrono::seconds(10)); + // let them work together + std::this_thread::sleep_for(std::chrono::seconds(10)); - // stop publishing thread - pub_stop = true; - pub_t.join(); + // stop publishing thread + pub_stop = true; + pub_t.join(); - sub_stop = true; - sub_t.join(); + sub_stop = true; + sub_t.join(); - // finalize eCAL API - // without destroying any pub / sub - EXPECT_EQ(0, eCAL::Finalize()); + // finalize eCAL API + // without destroying any pub / sub + EXPECT_EQ(0, eCAL::Finalize()); + } } /* excluded for now, system timer jitter too high */ #if 0 TEST(Core, TimerCallback) -{ +{ // initialize eCAL API EXPECT_EQ(0, eCAL::Initialize(0, nullptr, "timer callback")); From 184af737d4eb2c12aec3eba318a0054a4604c19b Mon Sep 17 00:00:00 2001 From: Kerstin Keller Date: Mon, 22 Jan 2024 12:56:26 +0100 Subject: [PATCH 2/3] Call this->destroy() --- ecal/core/include/ecal/msg/capnproto/subscriber.h | 2 +- ecal/core/include/ecal/msg/flatbuffers/subscriber.h | 2 +- ecal/core/include/ecal/msg/messagepack/subscriber.h | 2 +- ecal/core/include/ecal/msg/protobuf/subscriber.h | 2 +- ecal/core/include/ecal/msg/string/subscriber.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ecal/core/include/ecal/msg/capnproto/subscriber.h b/ecal/core/include/ecal/msg/capnproto/subscriber.h index 3573f027d7..02cb9edac5 100644 --- a/ecal/core/include/ecal/msg/capnproto/subscriber.h +++ b/ecal/core/include/ecal/msg/capnproto/subscriber.h @@ -73,7 +73,7 @@ namespace eCAL **/ ~CBuilderSubscriber() override { - Destroy(); + this->Destroy(); } /** diff --git a/ecal/core/include/ecal/msg/flatbuffers/subscriber.h b/ecal/core/include/ecal/msg/flatbuffers/subscriber.h index 619e429122..0b274b2f39 100644 --- a/ecal/core/include/ecal/msg/flatbuffers/subscriber.h +++ b/ecal/core/include/ecal/msg/flatbuffers/subscriber.h @@ -61,7 +61,7 @@ namespace eCAL **/ ~CSubscriber() override { - Destroy(); + this->Destroy(); } /** diff --git a/ecal/core/include/ecal/msg/messagepack/subscriber.h b/ecal/core/include/ecal/msg/messagepack/subscriber.h index 3367399788..ece4e0d4ef 100644 --- a/ecal/core/include/ecal/msg/messagepack/subscriber.h +++ b/ecal/core/include/ecal/msg/messagepack/subscriber.h @@ -64,7 +64,7 @@ namespace eCAL **/ ~CSubscriber() override { - Destroy(); + this->Destroy(); } /** diff --git a/ecal/core/include/ecal/msg/protobuf/subscriber.h b/ecal/core/include/ecal/msg/protobuf/subscriber.h index ce2a987eb2..ed057364f7 100644 --- a/ecal/core/include/ecal/msg/protobuf/subscriber.h +++ b/ecal/core/include/ecal/msg/protobuf/subscriber.h @@ -79,7 +79,7 @@ namespace eCAL **/ ~CSubscriber() override { - Destroy(); + this->Destroy(); } /** diff --git a/ecal/core/include/ecal/msg/string/subscriber.h b/ecal/core/include/ecal/msg/string/subscriber.h index 1662999040..8caccd038e 100644 --- a/ecal/core/include/ecal/msg/string/subscriber.h +++ b/ecal/core/include/ecal/msg/string/subscriber.h @@ -67,7 +67,7 @@ namespace eCAL **/ ~CSubscriber() override { - Destroy(); + this->Destroy(); } /** From 16b9b2829a188fd32b2cef6970d1a2d71eac37d9 Mon Sep 17 00:00:00 2001 From: Kerstin Keller Date: Mon, 22 Jan 2024 15:32:44 +0100 Subject: [PATCH 3/3] Clang tidy suggestions. --- ecal/core/include/ecal/msg/subscriber.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ecal/core/include/ecal/msg/subscriber.h b/ecal/core/include/ecal/msg/subscriber.h index b0ffec1e1b..0d342e0630 100644 --- a/ecal/core/include/ecal/msg/subscriber.h +++ b/ecal/core/include/ecal/msg/subscriber.h @@ -76,6 +76,8 @@ namespace eCAL { } + virtual ~CMsgSubscriber() = default; + /** * @brief Copy Constructor is not available. **/ @@ -125,9 +127,6 @@ namespace eCAL return *this; } - virtual ~CMsgSubscriber() - {} - /** * @brief Creates this object. * @@ -252,7 +251,7 @@ namespace eCAL private: void ReceiveCallback(const char* topic_name_, const struct eCAL::SReceiveCallbackData* data_) { - MsgReceiveCallbackT fn_callback; + MsgReceiveCallbackT fn_callback = nullptr; { std::lock_guard callback_lock(m_cb_callback_mutex); fn_callback = m_cb_callback;