From f10e350730c2340be9e701f97545159cb95f7498 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 15 Aug 2022 23:47:39 -0400 Subject: [PATCH] Fix the ReadClient destructor to call OnDeallocatePaths. (#21660) 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 | 8 ++++--- 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(+), 11 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 257a86f28790cd..3c3bc5b19e2e76 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()) { @@ -181,7 +190,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 89ea133fad5b1b..66c1428c0fa3e3 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) {} @@ -474,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 481439f8ac3f27..d97b11f3213a8d 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1763,6 +1763,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); @@ -1868,6 +1869,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); @@ -1944,6 +1946,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 026c3d7a5b504f..62b699dffcbdfa 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -300,6 +300,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. @@ -768,6 +775,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 c3a9e57b19edc3..937734b497f836 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -118,6 +118,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.