Skip to content

Commit

Permalink
Fix the ReadClient destructor to call OnDeallocatePaths. (#21660)
Browse files Browse the repository at this point in the history
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 #21658
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Oct 27, 2022
1 parent db85e70 commit 1157976
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 11 deletions.
28 changes: 20 additions & 8 deletions src/app/ReadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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())
{
Expand Down Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions src/app/ReadClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

Expand Down Expand Up @@ -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();
Expand Down
3 changes: 3 additions & 0 deletions src/app/tests/TestReadInteraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down
14 changes: 14 additions & 0 deletions src/controller/TypedReadCallback.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }
Expand Down Expand Up @@ -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<app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }

private:
Expand Down
14 changes: 14 additions & 0 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }
Expand Down
7 changes: 7 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 1157976

Please sign in to comment.