Skip to content

Commit

Permalink
Gracefully handle an exchange delegate evicting the session on timeou…
Browse files Browse the repository at this point in the history
…t. (#25908)

Otherwise the session eviction (which closes our exchange) would destroy it, and
then we would unwind into ExchangeContext::NotifyResponseTimeout and try to use
the exchange's members.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Dec 18, 2023
1 parent 6d8a224 commit 1077876
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 0 deletions.
5 changes: 5 additions & 0 deletions src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,11 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
{
SetResponseExpected(false);

// Hold a ref to ourselves so we can make calls into our delegate that might
// decrease our refcount (e.g. by expiring out session) without worrying
// about use-after-free.
ExchangeHandle ref(*this);

// mSession might be null if this timeout is due to the session being
// evicted.
if (mSession)
Expand Down
36 changes: 36 additions & 0 deletions src/messaging/tests/TestExchangeMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ class WaitForTimeoutDelegate : public ExchangeDelegate
bool IsOnResponseTimeoutCalled = false;
};

class ExpireSessionFromTimeoutDelegate : public WaitForTimeoutDelegate
{
void OnResponseTimeout(ExchangeContext * ec) override
{
ec->GetSessionHandle()->AsSecureSession()->MarkForEviction();
WaitForTimeoutDelegate::OnResponseTimeout(ec);
}
};

void CheckNewContextTest(nlTestSuite * inSuite, void * inContext)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);
Expand Down Expand Up @@ -162,6 +171,32 @@ void CheckSessionExpirationTimeout(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, ctx.CreateSessionAliceToBob() == CHIP_NO_ERROR);
}

void CheckSessionExpirationDuringTimeout(nlTestSuite * inSuite, void * inContext)
{
using namespace chip::System::Clock::Literals;

TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);

ExpireSessionFromTimeoutDelegate sendDelegate;
ExchangeContext * ec1 = ctx.NewExchangeToBob(&sendDelegate);

ec1->SetResponseTimeout(System::Clock::Timeout(100));

ec1->SendMessage(Protocols::BDX::Id, kMsgType_TEST1, System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize),
SendFlags(Messaging::SendMessageFlags::kExpectResponse).Set(Messaging::SendMessageFlags::kNoAutoRequestAck));

ctx.DrainAndServiceIO();
NL_TEST_ASSERT(inSuite, !sendDelegate.IsOnResponseTimeoutCalled);

// Wait for our timeout to elapse. Give it an extra 100ms.
ctx.GetIOContext().DriveIOUntil(200_ms32, [&sendDelegate] { return sendDelegate.IsOnResponseTimeoutCalled; });

NL_TEST_ASSERT(inSuite, sendDelegate.IsOnResponseTimeoutCalled);

// recreate closed session.
NL_TEST_ASSERT(inSuite, ctx.CreateSessionAliceToBob() == CHIP_NO_ERROR);
}

void CheckUmhRegistrationTest(nlTestSuite * inSuite, void * inContext)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);
Expand Down Expand Up @@ -237,6 +272,7 @@ const nlTest sTests[] =
NL_TEST_DEF("Test ExchangeMgr::CheckExchangeMessages", CheckExchangeMessages),
NL_TEST_DEF("Test OnConnectionExpired basics", CheckSessionExpirationBasics),
NL_TEST_DEF("Test OnConnectionExpired timeout handling", CheckSessionExpirationTimeout),
NL_TEST_DEF("Test session eviction in timeout handling", CheckSessionExpirationDuringTimeout),

NL_TEST_SENTINEL()
};
Expand Down

0 comments on commit 1077876

Please sign in to comment.