From cb859eb4f43424c97e1c5d8bc9017c11074a498e Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 2 Jul 2018 17:27:22 +0200 Subject: [PATCH] [5664] Implemented fix for callout handle store memory leak. --- src/bin/dhcp4/dhcp4_srv.cc | 76 ++++++++++------ src/bin/dhcp6/dhcp6_srv.cc | 76 +++++++++++----- src/lib/dhcpsrv/Makefile.am | 2 +- src/lib/dhcpsrv/alloc_engine.cc | 148 ++++++++++++++++++++++---------- src/lib/dhcpsrv/alloc_engine.h | 29 +++++-- src/lib/hooks/Makefile.am | 2 +- src/lib/hooks/callout_handle.cc | 24 +++++- src/lib/hooks/callout_handle.h | 66 ++++++++++++++ 8 files changed, 316 insertions(+), 107 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index dfda5279b6..4f5c8fa521 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -369,8 +369,11 @@ Dhcpv4Exchange::setHostIdentifiers() { Host::IdentifierType type = Host::IDENT_FLEX; std::vector id; - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Pass incoming packet as argument callout_handle->setArgument("query4", context_->query_); @@ -530,8 +533,11 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, bool& drop, HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // We're reusing callout_handle from previous calls - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(query); @@ -648,8 +654,11 @@ Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query, bool& drop, HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // We're reusing callout_handle from previous calls - callout_handle->deleteAllArguments(); + // 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); // Set new arguments callout_handle->setArgument("query4", query); @@ -845,8 +854,11 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) { if (HooksManager::calloutsPresent(Hooks.hook_index_buffer4_receive_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(query); @@ -957,8 +969,11 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) { if (HooksManager::calloutsPresent(Hooks.hook_index_pkt4_receive_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(query); @@ -1041,11 +1056,11 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) { if (ctx && HooksManager::calloutsPresent(Hooks.hook_index_leases4_committed_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Delete all previous arguments - callout_handle->deleteAllArguments(); - - // Clear skip flag if it was set in previous callouts - callout_handle->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); + // 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); ScopedEnableOptionsCopy query4_options_copy(query); @@ -1124,11 +1139,11 @@ Dhcpv4Srv::processPacketPktSend(hooks::CalloutHandlePtr& callout_handle, // Execute all callouts registered for pkt4_send if (HooksManager::calloutsPresent(Hooks.hook_index_pkt4_send_)) { - // Delete all previous arguments - callout_handle->deleteAllArguments(); - - // Clear skip flag if it was set in previous callouts - callout_handle->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); + // 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); // Enable copying options from the query and response packets within // hook library. @@ -1183,8 +1198,11 @@ Dhcpv4Srv::processPacketBufferSend(CalloutHandlePtr& callout_handle, // Let's execute all callouts registered for buffer4_send if (HooksManager::calloutsPresent(Hooks.hook_index_buffer4_send_)) { - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy resp4_options_copy(rsp); @@ -2674,8 +2692,11 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& cont if (HooksManager::calloutsPresent(Hooks.hook_index_lease4_release_)) { CalloutHandlePtr callout_handle = getCalloutHandle(release); - // Delete all previous arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(release); @@ -2824,8 +2845,11 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline, if (HooksManager::calloutsPresent(Hooks.hook_index_lease4_decline_)) { CalloutHandlePtr callout_handle = getCalloutHandle(decline); - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(decline); diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 9ed2a3ff70..684ab39805 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -346,8 +346,11 @@ Dhcpv6Srv::initContext(const Pkt6Ptr& pkt, Host::IdentifierType type = Host::IDENT_FLEX; std::vector id; - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Pass incoming packet as argument callout_handle->setArgument("query6", pkt); @@ -520,12 +523,15 @@ Dhcpv6Srv::processPacket(Pkt6Ptr& query, Pkt6Ptr& rsp) { if (HooksManager::calloutsPresent(Hooks.hook_index_buffer6_receive_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); + // 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); + // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(query); - // Delete previously set arguments - callout_handle->deleteAllArguments(); - // Pass incoming packet as argument callout_handle->setArgument("query6", query); @@ -637,8 +643,11 @@ Dhcpv6Srv::processPacket(Pkt6Ptr& query, Pkt6Ptr& rsp) { if (HooksManager::calloutsPresent(Hooks.hook_index_pkt6_receive_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(query); @@ -784,11 +793,11 @@ Dhcpv6Srv::processPacket(Pkt6Ptr& query, Pkt6Ptr& rsp) { HooksManager::calloutsPresent(Hooks.hook_index_leases6_committed_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Delete all previous arguments - callout_handle->deleteAllArguments(); - - // Clear skip flag if it was set in previous callouts - callout_handle->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); + // 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); ScopedEnableOptionsCopy query6_options_copy(query); @@ -888,12 +897,15 @@ Dhcpv6Srv::processPacketPktSend(hooks::CalloutHandlePtr& callout_handle, // Execute all callouts registered for packet6_send if (HooksManager::calloutsPresent(Hooks.hook_index_pkt6_send_)) { + // 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); + // Enable copying options from the packets within hook library. ScopedEnableOptionsCopy query_resp_options_copy(query, rsp); - // Delete all previous arguments - callout_handle->deleteAllArguments(); - // Pass incoming packet as argument callout_handle->setArgument("query6", query); @@ -948,8 +960,11 @@ Dhcpv6Srv::processPacketBufferSend(CalloutHandlePtr& callout_handle, // Let's execute all callouts registered for buffer6_send if (HooksManager::calloutsPresent(Hooks.hook_index_buffer6_send_)) { - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy response6_options_copy(rsp); @@ -1354,8 +1369,11 @@ Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question, bool& drop) { if (HooksManager::calloutsPresent(Hooks.hook_index_subnet6_select_)) { CalloutHandlePtr callout_handle = getCalloutHandle(question); - // We're reusing callout_handle from previous calls - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(question); @@ -2451,6 +2469,12 @@ Dhcpv6Srv::releaseIA_NA(const DuidPtr& duid, const Pkt6Ptr& query, if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_release_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); + // 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); + // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(query); @@ -2614,8 +2638,11 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_release_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); - // Delete all previous arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(query); @@ -3155,8 +3182,11 @@ Dhcpv6Srv::declineLease(const Pkt6Ptr& decline, const Lease6Ptr lease, if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_decline_)) { CalloutHandlePtr callout_handle = getCalloutHandle(decline); - // Delete previously set arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(decline); diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am index 564f623232..f8ef361b51 100644 --- a/src/lib/dhcpsrv/Makefile.am +++ b/src/lib/dhcpsrv/Makefile.am @@ -221,7 +221,7 @@ libkea_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/util/libkea-util.la libkea_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la libkea_dhcpsrv_la_LIBADD += $(LOG4CPLUS_LIBS) $(CRYPTO_LIBS) $(BOOST_LIBS) -libkea_dhcpsrv_la_LDFLAGS = -no-undefined -version-info 10:0:0 +libkea_dhcpsrv_la_LDFLAGS = -no-undefined -version-info 11:0:0 libkea_dhcpsrv_la_LDFLAGS += $(CRYPTO_LDFLAGS) if HAVE_MYSQL libkea_dhcpsrv_la_LDFLAGS += $(MYSQL_LIBS) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 391915b37d..c48f3857f5 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -768,6 +768,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { Pool6Ptr pool; + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + while (subnet) { if (!subnet->clientSupported(ctx.query_->getClasses())) { @@ -813,7 +815,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { // The hint is valid and not currently used, let's create a // lease for it - lease = createLease6(ctx, hint, pool->getLength()); + lease = createLease6(ctx, hint, pool->getLength(), callout_status); // It can happen that the lease allocation failed (we could // have lost the race condition. That means that the hint is @@ -852,7 +854,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { ctx.currentIA().old_leases_.push_back(old_lease); /// We found a lease and it is expired, so we can reuse it - lease = reuseExpiredLease(lease, ctx, pool->getLength()); + lease = reuseExpiredLease(lease, ctx, pool->getLength(), + callout_status); /// @todo: We support only one lease per ia for now leases.push_back(lease); @@ -966,7 +969,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { // free. Let's allocate it. ctx.subnet_ = subnet; - Lease6Ptr lease = createLease6(ctx, candidate, prefix_len); + Lease6Ptr lease = createLease6(ctx, candidate, prefix_len, callout_status); if (lease) { // We are allocating a new lease (not renewing). So, the // old lease should be NULL. @@ -976,8 +979,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { return (leases); } else if (ctx.callout_handle_ && - (ctx.callout_handle_->getStatus() != - CalloutHandle::NEXT_STEP_CONTINUE)) { + (callout_status != CalloutHandle::NEXT_STEP_CONTINUE)) { // Don't retry when the callout status is not continue. break; } @@ -993,7 +995,8 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) { ctx.currentIA().old_leases_.push_back(old_lease); ctx.subnet_ = subnet; - existing = reuseExpiredLease(existing, ctx, prefix_len); + existing = reuseExpiredLease(existing, ctx, prefix_len, + callout_status); leases.push_back(existing); return (leases); @@ -1149,7 +1152,8 @@ AllocEngine::allocateReservedLeases6(ClientContext6& ctx, } // Ok, let's create a new lease... - Lease6Ptr lease = createLease6(ctx, addr, prefix_len); + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + Lease6Ptr lease = createLease6(ctx, addr, prefix_len, callout_status); // ... and add it to the existing leases list. existing_leases.push_back(lease); @@ -1405,7 +1409,8 @@ AllocEngine::removeNonreservedLeases6(ClientContext6& ctx, Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, - uint8_t prefix_len) { + uint8_t prefix_len, + CalloutHandle::CalloutNextStep& callout_status) { if (!expired->expired()) { isc_throw(BadValue, "Attempt to recycle lease that is still valid"); @@ -1446,8 +1451,11 @@ AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, if (ctx.callout_handle_ && HooksManager::getHooksManager().calloutsPresent(hook_index_lease6_select_)) { - // Delete all previous arguments - ctx.callout_handle_->deleteAllArguments(); + // 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(ctx.callout_handle_); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(ctx.query_); @@ -1469,10 +1477,12 @@ AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, // Call the callouts HooksManager::callCallouts(hook_index_lease6_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE6_SELECT_SKIP); return (Lease6Ptr()); } @@ -1512,7 +1522,8 @@ AllocEngine::reuseExpiredLease(Lease6Ptr& expired, ClientContext6& ctx, Lease6Ptr AllocEngine::createLease6(ClientContext6& ctx, const IOAddress& addr, - uint8_t prefix_len) { + uint8_t prefix_len, + CalloutHandle::CalloutNextStep& callout_status) { if (ctx.currentIA().type_ != Lease::TYPE_PD) { prefix_len = 128; // non-PD lease types must be always /128 @@ -1532,8 +1543,11 @@ Lease6Ptr AllocEngine::createLease6(ClientContext6& ctx, if (ctx.callout_handle_ && HooksManager::getHooksManager().calloutsPresent(hook_index_lease6_select_)) { - // Delete all previous arguments - ctx.callout_handle_->deleteAllArguments(); + // 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(ctx.callout_handle_); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(ctx.query_); @@ -1553,10 +1567,12 @@ Lease6Ptr AllocEngine::createLease6(ClientContext6& ctx, // This is the first callout, so no need to clear any arguments HooksManager::callCallouts(hook_index_lease6_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE6_SELECT_SKIP); return (Lease6Ptr()); } @@ -1780,8 +1796,11 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { if (HooksManager::calloutsPresent(hook_point)) { CalloutHandlePtr callout_handle = ctx.callout_handle_; - // Delete all previous arguments - callout_handle->deleteAllArguments(); + // 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); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query6_options_copy(ctx.query_); @@ -2199,6 +2218,13 @@ AllocEngine::reclaimExpiredLease(const Lease6Ptr& lease, // will not update DNS nor update the database. bool skipped = false; if (callout_handle) { + + // 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); + callout_handle->deleteAllArguments(); callout_handle->setArgument("lease6", lease); callout_handle->setArgument("remove_lease", reclaim_mode == DB_RECLAIM_REMOVE); @@ -2289,7 +2315,13 @@ AllocEngine::reclaimExpiredLease(const Lease4Ptr& lease, // will not update DNS nor update the database. bool skipped = false; if (callout_handle) { - callout_handle->deleteAllArguments(); + + // 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); + callout_handle->setArgument("lease4", lease); callout_handle->setArgument("remove_lease", reclaim_mode == DB_RECLAIM_REMOVE); @@ -2390,8 +2422,11 @@ AllocEngine::reclaimDeclined(const Lease4Ptr& lease) { callout_handle = HooksManager::createCalloutHandle(); } - // Delete all previous arguments - callout_handle->deleteAllArguments(); + // 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); // Pass necessary arguments callout_handle->setArgument("lease4", lease); @@ -2448,8 +2483,11 @@ AllocEngine::reclaimDeclined(const Lease6Ptr& lease) { callout_handle = HooksManager::createCalloutHandle(); } - // Delete all previous arguments - callout_handle->deleteAllArguments(); + // 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); // Pass necessary arguments callout_handle->setArgument("lease6", lease); @@ -2916,6 +2954,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // caller. Lease4Ptr new_lease; + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + // Check if there is a reservation for the client. If there is, we want to // assign the reserved address, rather than any other one. if (hasAddressReservation(ctx)) { @@ -2936,7 +2976,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // Note that we don't remove the existing client's lease at this point // because this is not a real allocation, we just offer what we can // allocate in the DHCPREQUEST time. - new_lease = allocateOrReuseLease4(ctx.currentHost()->getIPv4Reservation(), ctx); + new_lease = allocateOrReuseLease4(ctx.currentHost()->getIPv4Reservation(), ctx, + callout_status); if (!new_lease) { LOG_WARN(alloc_engine_logger, ALLOC_ENGINE_V4_DISCOVER_ADDRESS_CONFLICT) .arg(ctx.query_->getLabel()) @@ -2985,7 +3026,8 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { .arg(ctx.requested_address_.toText()) .arg(ctx.query_->getLabel()); - new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx); + new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx, + callout_status); } // The allocation engine failed to allocate all of the candidate @@ -3156,7 +3198,9 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // or the existing lease has expired. If the allocation fails, // e.g. because the lease is in use, we will return NULL to // indicate that we were unable to allocate the lease. - new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx); + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; + new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx, + callout_status); } else { @@ -3195,7 +3239,8 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { } Lease4Ptr -AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr) { +AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr, + CalloutHandle::CalloutNextStep& callout_status) { if (!ctx.hwaddr_) { isc_throw(BadValue, "Can't create a lease with NULL HW address"); } @@ -3226,8 +3271,11 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr) { if (ctx.callout_handle_ && HooksManager::getHooksManager().calloutsPresent(hook_index_lease4_select_)) { - // Delete all previous arguments - ctx.callout_handle_->deleteAllArguments(); + // 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(ctx.callout_handle_); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(ctx.query_); @@ -3252,10 +3300,12 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr) { // This is the first callout, so no need to clear any arguments HooksManager::callCallouts(hook_index_lease4_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE4_SELECT_SKIP); return (Lease4Ptr()); } @@ -3331,8 +3381,11 @@ AllocEngine::renewLease4(const Lease4Ptr& lease, if (HooksManager::getHooksManager(). calloutsPresent(Hooks.hook_index_lease4_renew_)) { - // Delete all previous arguments - ctx.callout_handle_->deleteAllArguments(); + // 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(ctx.callout_handle_); // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(ctx.query_); @@ -3395,7 +3448,8 @@ AllocEngine::renewLease4(const Lease4Ptr& lease, Lease4Ptr AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, - AllocEngine::ClientContext4& ctx) { + AllocEngine::ClientContext4& ctx, + CalloutHandle::CalloutNextStep& callout_status) { if (!expired) { isc_throw(BadValue, "null lease specified for reuseExpiredLease"); } @@ -3426,8 +3480,11 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, // Enable copying options from the packet within hook library. ScopedEnableOptionsCopy query4_options_copy(ctx.query_); - // Delete all previous arguments - ctx.callout_handle_->deleteAllArguments(); + // 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(ctx.callout_handle_); // Pass necessary arguments // Pass the original client query @@ -3450,10 +3507,12 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, // Call the callouts HooksManager::callCallouts(hook_index_lease4_select_, *ctx.callout_handle_); + callout_status = ctx.callout_handle_->getStatus(); + // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease // won't be inserted into the database. - if (ctx.callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if (callout_status == CalloutHandle::NEXT_STEP_SKIP) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE4_SELECT_SKIP); return (Lease4Ptr()); @@ -3485,14 +3544,15 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, } Lease4Ptr -AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& ctx) { +AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& ctx, + CalloutHandle::CalloutNextStep& callout_status) { ctx.conflicting_lease_.reset(); Lease4Ptr exist_lease = LeaseMgrFactory::instance().getLease4(candidate); if (exist_lease) { if (exist_lease->expired()) { ctx.old_lease_ = Lease4Ptr(new Lease4(*exist_lease)); - return (reuseExpiredLease4(exist_lease, ctx)); + return (reuseExpiredLease4(exist_lease, ctx, callout_status)); } else { // If there is a lease and it is not expired, pass this lease back @@ -3502,7 +3562,7 @@ AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& c } } else { - return (createLease4(ctx, candidate)); + return (createLease4(ctx, candidate, callout_status)); } return (Lease4Ptr()); } @@ -3552,12 +3612,7 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) { max_attempts = 0; } - // Set the default status code in case the lease4_select callouts - // do not exist and the callout handle has a status returned by - // any of the callouts already invoked for this packet. - if (ctx.callout_handle_) { - ctx.callout_handle_->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); - } + CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; for (uint64_t i = 0; i < max_attempts; ++i) { IOAddress candidate = allocator->pickAddress(subnet, @@ -3570,13 +3625,12 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) { // The call below will return the non-NULL pointer if we // successfully allocate this lease. This means that the // address is not in use by another client. - new_lease = allocateOrReuseLease4(candidate, ctx); + new_lease = allocateOrReuseLease4(candidate, ctx, callout_status); if (new_lease) { return (new_lease); } else if (ctx.callout_handle_ && - (ctx.callout_handle_->getStatus() != - CalloutHandle::NEXT_STEP_CONTINUE)) { + (callout_status != CalloutHandle::NEXT_STEP_CONTINUE)) { // Don't retry when the callout status is not continue. subnet.reset(); break; diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 4b1dce5c73..3a64eb91a4 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -761,6 +761,7 @@ private: /// available /// @param prefix_len length of the prefix (for PD only) /// should be 128 for other lease types + /// @param [out] callout_status callout returned by the lease6_select /// /// The following fields of the ctx structure are used: /// @ref ClientContext6::subnet_ subnet the lease is allocated from @@ -784,7 +785,8 @@ private: /// became unavailable) Lease6Ptr createLease6(ClientContext6& ctx, const isc::asiolink::IOAddress& addr, - const uint8_t prefix_len); + const uint8_t prefix_len, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Allocates a normal, in-pool, unreserved lease from the pool. /// @@ -859,6 +861,7 @@ private: /// @param ctx client context that contains all details. /// @param prefix_len prefix length (for PD leases) /// Should be 128 for other lease types + /// @param [out] callout_status callout returned by the lease6_select /// /// The following parameters are used from the ctx structure: /// @ref ClientContext6::subnet_ subnet the lease is allocated from @@ -879,9 +882,11 @@ private: /// /// @return refreshed lease /// @throw BadValue if trying to recycle lease that is still valid - Lease6Ptr reuseExpiredLease(Lease6Ptr& expired, - ClientContext6& ctx, - uint8_t prefix_len); + Lease6Ptr + reuseExpiredLease(Lease6Ptr& expired, + ClientContext6& ctx, + uint8_t prefix_len, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Updates FQDN and Client's Last Transmission Time /// for a collection of leases. @@ -1375,6 +1380,7 @@ private: /// /// @param ctx client context that contains additional parameters. /// @param addr An address that was selected and is confirmed to be available + /// @param [out] callout_status callout returned by the lease6_select /// /// In particular, the following fields from Client context are used: /// - @ref ClientContext4::subnet_ Subnet the lease is allocated from @@ -1395,7 +1401,8 @@ private: /// @return allocated lease (or NULL in the unlikely case of the lease just /// become unavailable) Lease4Ptr createLease4(const ClientContext4& ctx, - const isc::asiolink::IOAddress& addr); + const isc::asiolink::IOAddress& addr, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Renews a DHCPv4 lease. /// @@ -1422,11 +1429,14 @@ private: /// @param expired An old, expired lease. /// @param ctx Message processing context. It holds various information /// extracted from the client's message and required to allocate a lease. + /// @param [out] callout_status callout returned by the lease4_select /// /// @return Updated lease instance. /// @throw BadValue if trying to reuse a lease which is still valid or /// when the provided parameters are invalid. - Lease4Ptr reuseExpiredLease4(Lease4Ptr& expired, ClientContext4& ctx); + Lease4Ptr + reuseExpiredLease4(Lease4Ptr& expired, ClientContext4& ctx, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Allocates the lease by replacing an existing lease. /// @@ -1439,11 +1449,14 @@ private: /// allocated. /// @param ctx Client context holding the data extracted from the /// client's message. + /// @param [out] callout_status callout returned by the lease4_select /// /// @return A pointer to the allocated lease or NULL if the allocation /// was not successful. - Lease4Ptr allocateOrReuseLease4(const asiolink::IOAddress& address, - ClientContext4& ctx); + Lease4Ptr + allocateOrReuseLease4(const asiolink::IOAddress& address, + ClientContext4& ctx, + hooks::CalloutHandle::CalloutNextStep& callout_status); /// @brief Allocates the lease from the dynamic pool. /// diff --git a/src/lib/hooks/Makefile.am b/src/lib/hooks/Makefile.am index 062dfe3152..07a9f45845 100644 --- a/src/lib/hooks/Makefile.am +++ b/src/lib/hooks/Makefile.am @@ -51,7 +51,7 @@ nodist_libkea_hooks_la_SOURCES = hooks_messages.cc hooks_messages.h libkea_hooks_la_CXXFLAGS = $(AM_CXXFLAGS) libkea_hooks_la_CPPFLAGS = $(AM_CPPFLAGS) -libkea_hooks_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined -version-info 6:0:0 +libkea_hooks_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined -version-info 7:0:0 libkea_hooks_la_LIBADD = libkea_hooks_la_LIBADD += $(top_builddir)/src/lib/log/libkea-log.la libkea_hooks_la_LIBADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la diff --git a/src/lib/hooks/callout_handle.cc b/src/lib/hooks/callout_handle.cc index 8e83334f52..02b3429d82 100644 --- a/src/lib/hooks/callout_handle.cc +++ b/src/lib/hooks/callout_handle.cc @@ -158,5 +158,27 @@ CalloutHandle::getHookName() const { return (hook); } -} // namespace util +ScopedCalloutHandleState:: +ScopedCalloutHandleState(const CalloutHandlePtr& callout_handle) + : callout_handle_(callout_handle) { + if (!callout_handle_) { + isc_throw(BadValue, "callout_handle argument must not be null"); + } + + resetState(); +} + +ScopedCalloutHandleState::~ScopedCalloutHandleState() { + resetState(); +} + +void +ScopedCalloutHandleState::resetState() { + // No need to check if the handle is null because the constructor + // already checked that. + callout_handle_->deleteAllArguments(); + callout_handle_->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); +} + +} // namespace hooks } // namespace isc diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h index 206c3b6db7..abb5b06f78 100644 --- a/src/lib/hooks/callout_handle.h +++ b/src/lib/hooks/callout_handle.h @@ -415,6 +415,72 @@ private: /// A shared pointer to a CalloutHandle object. typedef boost::shared_ptr CalloutHandlePtr; +/// @brief Wrapper class around callout handle which automatically +/// resets handle's state. +/// +/// The Kea servers often require to associate processed packets with +/// @c CalloutHandle instances. This is to facilitate the case when the +/// hooks library passes information between the callouts using the +/// 'context' stored in the callout handle. The callouts invoked throughout +/// the packet lifetime have access to the context information for the +/// given packet. +/// +/// The association between the packets and the callout handles is +/// achieved by giving the ownership of the @c CalloutHandle objects to +/// the @c Pkt objects. When the @c Pkt object goes out of scope, it should +/// also release the pointer to the owned @c CalloutHandle object. +/// However, this causes a risk of circular dependency between the shared +/// pointer to the @c Pkt object and the shared pointer to the +/// @c CalloutHandle it owns, because the pointer to the packet is often +/// set as an argument of the callout handle prior to invoking a callout. +/// +/// In order to break the circular dependency, the arguments of the +/// callout handle must be deleted as soon as they are not needed +/// anymore. This class is a wrapper around the callout handle object, +/// which resets its state during construction and destruction. All +/// Kea hook points must use this class within the scope where the +/// @c HooksManager::callCallouts is invoked to reset the state of the +/// callout handle. The state is reset when this object goes out of +/// scope. +/// +/// Currently, the following operations are performed during the reset: +/// - all arguments of the callout handle are deleted, +/// - the next step status is set to @c CalloutHandle::NEXT_STEP CONTINUE +/// +/// This class must never be modified to also delete the context +/// information from the callout handle. The context is intended +/// to be used to share stateful data across callouts and hook points +/// and its contents must exist for the duration of the packet lifecycle. +/// Otherwise, we could simply re-create the callout handle for +/// each hook point and we wouldn't need this RAII class. +class ScopedCalloutHandleState { +public: + + /// @brief Constructor. + /// + /// Resets state of the callout handle. + /// + /// @param callout_handle reference to the pointer to the callout + /// handle which state should be reset. + /// @throw isc::BadValue if the callout handle is null. + explicit ScopedCalloutHandleState(const CalloutHandlePtr& callout_handle); + + /// @brief Destructor. + /// + /// Resets state of the callout handle. + ~ScopedCalloutHandleState(); + +private: + + /// @brief Resets the callout handle state. + /// + /// It is used internally by the constructor and destructor. + void resetState(); + + /// @brief Holds pointer to the wrapped callout handle. + CalloutHandlePtr callout_handle_; +}; + } // namespace hooks } // namespace isc -- 2.47.2