From: Razvan Becheriu Date: Tue, 5 Jul 2022 21:13:02 +0000 (+0300) Subject: [#2477] backport the fix for HA+MT CalloutHandle reset crash (#2743) to 2.0.3 X-Git-Tag: Kea-2.0.3~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=de0b7d2147318813b5249f299c471b7edafe4c89;p=thirdparty%2Fkea.git [#2477] backport the fix for HA+MT CalloutHandle reset crash (#2743) to 2.0.3 --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 4d732a2c07..3d6f56edd8 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1329,11 +1329,31 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp, CalloutHandlePtr callout_handle = getCalloutHandle(query); if (ctx && HooksManager::calloutsPresent(Hooks.hook_index_leases4_committed_)) { + // The ScopedCalloutHandleState class which guarantees that the task + // is added to the thread pool after the response is reset (if needed) + // and CalloutHandle state is reset. In ST it does nothing. + // A smart pointer is used to store the ScopedCalloutHandleState so that + // a copy of the pointer is created by the lambda and only on the + // destruction of the last reference the task is added. + // In MT there are 2 cases: + // 1. packet is unparked before current thread smart pointer to + // ScopedCalloutHandleState is destroyed: + // - the lamba uses the smart pointer to set the callout which adds the + // task, but the task is added after ScopedCalloutHandleState is + // destroyed, on the destruction of the last reference which is held + // by the current thread. + // 2. packet is unparked after the current thread smart pointer to + // ScopedCalloutHandleState is destroyed: + // - the current thread reference to ScopedCalloutHandleState is + // destroyed, but the reference in the lambda keeps it alive until + // the lamba is called and the last reference is released, at which + // time the task is actually added. // Use the RAII wrapper to make sure that the callout handle state is // reset when this object goes out of scope. All hook points must do // it to prevent possible circular dependency between the callout // handle and its arguments. - ScopedCalloutHandleState callout_handle_state(callout_handle); + std::shared_ptr callout_handle_state = + std::make_shared(callout_handle); ScopedEnableOptionsCopy query4_options_copy(query); @@ -1386,13 +1406,15 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp, // NEXT_STEP_PARK. Otherwise the callback we bind here will be // executed when the hook library unparks the packet. HooksManager::park("leases4_committed", query, - [this, callout_handle, query, rsp]() mutable { + [this, callout_handle, query, rsp, callout_handle_state]() mutable { if (MultiThreadingMgr::instance().getMode()) { typedef function CallBack; boost::shared_ptr call_back = boost::make_shared(std::bind(&Dhcpv4Srv::sendResponseNoThrow, this, callout_handle, query, rsp)); - MultiThreadingMgr::instance().getThreadPool().add(call_back); + callout_handle_state->on_completion_ = [call_back]() { + MultiThreadingMgr::instance().getThreadPool().add(call_back); + }; } else { processPacketPktSend(callout_handle, query, rsp); processPacketBufferSend(callout_handle, rsp); diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index b0c16d346c..584691d13b 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -978,12 +978,31 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) { if (!ctx.fake_allocation_ && (ctx.query_->getType() != DHCPV6_CONFIRM) && (ctx.query_->getType() != DHCPV6_INFORMATION_REQUEST) && HooksManager::calloutsPresent(Hooks.hook_index_leases6_committed_)) { - + // The ScopedCalloutHandleState class which guarantees that the task + // is added to the thread pool after the response is reset (if needed) + // and CalloutHandle state is reset. In ST it does nothing. + // A smart pointer is used to store the ScopedCalloutHandleState so that + // a copy of the pointer is created by the lambda and only on the + // destruction of the last reference the task is added. + // In MT there are 2 cases: + // 1. packet is unparked before current thread smart pointer to + // ScopedCalloutHandleState is destroyed: + // - the lamba uses the smart pointer to set the callout which adds the + // task, but the task is added after ScopedCalloutHandleState is + // destroyed, on the destruction of the last reference which is held + // by the current thread. + // 2. packet is unparked after the current thread smart pointer to + // ScopedCalloutHandleState is destroyed: + // - the current thread reference to ScopedCalloutHandleState is + // destroyed, but the reference in the lambda keeps it alive until + // the lamba is called and the last reference is released, at which + // time the task is actually added. // Use the RAII wrapper to make sure that the callout handle state is // reset when this object goes out of scope. All hook points must do // it to prevent possible circular dependency between the callout // handle and its arguments. - ScopedCalloutHandleState callout_handle_state(callout_handle); + std::shared_ptr callout_handle_state = + std::make_shared(callout_handle); ScopedEnableOptionsCopy query6_options_copy(query); @@ -1056,13 +1075,15 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) { // NEXT_STEP_PARK. Otherwise the callback we bind here will be // executed when the hook library unparks the packet. HooksManager::park("leases6_committed", query, - [this, callout_handle, query, rsp]() mutable { + [this, callout_handle, query, rsp, callout_handle_state]() mutable { if (MultiThreadingMgr::instance().getMode()) { typedef function CallBack; boost::shared_ptr call_back = boost::make_shared(std::bind(&Dhcpv6Srv::sendResponseNoThrow, this, callout_handle, query, rsp)); - MultiThreadingMgr::instance().getThreadPool().add(call_back); + callout_handle_state->on_completion_ = [call_back]() { + MultiThreadingMgr::instance().getThreadPool().add(call_back); + }; } else { processPacketPktSend(callout_handle, query, rsp); processPacketBufferSend(callout_handle, rsp); diff --git a/src/lib/hooks/callout_handle.cc b/src/lib/hooks/callout_handle.cc index e80760ac92..89a235a8fc 100644 --- a/src/lib/hooks/callout_handle.cc +++ b/src/lib/hooks/callout_handle.cc @@ -149,6 +149,10 @@ ScopedCalloutHandleState(const CalloutHandlePtr& callout_handle) ScopedCalloutHandleState::~ScopedCalloutHandleState() { resetState(); + + if (on_completion_) { + on_completion_(); + } } void diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h index b9e018b7c0..a8be978cfe 100644 --- a/src/lib/hooks/callout_handle.h +++ b/src/lib/hooks/callout_handle.h @@ -490,6 +490,9 @@ public: /// Resets state of the callout handle. ~ScopedCalloutHandleState(); + /// @brief Continuation callback. + std::function on_completion_; + private: /// @brief Resets the callout handle state.