From b529b390dcffc2a19f915fec84f145ec64373954 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 24 Aug 2022 16:44:13 +0000 Subject: [PATCH 1/4] Couple with BindingManager Fixed: * Issue where BindingManager::EstablishConnection only called once * Prevented used after free * Prevented buffer overrun --- .../ameba/main/BindingHandler.cpp | 10 ++++++++-- .../ameba/main/BindingHandler.cpp | 10 ++++++++-- .../esp32/main/BindingHandler.cpp | 10 ++++++++-- .../telink/src/binding-handler.cpp | 10 ++++++++-- src/app/clusters/bindings/BindingManager.cpp | 5 +++-- .../clusters/bindings/PendingNotificationMap.cpp | 7 ++++++- .../clusters/bindings/PendingNotificationMap.h | 2 +- src/app/tests/TestPendingNotificationMap.cpp | 15 +++++++++------ 8 files changed, 51 insertions(+), 18 deletions(-) diff --git a/examples/all-clusters-app/ameba/main/BindingHandler.cpp b/examples/all-clusters-app/ameba/main/BindingHandler.cpp index ccfb210a50a80d..a19edd374d4a69 100644 --- a/examples/all-clusters-app/ameba/main/BindingHandler.cpp +++ b/examples/all-clusters-app/ameba/main/BindingHandler.cpp @@ -133,12 +133,20 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + + Platform::Delete(static_cast(context)); +} + void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } #ifdef CONFIG_ENABLE_CHIP_SHELL @@ -400,8 +408,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/ameba/main/BindingHandler.cpp b/examples/light-switch-app/ameba/main/BindingHandler.cpp index ccfb210a50a80d..a19edd374d4a69 100644 --- a/examples/light-switch-app/ameba/main/BindingHandler.cpp +++ b/examples/light-switch-app/ameba/main/BindingHandler.cpp @@ -133,12 +133,20 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + + Platform::Delete(static_cast(context)); +} + void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } #ifdef CONFIG_ENABLE_CHIP_SHELL @@ -400,8 +408,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/esp32/main/BindingHandler.cpp b/examples/light-switch-app/esp32/main/BindingHandler.cpp index 179c8e4a7c7a41..33f47a9bbeed2c 100644 --- a/examples/light-switch-app/esp32/main/BindingHandler.cpp +++ b/examples/light-switch-app/esp32/main/BindingHandler.cpp @@ -131,12 +131,20 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + + Platform::Delete(static_cast(context)); +} + void InitBindingHandlerInternal(intptr_t arg) { auto & server = chip::Server::GetInstance(); chip::BindingManager::GetInstance().Init( { &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() }); chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler); + chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler); } #ifdef CONFIG_ENABLE_CHIP_SHELL @@ -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/telink/src/binding-handler.cpp b/examples/light-switch-app/telink/src/binding-handler.cpp index 9483448c70b34b..8cf85c7e858884 100755 --- a/examples/light-switch-app/telink/src/binding-handler.cpp +++ b/examples/light-switch-app/telink/src/binding-handler.cpp @@ -133,6 +133,13 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation } } +void LightSwitchContextReleaseHandler(void * context) +{ + VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + + Platform::Delete(static_cast(context)); +} + #ifdef ENABLE_CHIP_SHELL /******************************************************** @@ -385,6 +392,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 @@ -413,8 +421,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/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index db07abe3ed9f34..8feb23e609fe2d 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -188,9 +188,10 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste { if (iter->type == EMBER_UNICAST_BINDING) { - mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); + error = mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext); + SuccessOrExit(error); error = EstablishConnection(ScopedNodeId(iter->nodeId, iter->fabricIndex)); - SuccessOrExit(error == CHIP_NO_ERROR); + SuccessOrExit(error); } else if (iter->type == EMBER_MULTICAST_BINDING) { diff --git a/src/app/clusters/bindings/PendingNotificationMap.cpp b/src/app/clusters/bindings/PendingNotificationMap.cpp index a7e6adb595ce59..f0b80b8dd6990b 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.cpp +++ b/src/app/clusters/bindings/PendingNotificationMap.cpp @@ -78,9 +78,13 @@ CHIP_ERROR PendingNotificationMap::FindLRUConnectPeer(ScopedNodeId & nodeId) return CHIP_ERROR_NOT_FOUND; } -void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context) +CHIP_ERROR PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context) { RemoveEntry(bindingEntryId); + if (mNumEntries == EMBER_BINDING_TABLE_SIZE) { + // We know that the RemoveEntry above did not do anything so we don't need to try restoring it. + return CHIP_ERROR_NO_MEMORY; + } mPendingBindingEntries[mNumEntries] = bindingEntryId; mPendingContexts[mNumEntries] = context; if (context) @@ -88,6 +92,7 @@ void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, Pend context->IncrementConsumersNumber(); } mNumEntries++; + return CHIP_NO_ERROR; } void PendingNotificationMap::RemoveEntry(uint8_t bindingEntryId) diff --git a/src/app/clusters/bindings/PendingNotificationMap.h b/src/app/clusters/bindings/PendingNotificationMap.h index 2e70a0718d047b..99dd3e8eb73cd3 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.h +++ b/src/app/clusters/bindings/PendingNotificationMap.h @@ -102,7 +102,7 @@ class PendingNotificationMap CHIP_ERROR FindLRUConnectPeer(ScopedNodeId & nodeId); - void AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context); + CHIP_ERROR AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context); void RemoveEntry(uint8_t bindingEntryId); diff --git a/src/app/tests/TestPendingNotificationMap.cpp b/src/app/tests/TestPendingNotificationMap.cpp index 271b427f31013a..7f5f091a253252 100644 --- a/src/app/tests/TestPendingNotificationMap.cpp +++ b/src/app/tests/TestPendingNotificationMap.cpp @@ -64,8 +64,11 @@ void TestAddRemove(nlTestSuite * aSuite, void * aContext) CreateDefaultFullBindingTable(BindingTable::GetInstance()); for (uint8_t i = 0; i < EMBER_BINDING_TABLE_SIZE; i++) { - pendingMap.AddPendingNotification(i, nullptr); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(i, nullptr) == CHIP_NO_ERROR); } + // Confirm adding in one more element fails + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(EMBER_BINDING_TABLE_SIZE, nullptr) == CHIP_ERROR_NO_MEMORY); + auto iter = pendingMap.begin(); for (uint8_t i = 0; i < EMBER_BINDING_TABLE_SIZE; i++) { @@ -102,11 +105,11 @@ void TestLRUEntry(nlTestSuite * aSuite, void * aContext) PendingNotificationMap pendingMap; ClearBindingTable(BindingTable::GetInstance()); CreateDefaultFullBindingTable(BindingTable::GetInstance()); - pendingMap.AddPendingNotification(0, nullptr); - pendingMap.AddPendingNotification(1, nullptr); - pendingMap.AddPendingNotification(5, nullptr); - pendingMap.AddPendingNotification(7, nullptr); - pendingMap.AddPendingNotification(11, nullptr); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(0, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(1, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(5, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(7, nullptr) == CHIP_NO_ERROR); + NL_TEST_ASSERT(aSuite, pendingMap.AddPendingNotification(11, nullptr) == CHIP_NO_ERROR); chip::ScopedNodeId node; From bc56a4ab6f7b12df4a7d79b4ee1a7d5c230e2543 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 24 Aug 2022 18:00:29 +0000 Subject: [PATCH 2/4] Restyle --- src/app/clusters/bindings/PendingNotificationMap.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/clusters/bindings/PendingNotificationMap.cpp b/src/app/clusters/bindings/PendingNotificationMap.cpp index f0b80b8dd6990b..7e279b2449c7fb 100644 --- a/src/app/clusters/bindings/PendingNotificationMap.cpp +++ b/src/app/clusters/bindings/PendingNotificationMap.cpp @@ -81,7 +81,8 @@ CHIP_ERROR PendingNotificationMap::FindLRUConnectPeer(ScopedNodeId & nodeId) CHIP_ERROR PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context) { RemoveEntry(bindingEntryId); - if (mNumEntries == EMBER_BINDING_TABLE_SIZE) { + if (mNumEntries == EMBER_BINDING_TABLE_SIZE) + { // We know that the RemoveEntry above did not do anything so we don't need to try restoring it. return CHIP_ERROR_NO_MEMORY; } From 780e73bf2b48f27b38457df81d4cea319c8aaf57 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 25 Aug 2022 14:32:30 +0000 Subject: [PATCH 3/4] Allocate callbacks in EstablishConnection This allows multiple connection async establishments to multiple nodes. --- .../ameba/main/BindingHandler.cpp | 2 +- .../ameba/main/BindingHandler.cpp | 2 +- .../esp32/main/BindingHandler.cpp | 2 +- .../telink/src/binding-handler.cpp | 2 +- src/app/clusters/bindings/BindingManager.cpp | 28 +++++------- src/app/clusters/bindings/BindingManager.h | 44 ++++++++++++++----- 6 files changed, 49 insertions(+), 31 deletions(-) diff --git a/examples/all-clusters-app/ameba/main/BindingHandler.cpp b/examples/all-clusters-app/ameba/main/BindingHandler.cpp index a19edd374d4a69..29acdaae952896 100644 --- a/examples/all-clusters-app/ameba/main/BindingHandler.cpp +++ b/examples/all-clusters-app/ameba/main/BindingHandler.cpp @@ -135,7 +135,7 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation void LightSwitchContextReleaseHandler(void * context) { - VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); Platform::Delete(static_cast(context)); } diff --git a/examples/light-switch-app/ameba/main/BindingHandler.cpp b/examples/light-switch-app/ameba/main/BindingHandler.cpp index a19edd374d4a69..29acdaae952896 100644 --- a/examples/light-switch-app/ameba/main/BindingHandler.cpp +++ b/examples/light-switch-app/ameba/main/BindingHandler.cpp @@ -135,7 +135,7 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation void LightSwitchContextReleaseHandler(void * context) { - VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); Platform::Delete(static_cast(context)); } diff --git a/examples/light-switch-app/esp32/main/BindingHandler.cpp b/examples/light-switch-app/esp32/main/BindingHandler.cpp index 33f47a9bbeed2c..5cc520fd2accd0 100644 --- a/examples/light-switch-app/esp32/main/BindingHandler.cpp +++ b/examples/light-switch-app/esp32/main/BindingHandler.cpp @@ -133,7 +133,7 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation void LightSwitchContextReleaseHandler(void * context) { - VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); Platform::Delete(static_cast(context)); } diff --git a/examples/light-switch-app/telink/src/binding-handler.cpp b/examples/light-switch-app/telink/src/binding-handler.cpp index 8cf85c7e858884..ed6b07f8df467e 100755 --- a/examples/light-switch-app/telink/src/binding-handler.cpp +++ b/examples/light-switch-app/telink/src/binding-handler.cpp @@ -135,7 +135,7 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, Operation void LightSwitchContextReleaseHandler(void * context) { - VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler");); + VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "Invalid context for Light switch context release handler")); Platform::Delete(static_cast(context)); } diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 8feb23e609fe2d..96a79b01711c55 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -101,12 +101,12 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) VerifyOrReturnError(mInitParams.mCASESessionManager != nullptr, CHIP_ERROR_INCORRECT_STATE); mLastSessionEstablishmentError = CHIP_NO_ERROR; - mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, &mOnConnectedCallback, &mOnConnectionFailureCallback); + auto * connectionCallback = Platform::New(this); + mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, connectionCallback->GetOnDeviceConnected(), + connectionCallback->GetOnDeviceConnectionFailure()); if (mLastSessionEstablishmentError == CHIP_ERROR_NO_MEMORY) { // Release the least recently used entry - // TODO: Some reference counting mechanism shall be added the CASESessionManager - // so that other session clients don't get accidentally closed. ScopedNodeId peerToRemove; if (mPendingNotificationMap.FindLRUConnectPeer(peerToRemove) == CHIP_NO_ERROR) { @@ -114,18 +114,15 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) // Now retry mLastSessionEstablishmentError = CHIP_NO_ERROR; - mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, &mOnConnectedCallback, &mOnConnectionFailureCallback); + // At this point connectionCallback is null since it deletes itself when the callback is called. + connectionCallback = Platform::New(this); + mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, connectionCallback->GetOnDeviceConnected(), + connectionCallback->GetOnDeviceConnectionFailure()); } } return mLastSessionEstablishmentError; } -void BindingManager::HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) -{ - BindingManager * manager = static_cast(context); - manager->HandleDeviceConnected(exchangeMgr, sessionHandle); -} - void BindingManager::HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { FabricIndex fabricToRemove = kUndefinedFabricIndex; @@ -149,17 +146,16 @@ void BindingManager::HandleDeviceConnected(Messaging::ExchangeManager & exchange mPendingNotificationMap.RemoveAllEntriesForNode(ScopedNodeId(nodeToRemove, fabricToRemove)); } -void BindingManager::HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) -{ - BindingManager * manager = static_cast(context); - manager->HandleDeviceConnectionFailure(peerId, error); -} - void BindingManager::HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error) { // 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())); mLastSessionEstablishmentError = error; + // We don't release the entry when connection fails, because inside + // BindingManager::EstablishConnection we may try again the connection. The logic in there + // doesn't actually make any sense with how mPendingNotificationMap and + // CASESessionManager are implemented today, but the risk of making changes close to 1.0 release + // prevents us from cleaning this up at the moment. } void BindingManager::FabricRemoved(FabricIndex fabricIndex) diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 578ad4af9e4b76..6234e72d5eb21a 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -72,9 +72,7 @@ struct BindingManagerInitParams class BindingManager { public: - BindingManager() : - mOnConnectedCallback(HandleDeviceConnected, this), mOnConnectionFailureCallback(HandleDeviceConnectionFailure, this) - {} + BindingManager() {} void RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler handler) { mBoundDeviceChangedHandler = handler; } @@ -123,13 +121,37 @@ class BindingManager static BindingManager & GetInstance() { return sBindingManager; } private: - static BindingManager sBindingManager; - - static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); - void HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); + class ConnectionCallback + { + public: + ConnectionCallback(BindingManager * bindingManager) : + mBindingManager(bindingManager), mOnConnectedCallback(HandleDeviceConnected, this), + mOnConnectionFailureCallback(HandleDeviceConnectionFailure, this) + {} + + Callback::Callback * GetOnDeviceConnected() { return &mOnConnectedCallback; } + Callback::Callback * GetOnDeviceConnectionFailure() { return &mOnConnectionFailureCallback; } + + private: + static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) + { + ConnectionCallback * connectionCallback = static_cast(context); + connectionCallback->mBindingManager->HandleDeviceConnected(exchangeMgr, sessionHandle); + Platform::Delete(connectionCallback); + } + static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) + { + ConnectionCallback * connectionCallback = static_cast(context); + connectionCallback->mBindingManager->HandleDeviceConnectionFailure(peerId, error); + Platform::Delete(connectionCallback); + } + + BindingManager * mBindingManager; + Callback::Callback mOnConnectedCallback; + Callback::Callback mOnConnectionFailureCallback; + }; - static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error); - void HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error); + static BindingManager sBindingManager; CHIP_ERROR EstablishConnection(const ScopedNodeId & nodeId); @@ -137,8 +159,8 @@ class BindingManager BoundDeviceChangedHandler mBoundDeviceChangedHandler; BindingManagerInitParams mInitParams; - Callback::Callback mOnConnectedCallback; - Callback::Callback mOnConnectionFailureCallback; + void HandleDeviceConnected(Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle); + void HandleDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error); // Used to keep track of synchronous failures from FindOrEstablishSession. CHIP_ERROR mLastSessionEstablishmentError; From d1c937d9c689dc568a4039e8eb6c16a4c61d6174 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 25 Aug 2022 21:56:38 +0000 Subject: [PATCH 4/4] Address PR comments --- src/app/clusters/bindings/BindingManager.cpp | 11 ++++----- src/app/clusters/bindings/BindingManager.h | 26 ++++++++++++++------ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 96a79b01711c55..ad13376a0dfe42 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -101,7 +101,7 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) VerifyOrReturnError(mInitParams.mCASESessionManager != nullptr, CHIP_ERROR_INCORRECT_STATE); mLastSessionEstablishmentError = CHIP_NO_ERROR; - auto * connectionCallback = Platform::New(this); + auto * connectionCallback = Platform::New(*this); mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, connectionCallback->GetOnDeviceConnected(), connectionCallback->GetOnDeviceConnectionFailure()); if (mLastSessionEstablishmentError == CHIP_ERROR_NO_MEMORY) @@ -115,7 +115,7 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId) // Now retry mLastSessionEstablishmentError = CHIP_NO_ERROR; // At this point connectionCallback is null since it deletes itself when the callback is called. - connectionCallback = Platform::New(this); + connectionCallback = Platform::New(*this); mInitParams.mCASESessionManager->FindOrEstablishSession(nodeId, connectionCallback->GetOnDeviceConnected(), connectionCallback->GetOnDeviceConnectionFailure()); } @@ -152,10 +152,9 @@ void BindingManager::HandleDeviceConnectionFailure(const ScopedNodeId & peerId, ChipLogError(AppServer, "Failed to establish connection to node 0x" ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); mLastSessionEstablishmentError = error; // We don't release the entry when connection fails, because inside - // BindingManager::EstablishConnection we may try again the connection. The logic in there - // doesn't actually make any sense with how mPendingNotificationMap and - // CASESessionManager are implemented today, but the risk of making changes close to 1.0 release - // prevents us from cleaning this up at the moment. + // BindingManager::EstablishConnection we may try again the connection. + // TODO(#22173): The logic in there doesn't actually make any sense with how + // mPendingNotificationMap and CASESessionManager are implemented today. } void BindingManager::FabricRemoved(FabricIndex fabricIndex) diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 6234e72d5eb21a..646281f6718e4f 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -121,10 +121,20 @@ class BindingManager static BindingManager & GetInstance() { return sBindingManager; } private: + /* + * Used when providing OnConnection/Failure callbacks to CASESessionManager when establishing session. + * + * Since the BindingManager calls EstablishConnection inside of a loop, and it is possible that the + * callback is called some time after the loop is completed, we need a separate callbacks for each + * connection we are trying to establish. Failure to provide different instances of the callback + * to CASESessionManager may result in the callback only be called for that last EstablishConnection + * that was called when it establishes the connections asynchronously. + * + */ class ConnectionCallback { public: - ConnectionCallback(BindingManager * bindingManager) : + ConnectionCallback(BindingManager & bindingManager) : mBindingManager(bindingManager), mOnConnectedCallback(HandleDeviceConnected, this), mOnConnectionFailureCallback(HandleDeviceConnectionFailure, this) {} @@ -135,18 +145,18 @@ class BindingManager private: static void HandleDeviceConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle) { - ConnectionCallback * connectionCallback = static_cast(context); - connectionCallback->mBindingManager->HandleDeviceConnected(exchangeMgr, sessionHandle); - Platform::Delete(connectionCallback); + ConnectionCallback * _this = static_cast(context); + _this->mBindingManager.HandleDeviceConnected(exchangeMgr, sessionHandle); + Platform::Delete(_this); } static void HandleDeviceConnectionFailure(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) { - ConnectionCallback * connectionCallback = static_cast(context); - connectionCallback->mBindingManager->HandleDeviceConnectionFailure(peerId, error); - Platform::Delete(connectionCallback); + ConnectionCallback * _this = static_cast(context); + _this->mBindingManager.HandleDeviceConnectionFailure(peerId, error); + Platform::Delete(_this); } - BindingManager * mBindingManager; + BindingManager & mBindingManager; Callback::Callback mOnConnectedCallback; Callback::Callback mOnConnectionFailureCallback; };