Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes when closing all exchanges for fabric. #19780

Merged
merged 6 commits into from
Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,13 @@ ExchangeContext::~ExchangeContext()
VerifyOrDie(!IsAckPending());

if (ReleaseSessionOnDestruction() && mSession)
mSession->AsSecureSession()->MarkForEviction();
{
// Move the session out of the holder for the eviction, so it doesn't try
// to notify us in our destructor.
Optional<SessionHandle> session = mSession.Get();
mSession.Release();
session.Value()->AsSecureSession()->MarkForEviction();
}

#if CONFIG_DEVICE_LAYER && CHIP_DEVICE_CONFIG_ENABLE_SED
// Make sure that the exchange withdraws the request for Sleepy End Device active mode.
Expand Down Expand Up @@ -573,5 +579,20 @@ ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isEphemeralEx
return ApplicationExchangeDispatch::Instance();
}

void ExchangeContext::AbortAllOtherCommunicationOnFabric()
mrjerryjohns marked this conversation as resolved.
Show resolved Hide resolved
{
if (!mSession || !mSession->IsSecureSession())
{
ChipLogError(ExchangeManager, "AbortAllOtherCommunicationOnFabric called on a non-PASE/CASE session");
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
return;
}

GetExchangeMgr()->GetSessionManager()->ReleaseSessionsForFabricExceptOne(mSession->GetFabricIndex(), mSession.Get().Value());

mSession->AsSecureSession()->MarkInactive(mSession);

SetAutoReleaseSession();
}

} // namespace Messaging
} // namespace chip
26 changes: 26 additions & 0 deletions src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,17 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
// using this function is recommended.
void SetResponseTimeout(Timeout timeout);

// This API is used by commands that need to shut down all existing
// sessions/exchanges on a fabric but need to make sure the response to the
// command still goes out on the exchange the command came in on. This API
// will ensure that all secure sessions for the fabric this exchanges is on
// are released except the one this exchange is using, and will release
// that session once this exchange is done sending the response.
//
// This API is a no-op if called on an exchange that is not using a
// SecureSession.
void AbortAllOtherCommunicationOnFabric();

private:
Timeout mResponseTimeout{ 0 }; // Maximum time to wait for response (in milliseconds); 0 disables response timeout.
ExchangeDelegate * mDelegate = nullptr;
Expand Down Expand Up @@ -274,7 +285,22 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
void UpdateSEDIntervalMode(bool activeMode);

static ExchangeMessageDispatch & GetMessageDispatch(bool isEphemeralExchange, ExchangeDelegate * delegate);

// If SetAutoReleaseSession() is called, this exchange must be using a SecureSession, and should
// evict it when the exchange is done with all its work (including any MRP traffic).
inline void SetAutoReleaseSession();
inline bool ReleaseSessionOnDestruction();
};

inline void ExchangeContext::SetAutoReleaseSession()
{
mFlags.Set(Flags::kFlagAutoReleaseSession, true);
}

inline bool ExchangeContext::ReleaseSessionOnDestruction()
{
return mFlags.Has(Flags::kFlagAutoReleaseSession);
}

} // namespace Messaging
} // namespace chip
27 changes: 8 additions & 19 deletions src/messaging/ExchangeMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,14 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
packetHeader.GetDestinationGroupId().Value());
}

// Do not handle unsolicited messages on a inactive session.
// Do not handle messages that don't match an existing exchange on a
// inactive session, since we should not be creating new exchanges there.
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
if (!session->IsActiveSession())
{
ChipLogProgress(ExchangeManager, "Dropping message on inactive session that does not match an existing exchange");
return;
}

// If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator.
// Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all
// unsolicited messages must be marked as being from an initiator.
Expand Down Expand Up @@ -376,23 +383,5 @@ void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * deleg
});
}

