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 · Sep 6, 2023 · 3466584 · 3466584
1 parent 835a6d2
commit 3466584
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;
  }
  }