From: Razvan Becheriu Date: Wed, 1 Oct 2025 11:24:46 +0000 (+0300) Subject: [#4140] fixed deadlock X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5182d815be0f6d91e60bc848b6b663a757dc43e9;p=thirdparty%2Fkea.git [#4140] fixed deadlock --- diff --git a/AUTHORS b/AUTHORS index c82a04477e..faf95ab197 100644 --- a/AUTHORS +++ b/AUTHORS @@ -317,3 +317,7 @@ We have received the following contributions: - Darwin4053 2025-06: Fixed arguments order in flex option hook library debug messages. + + - liyunqing_kylin + 2025-09: Reported and provided a patch that fixes a dead lock in ping-check + hooks library. diff --git a/ChangeLog b/ChangeLog index 99ce29bcf5..e315756be3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2404. [bug] razvan, liyunqing_kylin + Fixed deadlock in ping-check hooks library. Thanks to + liyunqing_kylin for reporting and providing a patch. + (Gitlab #4140) + Kea 3.1.2 (development) released on September 24, 2025 2403. [func] fdupont diff --git a/src/hooks/dhcp/ping_check/ping_channel.cc b/src/hooks/dhcp/ping_check/ping_channel.cc index 6a6a88c038..2e9e18f449 100644 --- a/src/hooks/dhcp/ping_check/ping_channel.cc +++ b/src/hooks/dhcp/ping_check/ping_channel.cc @@ -36,17 +36,19 @@ PingChannel::nextEchoInstanceNum() { PingChannel::PingChannel(IOServicePtr& io_service, NextToSendCallback next_to_send_cb, + UpdateToSendCallback update_to_send_cb, EchoSentCallback echo_sent_cb, ReplyReceivedCallback reply_received_cb, ShutdownCallback shutdown_cb) : io_service_(io_service), next_to_send_cb_(next_to_send_cb), + update_to_send_cb_(update_to_send_cb), echo_sent_cb_(echo_sent_cb), reply_received_cb_(reply_received_cb), shutdown_cb_(shutdown_cb), socket_(0), input_buf_(256), reading_(false), sending_(false), stopping_(false), mutex_(new std::mutex), - single_threaded_(!MultiThreadingMgr::instance().getMode()), + send_mutex_(new std::mutex), single_threaded_(!MultiThreadingMgr::instance().getMode()), watch_socket_(0), registered_write_fd_(-1), registered_read_fd_(-1) { if (!io_service_) { isc_throw(BadValue, @@ -310,24 +312,37 @@ PingChannel::startRead() { void PingChannel::sendNext() { try { + // Mutex used to do atomic read of the store entry using + // @ref PingContextStore::getNextToSend and update the context from + // WAITING_TO_SEND state to SENDING state using + // @ref PingContext::setState. Both functions (when transitioning from + // WAITING_TO_SEND state to SENDING state) should be called only + // with this mutex locked. + MultiThreadingLock send_lock(*send_mutex_); + + // Fetch the next one to send (outside the mutex). + PingContextPtr context = ((next_to_send_cb_)()); + if (!context) { + // Nothing to send. + return; + } + MultiThreadingLock lock(*mutex_); if (!canSend()) { // Can't send right now, get out. return; } - // Fetch the next one to send. - IOAddress target("0.0.0.0"); - if (!((next_to_send_cb_)(target))) { - // Nothing to send. - return; + // Update context to SENDING (inside the mutex). + if (update_to_send_cb_) { + (update_to_send_cb_)(context); } // Have an target IP, build an ECHO REQUEST for it. sending_ = true; ICMPMsgPtr next_echo(new ICMPMsg()); next_echo->setType(ICMPMsg::ECHO_REQUEST); - next_echo->setDestination(target); + next_echo->setDestination(context->getTarget()); uint32_t instance_num = nextEchoInstanceNum(); next_echo->setId(static_cast(instance_num >> 16)); @@ -345,7 +360,7 @@ PingChannel::sendNext() { ph::_1, // error ph::_2)); // bytes_transferred - ICMPEndpoint target_endpoint(target); + ICMPEndpoint target_endpoint(context->getTarget()); asyncSend(echo_icmp.get(), sizeof(struct icmp), &target_endpoint, cb); } catch (const std::exception& ex) { // Normal IO failures should be passed to the callback. A failure here diff --git a/src/hooks/dhcp/ping_check/ping_channel.h b/src/hooks/dhcp/ping_check/ping_channel.h index 064e7a2a82..0e00672d69 100644 --- a/src/hooks/dhcp/ping_check/ping_channel.h +++ b/src/hooks/dhcp/ping_check/ping_channel.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -66,8 +67,11 @@ typedef ICMPSocket PingSocket; /// @brief Defines a pointer to PingSocket. typedef boost::shared_ptr PingSocketPtr; -/// @brief Function type for callback that fetches next IOAddress to ping. -typedef std::function NextToSendCallback; +/// @brief Function type for callback to fetch a context with next target to ping. +typedef std::function NextToSendCallback; + +/// @brief Function type for callback to update a context to SENDING state. +typedef std::function UpdateToSendCallback; /// @brief Function type for callback to invoke upon ECHO send completion. typedef std::function EchoSentCallback; @@ -108,8 +112,10 @@ public: /// /// @param io_service pointer to the IOService instance that will manage /// the channel's IO. Must not be empty - /// @param next_to_send_cb callback to invoke to fetch the next IOAddress - /// to ping + /// @param next_to_send_cb callback to invoke to fetch the next context and + /// its target address to ping (called outside the mutex) + /// @param update_to_send_cb callback to invoke to update the selected + /// context to SENDING (called inside the mutex) /// @param echo_sent_cb callback to invoke when an ECHO send has completed /// @param reply_received_cb callback to invoke when an ICMP reply has been /// received. This callback is passed all inbound ICMP messages (e.g. ECHO @@ -120,6 +126,7 @@ public: /// @throw BadValue if io_service is empty. PingChannel(asiolink::IOServicePtr& io_service, NextToSendCallback next_to_send_cb, + UpdateToSendCallback update_to_send_cb, EchoSentCallback echo_sent_cb, ReplyReceivedCallback reply_received_cb, ShutdownCallback shutdown_cb = ShutdownCallback()); @@ -167,7 +174,7 @@ protected: /// @brief Receive data on the socket asynchronously /// /// Calls the underlying socket's asyncReceive() method to read a - /// packet of data from a remote endpoint. Arrival of the data is signalled + /// packet of data from a remote endpoint. Arrival of the data is signaled /// via a call to the callback function. /// /// This virtual function is provided as means to inject errors during @@ -315,9 +322,12 @@ protected: /// @brief IOService instance the drives socket IO asiolink::IOServicePtr io_service_; - /// @brief Callback to invoke to fetch the next address to ping. + /// @brief Callback to invoke to fetch the next context with target address to ping. NextToSendCallback next_to_send_cb_; + /// @brief Callback to invoke to update selected context to SENDING state. + UpdateToSendCallback update_to_send_cb_; + /// @brief Callback to invoke when an ECHO write has completed. EchoSentCallback echo_sent_cb_; @@ -348,6 +358,16 @@ protected: /// @brief The mutex used to protect internal state. const boost::scoped_ptr mutex_; + /// @brief The mutex used to protect internal state on send events. + /// + /// Mutex used to do atomic read of the store entry using + /// @ref PingContextStore::getNextToSend and update the context from + /// WAITING_TO_SEND state to SENDING state using + /// @ref PingContext::setState. Both functions (when transitioning from + /// WAITING_TO_SEND state to SENDING state) should be called only + /// with this mutex locked. + const boost::scoped_ptr send_mutex_; + /// @brief True if channel was opened in single-threaded mode, false /// otherwise. bool single_threaded_; diff --git a/src/hooks/dhcp/ping_check/ping_check_mgr.cc b/src/hooks/dhcp/ping_check/ping_check_mgr.cc index aadea02934..08cae08693 100644 --- a/src/hooks/dhcp/ping_check/ping_check_mgr.cc +++ b/src/hooks/dhcp/ping_check/ping_check_mgr.cc @@ -178,23 +178,22 @@ PingCheckMgr::startPing(dhcp::Lease4Ptr& lease, dhcp::Pkt4Ptr& query, hooks::Par startPing(lease, query, parking_lot, getGlobalConfig()); } -bool -PingCheckMgr::nextToSend(IOAddress& next) { - if (checkSuspended()) { - return (false); +PingContextPtr +PingCheckMgr::nextToSend() { + if (!checkSuspended()) { + return (store_->getNextToSend()); } - PingContextPtr context = store_->getNextToSend(); - if (!context) { - return (false); - } + return (PingContextPtr()); +} - next = context->getTarget(); +void +PingCheckMgr::updateContextToSend(PingContextPtr context) { // Transition to sending. + // Must not call @ref PingCheckMgr::checkSuspended() or + // it will cause a deadlock. context->setState(PingContext::SENDING); store_->updateContext(context); - - return (true); } void @@ -631,6 +630,8 @@ PingChannelPtr PingCheckMgr::createChannel(IOServicePtr io_service) { return (PingChannelPtr(new PingChannel(io_service, std::bind(&PingCheckMgr::nextToSend, + this), + std::bind(&PingCheckMgr::updateContextToSend, this, ph::_1), std::bind(&PingCheckMgr::sendCompleted, this, ph::_1, ph::_2), diff --git a/src/hooks/dhcp/ping_check/ping_check_mgr.h b/src/hooks/dhcp/ping_check/ping_check_mgr.h index 5f1b83e9fb..2e1ff971a2 100644 --- a/src/hooks/dhcp/ping_check/ping_check_mgr.h +++ b/src/hooks/dhcp/ping_check/ping_check_mgr.h @@ -118,16 +118,23 @@ public: hooks::ParkingLotHandlePtr& parking_lot); /// @brief Callback passed to PingChannel to use to retrieve the next - /// address to check. + /// context with address to check. /// /// Fetches the context which has been in the WAITING_TO_SEND state the - /// longest and returns its lease address. + /// longest and returns it. /// - /// @param[out] next upon return it will contain the next target address. - /// Contents are only meaningful if the function returns true. + /// @return The context selected to send, or null if none available. + virtual PingContextPtr nextToSend(); + + /// @brief Callback passed to PingChannel to update a context to SENDING + /// state just before sending. + /// + /// This is invoked inside the channel mutex. + /// Must not call @ref PingCheckMgr::checkSuspended() or + /// it will cause a deadlock. /// - /// @return True another target address exists, false otherwise. - virtual bool nextToSend(asiolink::IOAddress& next); + /// @param context The context to update. + virtual void updateContextToSend(PingContextPtr context); /// @brief Callback passed to PingChannel to invoke when an ECHO REQUEST /// send has completed. diff --git a/src/hooks/dhcp/ping_check/tests/ping_channel_unittests.cc b/src/hooks/dhcp/ping_check/tests/ping_channel_unittests.cc index 65e98a7228..5bb7bd3397 100644 --- a/src/hooks/dhcp/ping_check/tests/ping_channel_unittests.cc +++ b/src/hooks/dhcp/ping_check/tests/ping_channel_unittests.cc @@ -126,20 +126,24 @@ public: /// only meaningful if the function returns true. /// /// @return True another target address exists, false otherwise. - virtual bool nextToSend(IOAddress& next) { + virtual PingContextPtr nextToSend() { if (stopped_) { - return (false); + return (PingContextPtr()); } MultiThreadingLock lock(*mutex_); - bool use_next = true; - if (send_queue_.empty()) { - use_next = false; - } else { - next = send_queue_.front(); + if (!send_queue_.empty()) { + Lease4Ptr lease(new Lease4()); + lease->addr_ = send_queue_.front(); + Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 1234)); + return (PingContextPtr(new PingContext(lease, pkt))); } stopIfDone(); - return (use_next); + return (PingContextPtr()); + } + + /// @brief Callback to call to update context state (does nothing). + virtual void updateContextToSend(PingContextPtr /* context */) { } /// @brief Callback to invoke when an ECHO write has completed. @@ -315,7 +319,8 @@ PingChannelTest::sendReceiveTest(size_t num_threads, size_t num_targets /* = 25 // Create the channel instance with the appropriate io_service. ASSERT_NO_THROW_LOG(channel_.reset(new TestablePingChannel( channel_ios, - std::bind(&PingChannelTest::nextToSend, this, ph::_1), + std::bind(&PingChannelTest::nextToSend, this), + std::bind(&PingChannelTest::updateContextToSend, this, ph::_1), std::bind(&PingChannelTest::echoSent, this, ph::_1, ph::_2), std::bind(&PingChannelTest::replyReceived, this, ph::_1) ))); @@ -410,7 +415,8 @@ PingChannelTest::ioErrorTest(const std::function& set_error_trigger, // Create the channel instance with the appropriate io_service. ASSERT_NO_THROW_LOG(channel_.reset(new TestablePingChannel( channel_ios, - std::bind(&PingChannelTest::nextToSend, this, ph::_1), + std::bind(&PingChannelTest::nextToSend, this), + std::bind(&PingChannelTest::updateContextToSend, this, ph::_1), std::bind(&PingChannelTest::echoSent, this, ph::_1, ph::_2), std::bind(&PingChannelTest::replyReceived, this, ph::_1), ([this, &shutdown_cb_called]() { @@ -525,7 +531,8 @@ TEST_F(RootPingChannelTest, openCloseST) { // Create the channel instance. ASSERT_NO_THROW_LOG(channel_.reset(new TestablePingChannel( test_io_service_, - std::bind(&PingChannelTest::nextToSend, this, ph::_1), + std::bind(&PingChannelTest::nextToSend, this), + std::bind(&PingChannelTest::updateContextToSend, this, ph::_1), std::bind(&PingChannelTest::echoSent, this, ph::_1, ph::_2), std::bind(&PingChannelTest::replyReceived, this, ph::_1) ))); @@ -590,7 +597,8 @@ TEST_F(RootPingChannelTest, openCloseMT) { // Create the channel instance. ASSERT_NO_THROW_LOG(channel_.reset(new TestablePingChannel( test_io_service_, - std::bind(&PingChannelTest::nextToSend, this, ph::_1), + std::bind(&PingChannelTest::nextToSend, this), + std::bind(&PingChannelTest::updateContextToSend, this, ph::_1), std::bind(&PingChannelTest::echoSent, this, ph::_1, ph::_2), std::bind(&PingChannelTest::replyReceived, this, ph::_1) ))); diff --git a/src/hooks/dhcp/ping_check/tests/ping_check_mgr_unittests.cc b/src/hooks/dhcp/ping_check/tests/ping_check_mgr_unittests.cc index 8685597057..74b2d22131 100644 --- a/src/hooks/dhcp/ping_check/tests/ping_check_mgr_unittests.cc +++ b/src/hooks/dhcp/ping_check/tests/ping_check_mgr_unittests.cc @@ -322,9 +322,9 @@ protected: virtual PingChannelPtr createChannel(asiolink::IOServicePtr io_service) { return (TestablePingChannelPtr( new TestablePingChannel(io_service, - std::bind(&PingCheckMgr::nextToSend, this, ph::_1), - std::bind(&TestablePingCheckMgr::sendCompleted, - this, ph::_1, ph::_2), + std::bind(&PingCheckMgr::nextToSend, this), + std::bind(&PingCheckMgr::updateContextToSend, this, ph::_1), + std::bind(&TestablePingCheckMgr::sendCompleted, this, ph::_1, ph::_2), std::bind(&TestablePingCheckMgr::replyReceived, this, ph::_1), std::bind(&PingCheckMgr::channelShutdown, this)))); } @@ -845,9 +845,8 @@ public: // ST mode should ignore requested thread number. createMgr(3, 2, 250, true); - // Calling nextToSend() should return false. - IOAddress next("0.0.0.0"); - ASSERT_FALSE(mgr_->nextToSend(next)); + // Calling nextToSend() should return null. + ASSERT_FALSE(mgr_->nextToSend()); // Now let's start 3 contexts. size_t num_targets = 3; @@ -877,15 +876,19 @@ public: // Consecutive calls to nextToSend() should return target addresses // in the order they were created. + PingContextPtr ctx; for (auto const& lqp : lease_query_pairs_) { // Next to send should return the next address to send. - ASSERT_TRUE(mgr_->nextToSend(next)); + ASSERT_TRUE(ctx = mgr_->nextToSend()); + + // Update the context state explicitly. + ASSERT_NO_THROW(mgr_->updateContextToSend(ctx)); // It should match the lease as created. - ASSERT_EQ(next, lqp.lease_->addr_); + ASSERT_EQ(ctx->getTarget(), lqp.lease_->addr_); // Fetch the corresponding context. - PingContextPtr context = getContext(next); + PingContextPtr context = getContext(ctx->getTarget()); ASSERT_TRUE(context); // Verify the state has properly moved to SENDING. @@ -895,8 +898,8 @@ public: EXPECT_EQ(PingContext::SENDING, context->getState()); } - // A final call to nextToSend should return false. - ASSERT_FALSE(mgr_->nextToSend(next)); + // A final call to nextToSend should return null. + ASSERT_FALSE(mgr_->nextToSend()); } /// @brief Exercises PingCheckMgr::setNextExpiration. @@ -1322,9 +1325,8 @@ public: ASSERT_NO_THROW_LOG(mgr_->stop()); ASSERT_TRUE(mgr_->isStopped()); - // Calling nextToSend() should return false. - IOAddress next("0.0.0.0"); - ASSERT_FALSE(mgr_->nextToSend(next)); + // Calling nextToSend() should return null. + ASSERT_FALSE(mgr_->nextToSend()); // We should have as many declines as we have pairs created. compareLeaseQueryPairs(declines_); @@ -1423,9 +1425,8 @@ public: ASSERT_NO_THROW_LOG(mgr_->stop()); ASSERT_TRUE(mgr_->isStopped()); - // Calling nextToSend() should return false. - IOAddress next("0.0.0.0"); - ASSERT_FALSE(mgr_->nextToSend(next)); + // Calling nextToSend() should return null. + ASSERT_FALSE(mgr_->nextToSend()); // We should have as many declines as we have pairs created. compareLeaseQueryPairs(declines_); diff --git a/src/hooks/dhcp/ping_check/tests/ping_test_utils.h b/src/hooks/dhcp/ping_check/tests/ping_test_utils.h index df1ede7526..99c41eeb52 100644 --- a/src/hooks/dhcp/ping_check/tests/ping_test_utils.h +++ b/src/hooks/dhcp/ping_check/tests/ping_test_utils.h @@ -106,10 +106,11 @@ public: /// @throw BadValue if io_service is empty. TestablePingChannel(asiolink::IOServicePtr& io_service, NextToSendCallback next_to_send_cb, + UpdateToSendCallback update_to_send_cb, EchoSentCallback echo_sent_cb, ReplyReceivedCallback reply_received_cb, ShutdownCallback shutdown_cb = ShutdownCallback()) - : PingChannel(io_service, next_to_send_cb, echo_sent_cb, reply_received_cb, shutdown_cb), + : PingChannel(io_service, next_to_send_cb, update_to_send_cb, echo_sent_cb, reply_received_cb, shutdown_cb), read_number_(0), throw_on_read_number_(0), ec_on_read_number_(0), read_error_ec_(), write_number_(0), throw_on_write_number_(0), ec_on_write_number_(0), write_error_ec_(), route_loopback_(true), loopback_map_(), stopped_(false) {