void ExchangeManager::AbortExchangesForFabricExceptOne(FabricIndex fabricIndex, ExchangeContext * deferred)
{
VerifyOrDie(deferred->HasSessionHandle() && deferred->GetSessionHandle()->IsSecureSession());

mContextPool.ForEachActiveObject([&](auto * ec) {
if (ec->HasSessionHandle() && ec->GetSessionHandle()->GetFabricIndex() == fabricIndex)
{
if (ec == deferred)
ec->SetAutoReleaseSession();
else
ec->Abort();
}
return Loop::Continue;
});

mSessionManager->ReleaseSessionsForFabricExceptOne(fabricIndex, deferred->GetSessionHandle());
}

} // namespace Messaging
} // namespace chip
5 changes: 0 additions & 5 deletions src/messaging/ExchangeMgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,6 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate
*/
void CloseAllContextsForDelegate(const ExchangeDelegate * delegate);

// This API is used by commands that need to shut down all existing exchanges on a fabric but need to make sure the response to
// the command still goes out on the exchange the command came in on. This API flags that one exchange to shut down its session
// when it's done.
void AbortExchangesForFabricExceptOne(FabricIndex fabricIndex, ExchangeContext * deferred);

SessionManager * GetSessionManager() const { return mSessionManager; }

ReliableMessageMgr * GetReliableMessageMgr() { return &mReliableMessageMgr; };
Expand Down
15 changes: 0 additions & 15 deletions src/messaging/ReliableMessageContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,6 @@ class ReliableMessageContext
/// Determine whether this exchange is a EphemeralExchange for replying a StandaloneAck
bool IsEphemeralExchange() const;

// If SetAutoReleaseSession() is called, this exchange must be using a SecureSession, and should
// evict it when the exchange is done with all its work (including any MRP traffic).
void SetAutoReleaseSession();
bool ReleaseSessionOnDestruction();

/**
* Get the reliable message manager that corresponds to this reliable
* message context.
Expand Down Expand Up @@ -253,15 +248,5 @@ inline bool ReliableMessageContext::IsEphemeralExchange() const
return mFlags.Has(Flags::kFlagEphemeralExchange);
}

inline void ReliableMessageContext::SetAutoReleaseSession()
{
mFlags.Set(Flags::kFlagAutoReleaseSession, true);
}

inline bool ReliableMessageContext::ReleaseSessionOnDestruction()
{
return mFlags.Has(Flags::kFlagAutoReleaseSession);
}

} // namespace Messaging
} // namespace chip
11 changes: 4 additions & 7 deletions src/messaging/tests/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,13 @@ static_library("helpers") {
chip_test_suite("tests") {
output_name = "libMessagingLayerTests"

sources = [ "TestMessagingLayer.h" ]
test_sources = []

if (chip_device_platform != "efr32") {
# TODO(#10447): ReliableMessage Test has HF, and ExchangeMgr hangs on EFR32.
sources += [
# And TestAbortExchangesForFabric does not link on EFR32 for some reason.
test_sources += [
"TestAbortExchangesForFabric.cpp",
"TestExchangeMgr.cpp",
"TestReliableMessageProtocol.cpp",
]
Expand All @@ -67,9 +69,4 @@ chip_test_suite("tests") {
"${nlio_root}:nlio",
"${nlunit_test_root}:nlunit-test",
]

tests = [
"TestExchangeMgr",
"TestReliableMessageProtocol",
]
}
2 changes: 0 additions & 2 deletions src/messaging/tests/MessagingContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,6 @@ class MessagingContext : public PlatformMemoryUser
static const uint16_t kAliceKeyId = 2;
static const uint16_t kCharlieKeyId = 3;
static const uint16_t kDavidKeyId = 4;
NodeId GetBobNodeId() const;
NodeId GetAliceNodeId() const;
GroupId GetFriendsGroupId() const { return mFriendsGroupId; }

SessionManager & GetSecureSessionManager() { return mSessionManager; }
Expand Down
Loading