Skip to content

Commit

Permalink
Fix SecureSession being made defunct after expiration (#23097)
Browse files Browse the repository at this point in the history 
When an UpdateNOC call is received on the server, it expires the
associated session and proceeds to send back a response on the
associated exchange. If response fails to get ACK'ed, it will result in
the session being marked defunct. The SecureSession logic
over-aggressively asserts on marking expired sessions as defunct,
causing a VerifyOrDie crash to happen.

Fix:
Remove the VerifyOrDie

Testing:
Added a test in TestAbortExchangesForFabric to catch this scenario. Also
pivoted most of the secure sessions in those test to be of type CASE to
facilitate correctly triggering the various bits of logic in ExchangeMgr
and ReliableMessageMgr in these scenarios.
  • Loading branch information
@mrjerryjohns @pull
mrjerryjohns authored and pull[bot] committed Jul 7, 2023
1 parent 6ac1192 commit 2406417
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 19 deletions.
76 changes: 59 additions & 17 deletions src/messaging/tests/TestAbortExchangesForFabric.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,8 @@ class MockAppDelegate : public ExchangeDelegate
bool mOnMessageReceivedCalled = false;
};

void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext)
void CommonCheckAbortAllButOneExchange(nlTestSuite * inSuite, TestContext & ctx, bool dropResponseMessages)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);

// We want to have two sessions using the same fabric id that we use for
// creating our exchange contexts. That lets us test exchanges on the same
// session as the "special exchange" as well as on other sessions.
Expand All @@ -68,29 +66,32 @@ void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext)
// doing.
// TODO: These should really be CASE sessions...
SessionHolder session1;
CHIP_ERROR err =
sessionManager.InjectPaseSessionWithTestKey(session1, 100, ctx.GetBobFabric()->GetNodeId(), 101, ctx.GetAliceFabricIndex(),
ctx.GetBobAddress(), CryptoContext::SessionRole::kInitiator);
CHIP_ERROR err = sessionManager.InjectCaseSessionWithTestKey(session1, 100, 101, ctx.GetAliceFabric()->GetNodeId(),
ctx.GetBobFabric()->GetNodeId(), ctx.GetAliceFabricIndex(),
ctx.GetBobAddress(), CryptoContext::SessionRole::kInitiator, {});

NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

SessionHolder session1Reply;
err = sessionManager.InjectPaseSessionWithTestKey(session1Reply, 101, ctx.GetAliceFabric()->GetNodeId(), 100,
ctx.GetBobFabricIndex(), ctx.GetAliceAddress(),
CryptoContext::SessionRole::kResponder);
err = sessionManager.InjectCaseSessionWithTestKey(session1Reply, 101, 100, ctx.GetBobFabric()->GetNodeId(),
ctx.GetAliceFabric()->GetNodeId(), ctx.GetBobFabricIndex(),
ctx.GetAliceAddress(), CryptoContext::SessionRole::kResponder, {});

NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// TODO: Ideally this would go to a different peer, but we don't have that
// set up right now: only Alice and Bob have useful node ids and whatnot.
SessionHolder session2;
err =
sessionManager.InjectPaseSessionWithTestKey(session2, 200, ctx.GetBobFabric()->GetNodeId(), 201, ctx.GetAliceFabricIndex(),
ctx.GetBobAddress(), CryptoContext::SessionRole::kInitiator);
err = sessionManager.InjectCaseSessionWithTestKey(session2, 200, 201, ctx.GetAliceFabric()->GetNodeId(),
ctx.GetBobFabric()->GetNodeId(), ctx.GetAliceFabricIndex(),
ctx.GetBobAddress(), CryptoContext::SessionRole::kInitiator, {});

NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

SessionHolder session2Reply;
err = sessionManager.InjectPaseSessionWithTestKey(session2Reply, 201, ctx.GetAliceFabric()->GetNodeId(), 200,
ctx.GetBobFabricIndex(), ctx.GetAliceAddress(),
CryptoContext::SessionRole::kResponder);
err = sessionManager.InjectCaseSessionWithTestKey(session2Reply, 101, 100, ctx.GetBobFabric()->GetNodeId(),
ctx.GetAliceFabric()->GetNodeId(), ctx.GetBobFabricIndex(),
ctx.GetAliceAddress(), CryptoContext::SessionRole::kResponder, {});
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

auto & exchangeMgr = ctx.GetExchangeManager();
Expand Down Expand Up @@ -160,6 +161,12 @@ void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext)
const auto & sessionHandle1 = session1.Get();
const auto & sessionHandle2 = session2.Get();

session1->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
Test::MessagingContext::kResponsiveIdleRetransTimeout, Test::MessagingContext::kResponsiveActiveRetransTimeout));

session1Reply->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
Test::MessagingContext::kResponsiveIdleRetransTimeout, Test::MessagingContext::kResponsiveActiveRetransTimeout));

NL_TEST_ASSERT(inSuite, session1);
NL_TEST_ASSERT(inSuite, session2);
auto * specialExhange = exchangeMgr.NewContext(session1.Get().Value(), &delegate);
Expand All @@ -183,15 +190,49 @@ void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext)
// Should be waiting for an ack now.
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);

ctx.DrainAndServiceIO();
if (dropResponseMessages)
{
//
// This version of the test allows us to validate logic that marks expired sessions as defunct
// on encountering an MRP failure.
//
loopback.mNumMessagesToDrop = Test::LoopbackTransport::kUnlimitedMessageCount;
loopback.mDroppedMessageCount = 0;

//
// We've set the session into responsive mode by altering the MRP intervals, so we should be able to
// trigger a MRP failure due to timing out waiting for an ACK.
//
ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), [&]() { return false; });
}
else
{
ctx.DrainAndServiceIO();
}

// Should not get an app-level response, since we are not expecting one.
NL_TEST_ASSERT(inSuite, !delegate.mOnMessageReceivedCalled);

// We should have gotten our ack.
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);

waitingForSend1->Close();
waitingForSend2->Close();

loopback.mNumMessagesToDrop = 0;
loopback.mDroppedMessageCount = 0;
}

void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);
CommonCheckAbortAllButOneExchange(inSuite, ctx, false);
}

void CheckAbortAllButOneExchangeResponseTimeout(nlTestSuite * inSuite, void * inContext)
{
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);
CommonCheckAbortAllButOneExchange(inSuite, ctx, true);
}

// Test Suite
Expand All @@ -202,7 +243,8 @@ void CheckAbortAllButOneExchange(nlTestSuite * inSuite, void * inContext)
// clang-format off
const nlTest sTests[] =
{
NL_TEST_DEF("Test aborting all but one exchange", CheckAbortAllButOneExchange),
NL_TEST_DEF("Test aborting all but one exchange", CheckAbortAllButOneExchange),
NL_TEST_DEF("Test aborting all but one exchange + response timeout", CheckAbortAllButOneExchangeResponseTimeout),

NL_TEST_SENTINEL()
};
Expand Down
4 changes: 2 additions & 2 deletions src/transport/SecureSession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@ void SecureSession::MarkAsDefunct()

case State::kPendingEviction:
//
// Once a session is headed for eviction, we CANNOT bring it back to either being active or defunct.
// Once a session is headed for eviction, we CANNOT bring it back to either being active or defunct. Let's just
// do nothing and return.
//
VerifyOrDie(false);
return;
}
}
Expand Down

0 comments on commit 2406417

Please sign in to comment.