No longer provide OperationDeviceProxy in OnDeviceConnected callback (#…

…21256) Previous implementations of OnDeviceConnected held onto OperationalDeviceProxy when they really should not have could lead to use after free should something else free that OperationalDeviceProxy.
project-chip · Aug 3, 2023 · c34d922 · c34d922
1 parent 6985a23
commit c34d922
Show file tree

Hide file tree

Showing 72 changed files with 653 additions and 471 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
@@ -67,7 +67,8 @@ static void RegisterSwitchCommands()
 }
 #endif // defined(ENABLE_CHIP_SHELL)
 
-static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::DeviceProxy * peer_device, void * context)
+static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::OperationalDeviceProxy * peer_device,
+                                      void * context)
 {
     using namespace chip;
     using namespace chip::app;
@@ -88,6 +89,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch
             ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format());
         };
 
+        VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
         if (sSwitchOnOffState)
         {
             Clusters::OnOff::Commands::On::Type onCommand;

diff --git a/examples/all-clusters-app/ameba/main/BindingHandler.cpp b/examples/all-clusters-app/ameba/main/BindingHandler.cpp
@@ -50,7 +50,8 @@ Engine sShellSwitchBindingSubCommands;
 
 namespace {
 
-void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device)
+void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding,
+                                       OperationalDeviceProxy * peer_device)
 {
     auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
         ChipLogProgress(NotSpecified, "OnOff command succeeds");
@@ -60,6 +61,7 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
         ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format());
     };
 
+    VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
     switch (commandId)
     {
     case Clusters::OnOff::Commands::Toggle::Id:
@@ -106,7 +108,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl
     }
 }
 
-void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context)
+void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context)
 {
     VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
     BindingCommandData * data = static_cast<BindingCommandData *>(context);

diff --git a/examples/all-clusters-app/esp32/main/include/ShellCommands.h b/examples/all-clusters-app/esp32/main/include/ShellCommands.h
@@ -134,7 +134,7 @@ class CASECommands
 
 private:
     CASECommands() {}
-    static void OnConnected(void * context, OperationalDeviceProxy * deviceProxy)
+    static void OnConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle)
     {
         streamer_printf(streamer_get(), "Establish CASESession Success!\r\n");
         GetInstance().SetOnConnecting(false);

diff --git a/examples/all-clusters-app/nxp/mw320/binding-handler.cpp b/examples/all-clusters-app/nxp/mw320/binding-handler.cpp
@@ -68,7 +68,8 @@ static void RegisterSwitchCommands()
 }
 #endif // defined(ENABLE_CHIP_SHELL)
 
-static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::DeviceProxy * peer_device, void * context)
+static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, chip::OperationalDeviceProxy * peer_device,
+                                      void * context)
 {
     using namespace chip;
     using namespace chip::app;
@@ -92,6 +93,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch
         // command (SwitchCommandHandler)
         {
             Clusters::OnOff::Commands::Toggle::Type toggleCommand;
+            VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
             Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(),
                                              binding.remote, toggleCommand, onSuccess, onFailure);
         }

diff --git a/examples/all-clusters-minimal-app/esp32/main/include/ShellCommands.h b/examples/all-clusters-minimal-app/esp32/main/include/ShellCommands.h
@@ -134,7 +134,7 @@ class CASECommands
 
 private:
     CASECommands() {}
-    static void OnConnected(void * context, OperationalDeviceProxy * deviceProxy)
+    static void OnConnected(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle)
     {
         streamer_printf(streamer_get(), "Establish CASESession Success!\r\n");
         GetInstance().SetOnConnecting(false);

diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp
@@ -46,12 +46,14 @@ CHIP_ERROR ModelCommand::RunCommand()
                                                     &mOnDeviceConnectionFailureCallback);
 }
 
-void ModelCommand::OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device)
+void ModelCommand::OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr,
+                                       chip::SessionHandle & sessionHandle)
 {
     ModelCommand * command = reinterpret_cast<ModelCommand *>(context);
     VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectedFn: context is null"));
 
-    CHIP_ERROR err = command->SendCommand(device, command->mEndPointId);
+    chip::OperationalDeviceProxy device(&exchangeMgr, sessionHandle);
+    CHIP_ERROR err = command->SendCommand(&device, command->mEndPointId);
     VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err));
 }
 

diff --git a/examples/chip-tool/commands/clusters/ModelCommand.h b/examples/chip-tool/commands/clusters/ModelCommand.h
@@ -69,7 +69,8 @@ class ModelCommand : public CHIPCommand
     chip::NodeId mDestinationId;
     std::vector<chip::EndpointId> mEndPointId;
 
-    static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device);
+    static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr,
+                                    chip::SessionHandle & sessionHandle);
     static void OnDeviceConnectionFailureFn(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error);
 
     chip::Callback::Callback<chip::OnDeviceConnected> mOnDeviceConnectedCallback;

