From 16119000be1e42dfbc6cdc73dc4db2d6504a56d7 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 24 May 2022 12:53:36 -0400 Subject: [PATCH] Fix multiple subscriptions to the same attribute in chip-tool. (#18758) Because the command object is the same for subscriptions to the same attribute, we were reusing the same mSubscribeClient unique_ptr for the new subscription, which killed off the old one. The changes here are as follows: 1) Keep track of a list of subscribe clients in InteractionModelReports. 2) To fix a (pre-existing) issue where we would crash on exit from interactive mode if we had alive subscription, introduce a Cleanup function on CHIPCommand which is called either immediately after Shutdown or when we exit interactive mode, depending on what the command wants. 3) Rearrange the attribute/event read/report commands to make sure we handle cleanup correctly and don't leak ReadClients (even temporarily, in the error cases). Fixes https://github.com/project-chip/connectedhomeip/issues/18245 --- .../commands/clusters/ReportCommand.h | 165 ++++++++++-------- .../chip-tool/commands/common/CHIPCommand.cpp | 21 +++ .../chip-tool/commands/common/CHIPCommand.h | 20 ++- .../interactive/InteractiveCommands.cpp | 1 + .../interaction_model/InteractionModel.cpp | 29 +-- .../interaction_model/InteractionModel.h | 11 +- 6 files changed, 153 insertions(+), 94 deletions(-) diff --git a/examples/chip-tool/commands/clusters/ReportCommand.h b/examples/chip-tool/commands/clusters/ReportCommand.h index a6e5d12d5b8dff..dd533b2fb299d9 100644 --- a/examples/chip-tool/commands/clusters/ReportCommand.h +++ b/examples/chip-tool/commands/clusters/ReportCommand.h @@ -30,8 +30,6 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi InteractionModelReports(this), ModelCommand(commandName, credsIssuerConfig, /* supportsMultipleEndpoints = */ true) {} - virtual void OnSubscription(){}; - /////////// ReadClient Callback Interface ///////// void OnAttributeData(const chip::app::ConcreteDataAttributePath & path, chip::TLV::TLVReader * data, const chip::app::StatusIB & status) override @@ -96,13 +94,14 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi mError = error; } - void OnDone(chip::app::ReadClient *) override + void Shutdown() override { - InteractionModelReports::Shutdown(); - SetCommandExitStatus(mError); + // We don't shut down InteractionModelReports here; we leave it for + // Cleanup to handle. + ModelCommand::Shutdown(); } - void OnSubscriptionEstablished(chip::SubscriptionId subscriptionId) override { OnSubscription(); } + void Cleanup() override { InteractionModelReports::Shutdown(); } protected: // Use a 3x-longer-than-default timeout because wildcard reads can take a @@ -115,42 +114,94 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi CHIP_ERROR mError = CHIP_NO_ERROR; }; -class ReadAttribute : public ReportCommand +class ReadCommand : public ReportCommand +{ +protected: + ReadCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) : + ReportCommand(commandName, credsIssuerConfig) + {} + + void OnDone(chip::app::ReadClient * aReadClient) override + { + InteractionModelReports::CleanupReadClient(aReadClient); + SetCommandExitStatus(mError); + } +}; + +class SubscribeCommand : public ReportCommand +{ +protected: + SubscribeCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) : + ReportCommand(commandName, credsIssuerConfig) + {} + + void OnSubscriptionEstablished(chip::SubscriptionId subscriptionId) override + { + mSubscriptionEstablished = true; + SetCommandExitStatus(CHIP_NO_ERROR); + } + + void OnDone(chip::app::ReadClient * aReadClient) override + { + if (!mSubscriptionEstablished) + { + InteractionModelReports::CleanupReadClient(aReadClient); + SetCommandExitStatus(mError); + } + + // else we must be getting here from Cleanup(), which means we have + // already done our exit status thing, and have done the ReadClient + // cleanup. + } + + void Shutdown() override + { + mSubscriptionEstablished = false; + ReportCommand::Shutdown(); + } + + bool DeferInteractiveCleanup() override { return mSubscriptionEstablished; } + +private: + bool mSubscriptionEstablished = false; +}; + +class ReadAttribute : public ReadCommand { public: - ReadAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("read-by-id", credsIssuerConfig) + ReadAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-by-id", credsIssuerConfig) { AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds, "Comma-separated list of cluster ids to read from (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF to " "indicate a wildcard cluster."); AddAttributeIdArgument(); AddCommonArguments(); - ReportCommand::AddArguments(); + ReadCommand::AddArguments(); } ReadAttribute(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("read-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + ReadCommand("read-by-id", credsIssuerConfig), mClusterIds(1, clusterId) { AddAttributeIdArgument(); AddCommonArguments(); - ReportCommand::AddArguments(); + ReadCommand::AddArguments(); } ReadAttribute(chip::ClusterId clusterId, const char * attributeName, chip::AttributeId attributeId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("read", credsIssuerConfig), + ReadCommand("read", credsIssuerConfig), mClusterIds(1, clusterId), mAttributeIds(1, attributeId) { AddArgument("attr-name", attributeName); AddCommonArguments(); - ReportCommand::AddArguments(); + ReadCommand::AddArguments(); } ~ReadAttribute() {} CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector endpointIds) override { - return ReportCommand::ReadAttribute(device, endpointIds, mClusterIds, mAttributeIds, mFabricFiltered, mDataVersion); + return ReadCommand::ReadAttribute(device, endpointIds, mClusterIds, mAttributeIds, mFabricFiltered, mDataVersion); } private: @@ -175,60 +226,43 @@ class ReadAttribute : public ReportCommand chip::Optional> mDataVersion; }; -class SubscribeAttribute : public ReportCommand +class SubscribeAttribute : public SubscribeCommand { public: - SubscribeAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("subscribe-by-id", credsIssuerConfig) + SubscribeAttribute(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-by-id", credsIssuerConfig) { AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds, "Comma-separated list of cluster ids to subscribe to (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF " "to indicate a wildcard cluster."); AddAttributeIdArgument(); AddCommonArguments(); - ReportCommand::AddArguments(); + SubscribeCommand::AddArguments(); } SubscribeAttribute(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("subscribe-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + SubscribeCommand("subscribe-by-id", credsIssuerConfig), mClusterIds(1, clusterId) { AddAttributeIdArgument(); AddCommonArguments(); - ReportCommand::AddArguments(); + SubscribeCommand::AddArguments(); } SubscribeAttribute(chip::ClusterId clusterId, const char * attributeName, chip::AttributeId attributeId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("subscribe", credsIssuerConfig), + SubscribeCommand("subscribe", credsIssuerConfig), mClusterIds(1, clusterId), mAttributeIds(1, attributeId) { AddArgument("attr-name", attributeName); AddCommonArguments(); - ReportCommand::AddArguments(); + SubscribeCommand::AddArguments(); } ~SubscribeAttribute() {} CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector endpointIds) override { - return ReportCommand::SubscribeAttribute(device, endpointIds, mClusterIds, mAttributeIds, mMinInterval, mMaxInterval, - mFabricFiltered, mDataVersion, mKeepSubscriptions); - } - - void OnSubscription() override - { - // The ReadClient instance can not be released directly into the OnSubscription - // callback since it happens to be called by ReadClient itself which is doing additional - // work after that. - chip::DeviceLayer::PlatformMgr().ScheduleWork( - [](intptr_t arg) { - auto * command = reinterpret_cast(arg); - if (!command->IsInteractive()) - { - command->InteractionModelReports::Shutdown(); - } - command->SetCommandExitStatus(CHIP_NO_ERROR); - }, - reinterpret_cast(this)); + return SubscribeCommand::SubscribeAttribute(device, endpointIds, mClusterIds, mAttributeIds, mMinInterval, mMaxInterval, + mFabricFiltered, mDataVersion, mKeepSubscriptions); } private: @@ -263,43 +297,43 @@ class SubscribeAttribute : public ReportCommand chip::Optional mKeepSubscriptions; }; -class ReadEvent : public ReportCommand +class ReadEvent : public ReadCommand { public: - ReadEvent(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("read-event-by-id", credsIssuerConfig) + ReadEvent(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-event-by-id", credsIssuerConfig) { AddArgument("cluster-id", 0, UINT32_MAX, &mClusterIds); AddArgument("event-id", 0, UINT32_MAX, &mEventIds); AddArgument("fabric-filtered", 0, 1, &mFabricFiltered); AddArgument("event-min", 0, UINT64_MAX, &mEventNumber); - ReportCommand::AddArguments(); + ReadCommand::AddArguments(); } ReadEvent(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("read-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + ReadCommand("read-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId) { AddArgument("event-id", 0, UINT32_MAX, &mEventIds); AddArgument("fabric-filtered", 0, 1, &mFabricFiltered); AddArgument("event-min", 0, UINT64_MAX, &mEventNumber); - ReportCommand::AddArguments(); + ReadCommand::AddArguments(); } ReadEvent(chip::ClusterId clusterId, const char * eventName, chip::EventId eventId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("read-event", credsIssuerConfig), + ReadCommand("read-event", credsIssuerConfig), mClusterIds(1, clusterId), mEventIds(1, eventId) { AddArgument("event-name", eventName); AddArgument("fabric-filtered", 0, 1, &mFabricFiltered); AddArgument("event-min", 0, UINT64_MAX, &mEventNumber); - ReportCommand::AddArguments(); + ReadCommand::AddArguments(); } ~ReadEvent() {} CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector endpointIds) override { - return ReportCommand::ReadEvent(device, endpointIds, mClusterIds, mEventIds, mFabricFiltered, mEventNumber); + return ReadCommand::ReadEvent(device, endpointIds, mClusterIds, mEventIds, mFabricFiltered, mEventNumber); } private: @@ -309,10 +343,10 @@ class ReadEvent : public ReportCommand chip::Optional mEventNumber; }; -class SubscribeEvent : public ReportCommand +class SubscribeEvent : public SubscribeCommand { public: - SubscribeEvent(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("subscribe-event-by-id", credsIssuerConfig) + SubscribeEvent(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-event-by-id", credsIssuerConfig) { AddArgument("cluster-id", 0, UINT32_MAX, &mClusterIds); AddArgument("event-id", 0, UINT32_MAX, &mEventIds); @@ -321,11 +355,11 @@ class SubscribeEvent : public ReportCommand AddArgument("fabric-filtered", 0, 1, &mFabricFiltered); AddArgument("event-min", 0, UINT64_MAX, &mEventNumber); AddArgument("keepSubscriptions", 0, 1, &mKeepSubscriptions); - ReportCommand::AddArguments(); + SubscribeCommand::AddArguments(); } SubscribeEvent(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("subscribe-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + SubscribeCommand("subscribe-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId) { AddArgument("event-id", 0, UINT32_MAX, &mEventIds); AddArgument("min-interval", 0, UINT16_MAX, &mMinInterval); @@ -333,12 +367,12 @@ class SubscribeEvent : public ReportCommand AddArgument("fabric-filtered", 0, 1, &mFabricFiltered); AddArgument("event-min", 0, UINT64_MAX, &mEventNumber); AddArgument("keepSubscriptions", 0, 1, &mKeepSubscriptions); - ReportCommand::AddArguments(); + SubscribeCommand::AddArguments(); } SubscribeEvent(chip::ClusterId clusterId, const char * eventName, chip::EventId eventId, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand("subscribe-event", credsIssuerConfig), + SubscribeCommand("subscribe-event", credsIssuerConfig), mClusterIds(1, clusterId), mEventIds(1, eventId) { AddArgument("event-name", eventName, "Event name."); @@ -350,32 +384,15 @@ class SubscribeEvent : public ReportCommand AddArgument("event-min", 0, UINT64_MAX, &mEventNumber); AddArgument("keepSubscriptions", 0, 1, &mKeepSubscriptions, "false - Terminate existing subscriptions from initiator.\n true - Leave existing subscriptions in place."); - ReportCommand::AddArguments(); + SubscribeCommand::AddArguments(); } ~SubscribeEvent() {} CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector endpointIds) override { - return ReportCommand::SubscribeEvent(device, endpointIds, mClusterIds, mEventIds, mMinInterval, mMaxInterval, - mFabricFiltered, mEventNumber, mKeepSubscriptions); - } - - void OnSubscription() override - { - // The ReadClient instance can not be released directly into the OnEventSubscription - // callback since it happens to be called by ReadClient itself which is doing additional - // work after that. - chip::DeviceLayer::PlatformMgr().ScheduleWork( - [](intptr_t arg) { - auto * command = reinterpret_cast(arg); - if (!command->IsInteractive()) - { - command->InteractionModelReports::Shutdown(); - } - command->SetCommandExitStatus(CHIP_NO_ERROR); - }, - reinterpret_cast(this)); + return SubscribeCommand::SubscribeEvent(device, endpointIds, mClusterIds, mEventIds, mMinInterval, mMaxInterval, + mFabricFiltered, mEventNumber, mKeepSubscriptions); } private: diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 6b7d3b0702b0bf..729c77e291a1a3 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -31,6 +31,7 @@ #endif // CHIP_CONFIG_TRANSPORT_TRACE_ENABLED std::map> CHIPCommand::mCommissioners; +std::set CHIPCommand::sDeferredCleanups; using DeviceControllerFactory = chip::Controller::DeviceControllerFactory; @@ -180,8 +181,19 @@ CHIP_ERROR CHIPCommand::Run() CHIP_ERROR err = StartWaiting(GetWaitDuration()); + bool deferCleanup = (IsInteractive() && DeferInteractiveCleanup()); + Shutdown(); + if (deferCleanup) + { + sDeferredCleanups.insert(this); + } + else + { + Cleanup(); + } + ReturnErrorOnFailure(MaybeTearDownStack()); return err; @@ -422,3 +434,12 @@ void CHIPCommand::StopWaiting() LogErrorOnFailure(chip::DeviceLayer::PlatformMgr().StopEventLoopTask()); #endif // CONFIG_USE_SEPARATE_EVENTLOOP } + +void CHIPCommand::ExecuteDeferredCleanups() +{ + for (auto * cmd : sDeferredCleanups) + { + cmd->Cleanup(); + } + sDeferredCleanups.clear(); +} diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 0b051dbe31b02c..c1ff2d1acad46d 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -94,10 +94,24 @@ class CHIPCommand : public Command // Get the wait duration, in seconds, before the command times out. virtual chip::System::Clock::Timeout GetWaitDuration() const = 0; - // Shut down the command, in case any work needs to be done after the event - // loop has been stopped. + // Shut down the command. After a Shutdown call the command object is ready + // to be used for another command invocation. virtual void Shutdown() {} + // Clean up any resources allocated by the command. Some commands may hold + // on to resources after Shutdown(), but Cleanup() will guarantee those are + // cleaned up. + virtual void Cleanup() {} + + // If true, skip calling Cleanup() when in interactive mode, so the command + // can keep doing work as needed. Cleanup() will be called when quitting + // interactive mode. This method will be called before Shutdown, so it can + // use member values that Shutdown will normally reset. + virtual bool DeferInteractiveCleanup() { return false; } + + // Execute any deferred cleanups. Used when exiting interactive mode. + void ExecuteDeferredCleanups(); + PersistentStorage mDefaultStorage; PersistentStorage mCommissionerStorage; chip::Credentials::GroupDataProviderImpl mGroupDataProvider{ kMaxGroupsPerFabric, kMaxGroupKeysPerFabric }; @@ -122,6 +136,8 @@ class CHIPCommand : public Command CHIP_ERROR ShutdownCommissioner(std::string key); chip::FabricId CurrentCommissionerId(); static std::map> mCommissioners; + static std::set sDeferredCleanups; + chip::Optional mCommissionerName; chip::Optional mCommissionerNodeId; chip::Optional mBleAdapterId; diff --git a/examples/chip-tool/commands/interactive/InteractiveCommands.cpp b/examples/chip-tool/commands/interactive/InteractiveCommands.cpp index d43ccdd992e227..10459305562b48 100644 --- a/examples/chip-tool/commands/interactive/InteractiveCommands.cpp +++ b/examples/chip-tool/commands/interactive/InteractiveCommands.cpp @@ -95,6 +95,7 @@ bool InteractiveStartCommand::ParseCommand(char * command) { if (strcmp(command, kInteractiveModeStopCommand) == 0) { + ExecuteDeferredCleanups(); return false; } diff --git a/src/app/tests/suites/commands/interaction_model/InteractionModel.cpp b/src/app/tests/suites/commands/interaction_model/InteractionModel.cpp index 61ead1cb835a7a..f52e5ae5261a90 100644 --- a/src/app/tests/suites/commands/interaction_model/InteractionModel.cpp +++ b/src/app/tests/suites/commands/interaction_model/InteractionModel.cpp @@ -116,9 +116,7 @@ void InteractionModel::OnError(CHIP_ERROR error) void InteractionModel::OnDone(ReadClient * aReadClient) { - // TODO: Keep track of multiple read/subscribe interactions, clear out the - // right thing here. - mReadClient.reset(); + InteractionModelReports::CleanupReadClient(aReadClient); ContinueOnChipMainThread(CHIP_NO_ERROR); } @@ -300,10 +298,11 @@ CHIP_ERROR InteractionModelReports::ReportAttribute(DeviceProxy * device, std::v } } - auto & client = interactionType == ReadClient::InteractionType::Subscribe ? mSubscribeClient : mReadClient; - client = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), mBufferedReadAdapter, - interactionType); - return client->SendRequest(params); + auto client = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), + mBufferedReadAdapter, interactionType); + ReturnErrorOnFailure(client->SendRequest(params)); + mReadClients.push_back(std::move(client)); + return CHIP_NO_ERROR; } CHIP_ERROR InteractionModelReports::ReportEvent(DeviceProxy * device, std::vector endpointIds, @@ -405,8 +404,16 @@ CHIP_ERROR InteractionModelReports::ReportEvent(DeviceProxy * device, std::vecto } } - auto & client = interactionType == ReadClient::InteractionType::Subscribe ? mSubscribeClient : mReadClient; - client = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), mBufferedReadAdapter, - interactionType); - return client->SendRequest(params); + auto client = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), + mBufferedReadAdapter, interactionType); + ReturnErrorOnFailure(client->SendRequest(params)); + mReadClients.push_back(std::move(client)); + return CHIP_NO_ERROR; +} + +void InteractionModelReports::CleanupReadClient(ReadClient * aReadClient) +{ + mReadClients.erase( + std::remove_if(mReadClients.begin(), mReadClients.end(), [aReadClient](auto & item) { return item.get() == aReadClient; }), + mReadClients.end()); } diff --git a/src/app/tests/suites/commands/interaction_model/InteractionModel.h b/src/app/tests/suites/commands/interaction_model/InteractionModel.h index af3aa2684b7191..b7b983fe550185 100644 --- a/src/app/tests/suites/commands/interaction_model/InteractionModel.h +++ b/src/app/tests/suites/commands/interaction_model/InteractionModel.h @@ -89,14 +89,11 @@ class InteractionModelReports const chip::Optional & eventNumber = chip::NullOptional, const chip::Optional & keepSubscriptions = chip::NullOptional); - void Shutdown() - { - mSubscribeClient.reset(); - mReadClient.reset(); - } + void Shutdown() { mReadClients.clear(); } + + void CleanupReadClient(chip::app::ReadClient * aReadClient); - std::unique_ptr mReadClient; - std::unique_ptr mSubscribeClient; + std::vector> mReadClients; chip::app::BufferedReadCallback mBufferedReadAdapter; };