From b7718f3a14fd7cb238a41f8b06b5290812e1eb13 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 5 Aug 2022 00:15:01 -0400 Subject: [PATCH] Fix the ReadClient destructor to call OnDeallocatePaths. This is actually somewhat dangerous, because there is a good chance ~ReadClient is running from under the destructor of the ReadClient::Callback and we are invoking a virtual method on the callback. But this is the only way to ensure we don't leak. ReadClient::Callback implementations need to be able to handle this. Fixes https://github.com/project-chip/connectedhomeip/issues/21658 --- src/app/ReadClient.cpp | 28 +++++++++++++++------- src/app/ReadClient.h | 16 ++++--------- src/app/tests/TestReadInteraction.cpp | 3 +++ src/controller/TypedReadCallback.h | 14 +++++++++++ src/darwin/Framework/CHIP/MTRBaseDevice.mm | 14 +++++++++++ src/darwin/Framework/CHIP/MTRDevice.mm | 7 ++++++ 6 files changed, 63 insertions(+), 19 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 19e1ced153334c..ce059a658256aa 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -68,20 +68,29 @@ void ReadClient::ClearActiveSubscriptionState() void ReadClient::StopResubscription() { - CancelLivenessCheckTimer(); CancelResubscribeTimer(); - mpCallback.OnDeallocatePaths(std::move(mReadPrepareParams)); + + // Only deallocate the paths if they are not already deallocated. + if (mReadPrepareParams.mpAttributePathParamsList != nullptr || mReadPrepareParams.mpEventPathParamsList != nullptr || + mReadPrepareParams.mpDataVersionFilterList != nullptr) + { + mpCallback.OnDeallocatePaths(std::move(mReadPrepareParams)); + // Make sure we will never try to free those pointers again. + mReadPrepareParams.mpAttributePathParamsList = nullptr; + mReadPrepareParams.mAttributePathParamsListSize = 0; + mReadPrepareParams.mpEventPathParamsList = nullptr; + mReadPrepareParams.mEventPathParamsListSize = 0; + mReadPrepareParams.mpDataVersionFilterList = nullptr; + mReadPrepareParams.mDataVersionFilterListSize = 0; + } } ReadClient::~ReadClient() { + Close(CHIP_NO_ERROR, /* allowResubscription = */ false, /* allowOnDone = */ false); if (IsSubscriptionType()) { - CancelLivenessCheckTimer(); - CancelResubscribeTimer(); - - // // Only remove ourselves from the engine's tracker list if we still continue to have a valid pointer to it. // This won't be the case if the engine shut down before this destructor was called (in which case, mpImEngine // will point to null) @@ -141,7 +150,7 @@ CHIP_ERROR ReadClient::ScheduleResubscription(uint32_t aTimeTillNextResubscripti return CHIP_NO_ERROR; } -void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription) +void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription, bool allowOnDone) { if (IsReadType()) { @@ -182,7 +191,10 @@ void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription) StopResubscription(); } - mpCallback.OnDone(this); + if (allowOnDone) + { + mpCallback.OnDone(this); + } } const char * ReadClient::GetStateStr() const diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 2b161a7b62e6f5..b2bf688b60ac4f 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -200,8 +200,9 @@ class ReadClient : public Messaging::ExchangeDelegate * This function is invoked when using SendAutoResubscribeRequest, where the ReadClient was configured to auto re-subscribe * and the ReadPrepareParams was moved into this client for management. This will have to be free'ed appropriately by the * application. If SendAutoResubscribeRequest fails, this function will be called before it returns the failure. If - * SendAutoResubscribeRequest succeeds, this function will be called immediately before calling OnDone. If - * SendAutoResubscribeRequest is not called, this function will not be called. + * SendAutoResubscribeRequest succeeds, this function will be called immediately before calling OnDone, or + * when the ReadClient is destroyed, if that happens before OnDone. If SendAutoResubscribeRequest is not called, + * this function will not be called. */ virtual void OnDeallocatePaths(ReadPrepareParams && aReadPrepareParams) {} @@ -272,14 +273,6 @@ class ReadClient : public Messaging::ExchangeDelegate */ ~ReadClient() override; - /* - * This forcibly closes the exchange context if a valid one is pointed to. Such a situation does - * not arise during normal message processing flows that all normally call Close() above. This can only - * arise due to application-initiated destruction of the object when this object is handling receiving/sending - * message payloads. Abort() should be called first before the object is destroyed. - */ - void Abort(); - /** * Send a request. There can be one request outstanding on a given ReadClient. * If SendRequest returns success, no more SendRequest calls can happen on this ReadClient @@ -482,8 +475,9 @@ class ReadClient : public Messaging::ExchangeDelegate * AND if this ReadClient instance is tracking a subscription AND the applications decides to do so * in their implementation of Callback::OnResubscriptionNeeded(). * + * If allowOnDone is false, will not call OnDone. */ - void Close(CHIP_ERROR aError, bool allowResubscription = true); + void Close(CHIP_ERROR aError, bool allowResubscription = true, bool allowOnDone = true); void StopResubscription(); void ClearActiveSubscriptionState(); diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 660cd8ef8020c5..8685a59a8e72ae 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1761,6 +1761,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap delegate.mGotReport = false; + attributePathParams.release(); err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -1866,6 +1867,7 @@ void TestReadInteraction::TestSubscribePartialOverlap(nlTestSuite * apSuite, voi delegate.mGotReport = false; + attributePathParams.release(); err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -1942,6 +1944,7 @@ void TestReadInteraction::TestSubscribeSetDirtyFullyOverlap(nlTestSuite * apSuit delegate.mGotReport = false; + attributePathParams.release(); err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); diff --git a/src/controller/TypedReadCallback.h b/src/controller/TypedReadCallback.h index 7fa58df7c932b4..65f4b51a11cd78 100644 --- a/src/controller/TypedReadCallback.h +++ b/src/controller/TypedReadCallback.h @@ -65,6 +65,13 @@ class TypedReadAttributeCallback final : public app::ReadClient::Callback mBufferedReadAdapter(*this) {} + ~TypedReadAttributeCallback() + { + // Ensure we release the ReadClient before we tear down anything else, + // so it can call our OnDeallocatePaths properly. + mReadClient = nullptr; + } + app::BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; } void AdoptReadClient(Platform::UniquePtr aReadClient) { mReadClient = std::move(aReadClient); } @@ -180,6 +187,13 @@ class TypedReadEventCallback final : public app::ReadClient::Callback mOnResubscriptionAttempt(aOnResubscriptionAttempt) {} + ~TypedReadEventCallback() + { + // Ensure we release the ReadClient before we tear down anything else, + // so it can call our OnDeallocatePaths properly. + mReadClient = nullptr; + } + void AdoptReadClient(Platform::UniquePtr aReadClient) { mReadClient = std::move(aReadClient); } private: diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index 9b3f10521ae465..2e0e554e2552c5 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -299,6 +299,13 @@ - (void)invalidateCASESession { } + ~SubscriptionCallback() + { + // Ensure we release the ReadClient before we tear down anything else, + // so it can call our OnDeallocatePaths properly. + mReadClient = nullptr; + } + BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; } // We need to exist to get a ReadClient, so can't take this as a constructor argument. @@ -767,6 +774,13 @@ CHIP_ERROR Encode(chip::TLV::TLVWriter & writer, chip::TLV::Tag tag) const { } + ~BufferedReadAttributeCallback() + { + // Ensure we release the ReadClient before we tear down anything else, + // so it can call our OnDeallocatePaths properly. + mReadClient = nullptr; + } + app::BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; } void AdoptReadClient(Platform::UniquePtr aReadClient) { mReadClient = std::move(aReadClient); } diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index bfa1d55a910af7..affe3454a3699c 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -127,6 +127,13 @@ - (id)strongObject { } + ~SubscriptionCallback() + { + // Ensure we release the ReadClient before we tear down anything else, + // so it can call our OnDeallocatePaths properly. + mReadClient = nullptr; + } + BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; } // We need to exist to get a ReadClient, so can't take this as a constructor argument.