Fixed BindingManager to make sure it doesn't use invalid context

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(chip::BindingManagerContext * 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()

examples/light-switch-app/efr32/src/binding-handler.cpp

@@ -132,6 +132,15 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro
     }
 }
 
+void LightSwitchContextReleaseHandler(BindingManagerContext * context)
+{
+    VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "LightSwitchContextReleaseHandler: context is null"));
+
+    // Release the internal context stored in BindingManagerContext and then the BindingManagerContext itself.
+    Platform::Delete(static_cast<BindingCommandData *>(context->GetContext()));
+    Platform::Delete(context);
+}
+
 #ifdef ENABLE_CHIP_SHELL
 
 /********************************************************
@@ -384,6 +393,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
@@ -396,10 +406,9 @@ void SwitchWorkerFunction(intptr_t context)
 {
     VerifyOrReturn(context != 0, ChipLogError(NotSpecified, "SwitchWorkerFunction - Invalid work data"));
 
-    BindingCommandData * data = reinterpret_cast<BindingCommandData *>(context);
-    BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast<void *>(data));
-
-    Platform::Delete(data);
+    BindingCommandData * data              = reinterpret_cast<BindingCommandData *>(context);
+    BindingManagerContext * bindingContext = Platform::New<BindingManagerContext>(static_cast<void *>(data));
+    BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, bindingContext);
 }
 
 void BindingWorkerFunction(intptr_t context)

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<BindingHandler::BindingData>();
+        *data                              = aBindingData;
+        BindingManagerContext * context    = Platform::New<BindingManagerContext>(static_cast<void *>(data));
+
         // Establish new CASE session and retrasmit command that was not applied.
-        error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId,
-                                                                        static_cast<void *>(&aBindingData));
+        error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId, context);
     }
     else
     {
@@ -222,6 +226,15 @@ void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & bi
     }
 }
 
+void BindingHandler::LightSwitchContextReleaseHandler(BindingManagerContext * context)
+{
+    VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler"););
+
+    // Release the internal context stored in BindingManagerContext and then the BindingManagerContext itself.
+    Platform::Delete(static_cast<BindingData *>(context->GetContext()));
+    Platform::Delete(context);
+}
+
 void BindingHandler::InitInternal(intptr_t aArg)
 {
     LOG_INF("Initialize binding Handler");
@@ -234,6 +247,7 @@ void BindingHandler::InitInternal(intptr_t aArg)
     }
 
     BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler);
+    BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler);
     BindingHandler::GetInstance().PrintBindingTable();
 }
 
@@ -295,9 +309,8 @@ void BindingHandler::SwitchWorkerHandler(intptr_t aContext)
 {
     VerifyOrReturn(aContext != 0, LOG_ERR("Invalid Swich data"));
 
-    BindingData * data = reinterpret_cast<BindingData *>(aContext);
+    BindingData * data              = reinterpret_cast<BindingData *>(aContext);
+    BindingManagerContext * context = Platform::New<BindingManagerContext>(static_cast<void *>(data));
     LOG_INF("Notify Bounded Cluster | endpoint: %d cluster: %d", data->EndpointId, data->ClusterId);
-    BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, static_cast<void *>(data));
-
-    Platform::Delete(data);
+    BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, context);
 }

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<intptr_t>(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<intptr_t>(data));
-        Platform::Delete(data);
     }
 }

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(chip::BindingManagerContext * context);
     static void InitInternal(intptr_t);
 
     bool mCaseSessionRecovered = false;

src/app/clusters/bindings/BindingManager.cpp

@@ -19,6 +19,7 @@
 #include <app/util/binding-table.h>
 #include <credentials/FabricTable.h>
 #include <lib/support/CHIPMem.h>
+#include <lib/support/CodeUtils.h>
 
 namespace {
 
@@ -160,7 +161,15 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device)
         {
             fabricToRemove = entry.fabricIndex;
             nodeToRemove   = entry.nodeId;
-            mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext);
+
+            BindingManagerContext * context = static_cast<BindingManagerContext *>(pendingNotification.mContext);
+            mBoundDeviceChangedHandler(entry, device, context->GetContext());
+
+            context->DecrementConsumersNumber();
+            if (context->GetConsumersNumber() == 0)
+            {
+                mBoundDeviceContextReleaseHandler(context);
+            }
         }
     }
     mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove);
@@ -176,6 +185,32 @@ 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()));
+
+    FabricIndex fabricToRemove = kUndefinedFabricIndex;
+    NodeId nodeToRemove        = kUndefinedNodeId;
+
+    for (PendingNotificationEntry pendingNotification : mPendingNotificationMap)
+    {
+        EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId);
+
+        PeerId peer = PeerIdForNode(mInitParams.mFabricTable, entry.fabricIndex, entry.nodeId);
+
+        if (peerId == peer)
+        {
+            fabricToRemove = entry.fabricIndex;
+            nodeToRemove   = entry.nodeId;
+
+            BindingManagerContext * context = static_cast<BindingManagerContext *>(pendingNotification.mContext);
+
+            context->DecrementConsumersNumber();
+            if (context->GetConsumersNumber() == 0)
+            {
+                mBoundDeviceContextReleaseHandler(context);
+            }
+        }
+    }
+    mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove);
+
     mInitParams.mCASESessionManager->ReleaseSession(peerId);
 }
 
@@ -185,10 +220,15 @@ void BindingManager::FabricRemoved(CompressedFabricId compressedFabricId, Fabric
     mInitParams.mCASESessionManager->ReleaseSessionsForFabric(compressedFabricId);
 }
 
-CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context)
+CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, BindingManagerContext * context)
 {
     VerifyOrReturnError(mInitParams.mFabricTable != nullptr, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(mBoundDeviceChangedHandler, CHIP_NO_ERROR);
+    VerifyOrReturnError(mBoundDeviceContextReleaseHandler, CHIP_NO_ERROR);
+
+    CHIP_ERROR error = CHIP_NO_ERROR;
+
+    context->IncrementConsumersNumber();
 
     for (auto iter = BindingTable::GetInstance().begin(); iter != BindingTable::GetInstance().end(); ++iter)
     {
@@ -203,21 +243,33 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste
                 if (peerDevice != nullptr && peerDevice->IsConnected())
                 {
                     // We already have an active connection
-                    mBoundDeviceChangedHandler(*iter, peerDevice, context);
+                    mBoundDeviceChangedHandler(*iter, peerDevice, context->GetContext());
                 }
                 else
                 {
-                    mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), context);
-                    ReturnErrorOnFailure(EstablishConnection(iter->fabricIndex, iter->nodeId));
+                    mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), static_cast<void *>(context));
+                    context->IncrementConsumersNumber();
+                    error = EstablishConnection(iter->fabricIndex, iter->nodeId);
+                    SuccessOrExit(error == CHIP_NO_ERROR);
                 }
             }
             else if (iter->type == EMBER_MULTICAST_BINDING)
             {
-                mBoundDeviceChangedHandler(*iter, nullptr, context);
+                mBoundDeviceChangedHandler(*iter, nullptr, context->GetContext());
             }
         }
     }
-    return CHIP_NO_ERROR;
+
+exit:
+    context->DecrementConsumersNumber();
+
+    // Call release context handler only if any pending notification is not going to use it.
+    if (context->GetConsumersNumber() == 0)
+    {
+        mBoundDeviceContextReleaseHandler(context);
+    }
+
+    return error;
 }
 
 } // namespace chip

src/app/clusters/bindings/BindingManager.h

@@ -26,6 +26,26 @@
 
 namespace chip {
 
+class BindingManagerContext
+{
+public:
+    BindingManagerContext(void * context) : mContext(context) {}
+    void * GetContext() { return mContext; };
+    uint8_t GetConsumersNumber() { return mConsumersNumber; }
+    void IncrementConsumersNumber() { mConsumersNumber++; }
+    void DecrementConsumersNumber()
+    {
+        if (mConsumersNumber > 0)
+        {
+            mConsumersNumber--;
+        }
+    }
+
+private:
+    void * mContext;
+    uint8_t mConsumersNumber = 0;
+};
+
 /**
  * Application callback function when a cluster associated with a binding changes.
  *
@@ -40,6 +60,12 @@ namespace chip {
  */
 using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context);
 
+/**
+ * Application callback function when a context used in NotifyBoundClusterChanged will not be needed and should be
+ * released.
+ */
+using BoundDeviceContextReleaseHandler = void (*)(BindingManagerContext * context);
+
 struct BindingManagerInitParams
 {
     FabricTable * mFabricTable               = nullptr;
@@ -70,6 +96,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);
 
     /*
@@ -100,7 +136,7 @@ class BindingManager
      * be initiated. The BoundDeviceChangedHandler will be called once the session is established.
      *
      */
-    CHIP_ERROR NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context);
+    CHIP_ERROR NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, BindingManagerContext * context);
 
     static BindingManager & GetInstance() { return sBindingManager; }
 
@@ -117,6 +153,7 @@ class BindingManager
 
     PendingNotificationMap mPendingNotificationMap;
     BoundDeviceChangedHandler mBoundDeviceChangedHandler;
+    BoundDeviceContextReleaseHandler mBoundDeviceContextReleaseHandler;
     BindingManagerInitParams mInitParams;
 
     Callback::Callback<OnDeviceConnected> mOnConnectedCallback;