Skip to content

Commit

Permalink
Fix the ReadClient destructor to call OnDeallocatePaths.
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 committed Aug 5, 2022
1 parent 33999f7 commit 9274ebb
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 19 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 @@ -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
Expand Down
16 changes: 5 additions & 11 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 @@ -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
Expand Down Expand Up @@ -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();
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 @@ -1748,6 +1748,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 @@ -1853,6 +1854,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 @@ -1929,6 +1931,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 @@ -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.
Expand Down Expand Up @@ -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<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 @@ -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.
Expand Down

0 comments on commit 9274ebb

Please sign in to comment.