diff --git a/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp b/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp
@@ -28,17 +28,16 @@ CHIP_ERROR CloseSessionCommand::RunCommand()
     CommissioneeDeviceProxy * commissioneeDeviceProxy = nullptr;
     if (CHIP_NO_ERROR == CurrentCommissioner().GetDeviceBeingCommissioned(mDestinationId, &commissioneeDeviceProxy))
     {
-        return CloseSession(commissioneeDeviceProxy);
+        VerifyOrReturnError(commissioneeDeviceProxy->GetSecureSession().HasValue(), CHIP_ERROR_INCORRECT_STATE);
+        return CloseSession(*commissioneeDeviceProxy->GetExchangeManager(), commissioneeDeviceProxy->GetSecureSession().Value());
     }
 
     return CurrentCommissioner().GetConnectedDevice(mDestinationId, &mOnDeviceConnectedCallback,
                                                     &mOnDeviceConnectionFailureCallback);
 }
 
-CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device)
+CHIP_ERROR CloseSessionCommand::CloseSession(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle)
 {
-    VerifyOrReturnError(device->GetSecureSession().HasValue(), CHIP_ERROR_INCORRECT_STATE);
-
     // TODO perhaps factor out this code into something on StatusReport that
     // takes an exchange and maybe a SendMessageFlags?
     SecureChannel::StatusReport statusReport(SecureChannel::GeneralStatusCode::kSuccess, SecureChannel::Id,
@@ -51,7 +50,7 @@ CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device)
     System::PacketBufferHandle msg = bbuf.Finalize();
     VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_NO_MEMORY);
 
-    auto * exchange = device->GetExchangeManager()->NewContext(device->GetSecureSession().Value(), nullptr);
+    auto * exchange = exchangeMgr.NewContext(sessionHandle, nullptr);
     VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_NO_MEMORY);
 
     // Per spec, CloseSession reports are always sent with MRP disabled.
@@ -69,12 +68,13 @@ CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device)
     return err;
 }
 
-void CloseSessionCommand::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device)
+void CloseSessionCommand::OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr,
+                                              SessionHandle & sessionHandle)
 {
     auto * command = reinterpret_cast<CloseSessionCommand *>(context);
     VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectedFn: context is null"));
 
-    CHIP_ERROR err = command->CloseSession(device);
+    CHIP_ERROR err = command->CloseSession(exchangeMgr, sessionHandle);
     VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err));
 }
 

diff --git a/examples/chip-tool/commands/pairing/CloseSessionCommand.h b/examples/chip-tool/commands/pairing/CloseSessionCommand.h
@@ -19,7 +19,7 @@
 #pragma once
 
 #include "../common/CHIPCommand.h"
-#include <app/OperationalDeviceProxy.h>
+#include <app/OperationalSessionSetup.h>
 #include <lib/core/CHIPCallback.h>
 #include <lib/core/DataModelTypes.h>
 
@@ -46,11 +46,12 @@ class CloseSessionCommand : public CHIPCommand
     chip::NodeId mDestinationId;
     chip::Optional<uint16_t> mTimeoutSecs;
 
-    static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device);
+    static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr,
+                                    chip::SessionHandle & sessionHandle);
     static void OnDeviceConnectionFailureFn(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error);
 
     // Try to send the action CloseSession status report.
-    CHIP_ERROR CloseSession(chip::DeviceProxy * device);
+    CHIP_ERROR CloseSession(chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle);
 
     chip::Callback::Callback<chip::OnDeviceConnected> mOnDeviceConnectedCallback;
     chip::Callback::Callback<chip::OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;

diff --git a/examples/chip-tool/commands/tests/TestCommand.cpp b/examples/chip-tool/commands/tests/TestCommand.cpp
@@ -51,12 +51,13 @@ CHIP_ERROR TestCommand::WaitForCommissionee(const char * identity,
                                                         &mOnDeviceConnectionFailureCallback);
 }
 
-void TestCommand::OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device)
+void TestCommand::OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr,
+                                      chip::SessionHandle & sessionHandle)
 {
     ChipLogProgress(chipTool, " **** Test Setup: Device Connected\n");
     auto * command = static_cast<TestCommand *>(context);
     VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "Device connected, but cannot run the test, as the context is null"));
-    command->mDevices[command->GetIdentity()] = device;
+    command->mDevices[command->GetIdentity()] = std::make_unique<chip::OperationalDeviceProxy>(&exchangeMgr, sessionHandle);
 
     LogErrorOnFailure(command->ContinueOnChipMainThread(CHIP_NO_ERROR));
 }

diff --git a/examples/chip-tool/commands/tests/TestCommand.h b/examples/chip-tool/commands/tests/TestCommand.h
@@ -67,10 +67,11 @@ class TestCommand : public TestRunner,
     void OnWaitForMs() override { NextTest(); };
 
     /////////// Interaction Model Interface /////////
-    chip::DeviceProxy * GetDevice(const char * identity) override { return mDevices[identity]; }
+    chip::DeviceProxy * GetDevice(const char * identity) override { return mDevices[identity].get(); }
     void OnResponse(const chip::app::StatusIB & status, chip::TLV::TLVReader * data) override{};
 
