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(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()

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, "LightSwitchContextReleaseHandler: context is null"));
+
+    Platform::Delete(static_cast<BindingCommandData *>(context));
+}
+
 #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<BindingCommandData *>(context);
     BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast<void *>(data));
-
-    Platform::Delete(data);
 }
 
 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;
+
         // Establish new CASE session and retrasmit command that was not applied.
         error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId,
-                                                                        static_cast<void *>(&aBindingData));
+                                                                        static_cast<void *>(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"););
+
+    Platform::Delete(static_cast<BindingData *>(context));
+}
+
 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<BindingData *>(aContext);
     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);
 }

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(void * 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,7 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device)
         {
             fabricToRemove = entry.fabricIndex;
             nodeToRemove   = entry.nodeId;
-            mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext);
+            mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext->GetContext());
         }
     }
     mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove);
@@ -190,6 +191,12 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste
     VerifyOrReturnError(mInitParams.mFabricTable != nullptr, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(mBoundDeviceChangedHandler, CHIP_NO_ERROR);
 
+    CHIP_ERROR error      = CHIP_NO_ERROR;
+    auto * bindingContext = mPendingNotificationMap.NewPendingNotificationContext(context);
+    VerifyOrReturnError(bindingContext != nullptr, CHIP_ERROR_NO_MEMORY);
+
+    bindingContext->IncrementConsumersNumber();
+
     for (auto iter = BindingTable::GetInstance().begin(); iter != BindingTable::GetInstance().end(); ++iter)
     {
         if (iter->local == endpoint && (!iter->clusterId.HasValue() || iter->clusterId.Value() == cluster))
@@ -203,21 +210,26 @@ 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, bindingContext->GetContext());
                 }
                 else
                 {
-                    mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), context);
-                    ReturnErrorOnFailure(EstablishConnection(iter->fabricIndex, iter->nodeId));
+                    mPendingNotificationMap.AddPendingNotification(iter.GetIndex(), bindingContext);
+                    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, bindingContext->GetContext());
             }
         }
     }
-    return CHIP_NO_ERROR;
+
+exit:
+    bindingContext->DecrementConsumersNumber();
+
+    return error;
 }
 
 } // namespace chip

src/app/clusters/bindings/BindingManager.h

@@ -40,6 +40,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 = PendingNotificationContextReleaseHandler;
+
 struct BindingManagerInitParams
 {
     FabricTable * mFabricTable               = nullptr;
@@ -70,6 +76,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)
+    {
+        mPendingNotificationMap.RegisterPendingNotificationContextReleaseHandler(handler);
+    }
+
     CHIP_ERROR Init(const BindingManagerInitParams & params);
 
     /*

src/app/clusters/bindings/PendingNotificationMap.cpp

@@ -79,11 +79,15 @@ CHIP_ERROR PendingNotificationMap::FindLRUConnectPeer(FabricIndex * fabric, Node
     return CHIP_ERROR_NOT_FOUND;
 }
 
-void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, void * context)
+void PendingNotificationMap::AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context)
 {
     RemoveEntry(bindingEntryId);
     mPendingBindingEntries[mNumEntries] = bindingEntryId;
     mPendingContexts[mNumEntries]       = context;
+    if (context)
+    {
+        context->IncrementConsumersNumber();
+    }
     mNumEntries++;
 }
 
@@ -98,6 +102,10 @@ void PendingNotificationMap::RemoveEntry(uint8_t bindingEntryId)
             mPendingContexts[newEntryCount]       = mPendingContexts[i];
             newEntryCount++;
         }
+        else if (mPendingContexts[i] != nullptr)
+        {
+            mPendingContexts[i]->DecrementConsumersNumber();
+        }
     }
     mNumEntries = newEntryCount;
 }
@@ -114,6 +122,10 @@ void PendingNotificationMap::RemoveAllEntriesForNode(FabricIndex fabric, NodeId
             mPendingContexts[newEntryCount]       = mPendingContexts[i];
             newEntryCount++;
         }
+        else if (mPendingContexts[i] != nullptr)
+        {
+            mPendingContexts[i]->DecrementConsumersNumber();
+        }
     }
     mNumEntries = newEntryCount;
 }
@@ -130,6 +142,10 @@ void PendingNotificationMap::RemoveAllEntriesForFabric(FabricIndex fabric)
             mPendingContexts[newEntryCount]       = mPendingContexts[i];
             newEntryCount++;
         }
+        else if (mPendingContexts[i] != nullptr)
+        {
+            mPendingContexts[i]->DecrementConsumersNumber();
+        }
     }
     mNumEntries = newEntryCount;
 }

src/app/clusters/bindings/PendingNotificationMap.h

@@ -21,11 +21,49 @@
 
 namespace chip {
 
+/**
+ * Application callback function when a context used in PendingNotificationEntry will not be needed and should be
+ * released.
+ */
+using PendingNotificationContextReleaseHandler = void (*)(void * context);
+
+class PendingNotificationContext
+{
+public:
+    PendingNotificationContext(void * context, PendingNotificationContextReleaseHandler contextReleaseHandler) :
+        mContext(context), mPendingNotificationContextReleaseHandler(contextReleaseHandler)
+    {}
+    void * GetContext() { return mContext; };
+    uint8_t GetConsumersNumber() { return mConsumersNumber; }
+    void IncrementConsumersNumber() { mConsumersNumber++; }
+    void DecrementConsumersNumber()
+    {
+        if (mConsumersNumber > 0)
+        {
+            mConsumersNumber--;
+        }
+        else
+        {
+            // Release the context only if there is no pending notification pointing to it context.
+            if (mPendingNotificationContextReleaseHandler != nullptr)
+            {
+                mPendingNotificationContextReleaseHandler(mContext);
+            }
+            Platform::Delete(this);
+        }
+    }
+
+private:
+    void * mContext;
+    uint8_t mConsumersNumber = 0;
+    PendingNotificationContextReleaseHandler mPendingNotificationContextReleaseHandler;
+};
+
 struct PendingNotificationEntry
 {
 public:
     uint8_t mBindingEntryId;
-    void * mContext;
+    PendingNotificationContext * mContext;
 };
 
 // The pool for all the pending comands.
@@ -67,7 +105,7 @@ class PendingNotificationMap
 
     CHIP_ERROR FindLRUConnectPeer(FabricIndex * fabric, NodeId * node);
 
-    void AddPendingNotification(uint8_t bindingEntryId, void * context);
+    void AddPendingNotification(uint8_t bindingEntryId, PendingNotificationContext * context);
 
     void RemoveEntry(uint8_t bindingEntryId);
 
@@ -77,9 +115,20 @@ class PendingNotificationMap
 
     void RemoveAllEntriesForFabric(FabricIndex fabric);
 
+    void RegisterPendingNotificationContextReleaseHandler(PendingNotificationContextReleaseHandler handler)
+    {
+        mPendingNotificationContextReleaseHandler = handler;
+    }
+
+    PendingNotificationContext * NewPendingNotificationContext(void * context)
+    {
+        return Platform::New<PendingNotificationContext>(context, mPendingNotificationContextReleaseHandler);
+    };
+
 private:
     uint8_t mPendingBindingEntries[kMaxPendingNotifications];
-    void * mPendingContexts[kMaxPendingNotifications];
+    PendingNotificationContext * mPendingContexts[kMaxPendingNotifications];
+    PendingNotificationContextReleaseHandler mPendingNotificationContextReleaseHandler;
 
     uint8_t mNumEntries = 0;
 };