From bd062f8dbcaa1b3e043902db48879db0f7bd05f8 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Thu, 14 Apr 2022 12:53:18 +0200 Subject: [PATCH] Fixed BindingManager to make sure it doesn't use invalid context BindingManager uses application context passed to NotifyBoundClusterChanged and doesn't care about its life time. Applications delete the context after calling the method, so if the context will be used asynchronously (like for HandleDeviceConnect) it may have invalid value. * Added for BindingManager method that allows to register handler called when context is not used anymore and can be released. * Added releasing context in applications using context release handler. --- .../all-clusters-common/src/binding-handler.cpp | 6 ++++++ .../efr32/src/binding-handler.cpp | 10 ++++++++-- .../nrfconnect/main/BindingHandler.cpp | 16 +++++++++++++--- .../nrfconnect/main/LightSwitch.cpp | 2 -- .../nrfconnect/main/include/BindingHandler.h | 1 + src/app/clusters/bindings/BindingManager.cpp | 16 ++++++++++++++++ src/app/clusters/bindings/BindingManager.h | 13 +++++++++++++ 7 files changed, 57 insertions(+), 7 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp index 03c96e8c66be09..476ce1b75ceee8 100644 --- a/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp +++ b/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp @@ -103,12 +103,18 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch } } +static void BoundDeviceContextReleaseHandler(void * context) +{ + (void) context; +} + static void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler); } CHIP_ERROR InitBindingHandlers() diff --git a/examples/light-switch-app/efr32/src/binding-handler.cpp b/examples/light-switch-app/efr32/src/binding-handler.cpp index 6ce872e811ae47..4f7d3334c0653a 100644 --- a/examples/light-switch-app/efr32/src/binding-handler.cpp +++ b/examples/light-switch-app/efr32/src/binding-handler.cpp @@ -132,6 +132,13 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null")); + BindingCommandData * data = static_cast(context); + Platform::Delete(data); +} + #ifdef ENABLE_CHIP_SHELL /******************************************************** @@ -384,6 +391,7 @@ void InitBindingHandlerInternal(intptr_t arg) chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } } // namespace @@ -398,8 +406,6 @@ void SwitchWorkerFunction(intptr_t context) BindingCommandData * data = reinterpret_cast(context); BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast(data)); - - Platform::Delete(data); } void BindingWorkerFunction(intptr_t context) diff --git a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp index 54cce9924887fd..b1292f06e58c11 100644 --- a/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp +++ b/examples/light-switch-app/nrfconnect/main/BindingHandler.cpp @@ -57,9 +57,13 @@ void BindingHandler::OnInvokeCommandFailure(DeviceProxy * aDevice, BindingData & // Set flag to not try recover session multiple times. BindingHandler::GetInstance().mCaseSessionRecovered = true; + // Allocate new object to make sure its life time will be appropriate. + BindingHandler::BindingData * data = Platform::New(); + *data = aBindingData; + // Establish new CASE session and retrasmit command that was not applied. error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, - static_cast(&aBindingData)); + static_cast(data)); } else { @@ -222,6 +226,13 @@ void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & bi } } +void BindingHandler::LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + BindingData * data = static_cast(context); + Platform::Delete(data); +} + void BindingHandler::InitInternal(intptr_t aArg) { LOG_INF("Initialize binding Handler"); @@ -234,6 +245,7 @@ void BindingHandler::InitInternal(intptr_t aArg) } BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); BindingHandler::GetInstance().PrintBindingTable(); } @@ -298,6 +310,4 @@ void BindingHandler::SwitchWorkerHandler(intptr_t aContext) BindingData * data = reinterpret_cast(aContext); LOG_INF("Notify Bounded Cluster | endpoint: %d cluster: %d", data->EndpointId, data->ClusterId); BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, static_cast(data)); - - Platform::Delete(data); } diff --git a/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp b/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp index 7420d8f5254feb..2b54264eb45b14 100644 --- a/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp +++ b/examples/light-switch-app/nrfconnect/main/LightSwitch.cpp @@ -57,7 +57,6 @@ void LightSwitch::InitiateActionSwitch(Action mAction) } data->IsGroup = BindingHandler::GetInstance().IsGroupBound(); DeviceLayer::PlatformMgr().ScheduleWork(BindingHandler::SwitchWorkerHandler, reinterpret_cast(data)); - Platform::Delete(data); } } @@ -79,6 +78,5 @@ void LightSwitch::DimmerChangeBrightness() data->Value = (uint8_t) sBrightness; data->IsGroup = BindingHandler::GetInstance().IsGroupBound(); DeviceLayer::PlatformMgr().ScheduleWork(BindingHandler::SwitchWorkerHandler, reinterpret_cast(data)); - Platform::Delete(data); } } diff --git a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h index fcd2d4fa8288b3..c3820e1ec33815 100644 --- a/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h +++ b/examples/light-switch-app/nrfconnect/main/include/BindingHandler.h @@ -54,6 +54,7 @@ class BindingHandler static void OnOffProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LevelControlProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *); static void LightSwitchChangedHandler(const EmberBindingTableEntry &, chip::DeviceProxy *, void *); + static void LightSwitchContextReleaseHandler(void * context); static void InitInternal(intptr_t); bool mCaseSessionRecovered = false; diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index a87fe777c7533b..7a51b52944e257 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -161,6 +161,7 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device) fabricToRemove = entry.fabricIndex; nodeToRemove = entry.nodeId; mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext); + mBoundDeviceContextReleaseHandler(pendingNotification.mContext); } } mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove); @@ -176,6 +177,19 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err { // Simply release the entry, the connection will be re-established as needed. ChipLogError(AppServer, "Failed to establish connection to node 0x" ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); + + for (PendingNotificationEntry pendingNotification : mPendingNotificationMap) + { + EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId); + + PeerId peer = PeerIdForNode(mInitParams.mFabricTable, entry.fabricIndex, entry.nodeId); + + if (peerId == peer) + { + mBoundDeviceContextReleaseHandler(pendingNotification.mContext); + } + } + mInitParams.mCASESessionManager->ReleaseSession(peerId); } @@ -204,6 +218,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { // We already have an active connection mBoundDeviceChangedHandler(*iter, peerDevice, context); + mBoundDeviceContextReleaseHandler(context); } else { @@ -214,6 +229,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste else if (iter->type == EMBER_MULTICAST_BINDING) { mBoundDeviceChangedHandler(*iter, nullptr, context); + mBoundDeviceContextReleaseHandler(context); } } } diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 88bedac4df357b..d43d5d07a7f8a7 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -40,6 +40,8 @@ namespace chip { */ using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context); +using BoundDeviceContextReleaseHandler = void (*)(void * context); + struct BindingManagerInitParams { FabricTable * mFabricTable = nullptr; @@ -70,6 +72,16 @@ class BindingManager void RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler handler) { mBoundDeviceChangedHandler = handler; } + /* + * Registers handler that will be called when context used in NotifyBoundClusterChanged will not be needed and could be + * released. + * + */ + void RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler handler) + { + mBoundDeviceContextReleaseHandler = handler; + } + CHIP_ERROR Init(const BindingManagerInitParams & params); /* @@ -117,6 +129,7 @@ class BindingManager PendingNotificationMap mPendingNotificationMap; BoundDeviceChangedHandler mBoundDeviceChangedHandler; + BoundDeviceContextReleaseHandler mBoundDeviceContextReleaseHandler; BindingManagerInitParams mInitParams; Callback::Callback mOnConnectedCallback;