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

)

* 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.

* Fixed releasing the same context multiple times.

* Created BindingManagerContext to count all consumers using
context and release it in the proper moment.

* Added removing pending notification after connection failure.

* Moved allocating BindingManagerContext to NotifyBoundCLusterChange

* Removed removing pending notification on connect failure

* Addressed review comments

* Addressed second review comments

* Addressed review comments part 3

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,46 @@
 
 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; };
+    uint32_t GetConsumersNumber() { return mConsumersNumber; }
+    void IncrementConsumersNumber() { mConsumersNumber++; }
+    void DecrementConsumersNumber()
+    {
+        VerifyOrDie(mConsumersNumber > 0);
+        if (--mConsumersNumber == 0)
+        {
+            // Release the context only if there is no pending notification pointing to us.
+            if (mPendingNotificationContextReleaseHandler != nullptr)
+            {
+                mPendingNotificationContextReleaseHandler(mContext);
+            }
+            Platform::Delete(this);
+        }
+    }
+
+private:
+    void * mContext;
+    uint32_t mConsumersNumber = 0;
+    PendingNotificationContextReleaseHandler mPendingNotificationContextReleaseHandler;
+};
+
 struct PendingNotificationEntry
 {
 public:
     uint8_t mBindingEntryId;
-    void * mContext;
+    PendingNotificationContext * mContext;
 };
 
 // The pool for all the pending comands.
@@ -67,7 +102,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 +112,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;
 };