-    static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device);
+    static void OnDeviceConnectedFn(void * context, chip::Messaging::ExchangeManager & exchangeMgr,
+                                    chip::SessionHandle & sessionHandle);
     static void OnDeviceConnectionFailureFn(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error);
 
     CHIP_ERROR ContinueOnChipMainThread(CHIP_ERROR err) override;
@@ -94,7 +95,7 @@ class TestCommand : public TestRunner,
 
     chip::Optional<char *> mPICSFilePath;
     chip::Optional<uint16_t> mTimeout;
-    std::map<std::string, chip::DeviceProxy *> mDevices;
+    std::map<std::string, std::unique_ptr<chip::OperationalDeviceProxy>> mDevices;
 
     // When set to false, prevents interaction model events from affecting the current test status.
     // This flag exists because if an error happens while processing a response the allocated

diff --git a/examples/light-switch-app/ameba/main/BindingHandler.cpp b/examples/light-switch-app/ameba/main/BindingHandler.cpp
@@ -50,7 +50,8 @@ Engine sShellSwitchBindingSubCommands;
 
 namespace {
 
-void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device)
+void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding,
+                                       OperationalDeviceProxy * peer_device)
 {
     auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
         ChipLogProgress(NotSpecified, "OnOff command succeeds");
@@ -60,6 +61,7 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
         ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format());
     };
 
+    VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
     switch (commandId)
     {
     case Clusters::OnOff::Commands::Toggle::Id:
@@ -106,7 +108,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl
     }
 }
 
-void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context)
+void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context)
 {
     VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
     BindingCommandData * data = static_cast<BindingCommandData *>(context);

diff --git a/examples/light-switch-app/efr32/src/binding-handler.cpp b/examples/light-switch-app/efr32/src/binding-handler.cpp
@@ -51,7 +51,8 @@ Engine sShellSwitchBindingSubCommands;
 
 namespace {
 
-void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device)
+void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding,
+                                       Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle)
 {
     auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
         ChipLogProgress(NotSpecified, "OnOff command succeeds");
@@ -65,20 +66,17 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
     {
     case Clusters::OnOff::Commands::Toggle::Id:
         Clusters::OnOff::Commands::Toggle::Type toggleCommand;
-        Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
-                                         toggleCommand, onSuccess, onFailure);
+        Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, toggleCommand, onSuccess, onFailure);
         break;
 
     case Clusters::OnOff::Commands::On::Id:
         Clusters::OnOff::Commands::On::Type onCommand;
-        Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
-                                         onCommand, onSuccess, onFailure);
+        Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, onCommand, onSuccess, onFailure);
         break;
 
     case Clusters::OnOff::Commands::Off::Id:
         Clusters::OnOff::Commands::Off::Type offCommand;
-        Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
-                                         offCommand, onSuccess, onFailure);
+        Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, offCommand, onSuccess, onFailure);
         break;
     }
 }
@@ -107,7 +105,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl
     }
 }
 
-void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context)
+void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context)
 {
     VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
     BindingCommandData * data = static_cast<BindingCommandData *>(context);
@@ -126,7 +124,9 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro
         switch (data->clusterId)
         {
         case Clusters::OnOff::Id:
-            ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device);
+            VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
+            ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(),
+                                              peer_device->GetSecureSession().Value());
             break;
         }
     }

diff --git a/examples/light-switch-app/esp32/main/BindingHandler.cpp b/examples/light-switch-app/esp32/main/BindingHandler.cpp
@@ -50,7 +50,8 @@ Engine sShellSwitchBindingSubCommands;
 
 namespace {
 
-void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding, DeviceProxy * peer_device)
+void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding,
+                                       Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle)
 {
     auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
         ChipLogProgress(NotSpecified, "OnOff command succeeds");
@@ -64,20 +65,17 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
     {
     case Clusters::OnOff::Commands::Toggle::Id:
         Clusters::OnOff::Commands::Toggle::Type toggleCommand;
-        Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
-                                         toggleCommand, onSuccess, onFailure);
+        Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, toggleCommand, onSuccess, onFailure);
         break;
 
     case Clusters::OnOff::Commands::On::Id:
         Clusters::OnOff::Commands::On::Type onCommand;
-        Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
-                                         onCommand, onSuccess, onFailure);
+        Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, onCommand, onSuccess, onFailure);
         break;
 
     case Clusters::OnOff::Commands::Off::Id:
         Clusters::OnOff::Commands::Off::Type offCommand;
-        Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
-                                         offCommand, onSuccess, onFailure);
+        Controller::InvokeCommandRequest(exchangeMgr, sessionHandle, binding.remote, offCommand, onSuccess, onFailure);
         break;
     }
 }
@@ -106,7 +104,7 @@ void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTabl
     }
 }
 
-void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context)
+void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device, void * context)
 {
     VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
     BindingCommandData * data = static_cast<BindingCommandData *>(context);
@@ -125,7 +123,9 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro
         switch (data->clusterId)
         {
         case Clusters::OnOff::Id:
-            ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device);
+            VerifyOrDie(peer_device != nullptr && peer_device->ConnectionReady());
+            ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device->GetExchangeManager(),
+                                              peer_device->GetSecureSession().Value());
             break;
         }
     }