Skip to content

Commit

Permalink
Improve lifetime management of CASESession async work helper.
Browse files Browse the repository at this point in the history
As things stood, there were some codepaths (currently not really reached; it
would require scheduling of SendSigma3c to fail and then for someone to
InvokeBackgroundWorkWatchdog on the CASESession) where we could destroy the
object while we were working with references to its members.

Fixes #27541
  • Loading branch information
bzbarsky-apple committed Jul 6, 2023
1 parent 0d1f9c3 commit 0d9b285
Showing 1 changed file with 21 additions and 4 deletions.
25 changes: 21 additions & 4 deletions src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,12 @@ class CASESession::WorkHelper

// After work callback, processed in the main Matter task via `PlatformManager::ScheduleWork`.
// This is a member function to be called on the associated session after the work callback.
// The `status` value is the result of the work callback (called beforehand).
// The `status` value is the result of the work callback (called beforehand), or the status of
// queueing the after work callback back to the Matter thread, if the work callback succeeds
// but queueing fails.
//
// When this callback is called asynchronously (i.e. via ScheduleWork), the helper guarantees
// that it will keep itself (and hence `data`) alive until the callback completes.
typedef CHIP_ERROR (CASESession::*AfterWorkCallback)(DATA & data, CHIP_ERROR status);

public:
Expand All @@ -169,6 +174,9 @@ class CASESession::WorkHelper

// Do the work immediately.
// No scheduling, no outstanding work, no shared lifetime management.
//
// The caller must guarantee that it keeps the helper alive across this call, most likely by
// holding a reference to it on the stack.
CHIP_ERROR DoWork()
{
// Ensure that this function is being called from main Matter thread
Expand All @@ -195,7 +203,7 @@ class CASESession::WorkHelper
auto status = DeviceLayer::PlatformMgr().ScheduleBackgroundWork(WorkHandler, reinterpret_cast<intptr_t>(this));
if (status != CHIP_NO_ERROR)
{
// Release strong ptr since scheduling failed
// Release strong ptr since scheduling failed.
mStrongPtr.reset();
}
return status;
Expand Down Expand Up @@ -246,7 +254,9 @@ class CASESession::WorkHelper
ChipLogError(SecureChannel, "Failed to Schedule the AfterWorkCallback on foreground thread");
helper->mScheduleAfterWorkFailed.store(true);

// Release strong ptr since scheduling failed
// Release strong ptr since scheduling failed, because nothing guarantees that
// AfterWorkHandler will get called at this point to release the reference, and we
// don't want to leak.
helper->mStrongPtr.reset();
}
}
Expand All @@ -258,8 +268,15 @@ class CASESession::WorkHelper
assertChipStackLockedByCurrentThread();

auto * helper = reinterpret_cast<WorkHelper *>(arg);
// Hold strong ptr while work is handled
// Hold strong ptr while work is handled, and ensure that helper->mStrongPtr does not keep
// holding a reference.
auto strongPtr(std::move(helper->mStrongPtr));
if (!strongPtr)
{
// This can happen if scheduling AfterWorkHandler failed. Just grab a strong ref to
// handler directly, to fulfill our API contract.
strongPtr = helper->mWeakPtr.lock();
}
if (auto * session = helper->mSession.load())
{
// Execute callback in Matter thread; session should be OK with this
Expand Down

0 comments on commit 0d9b285

Please sign in to comment.