Skip to content

Commit

Permalink
Fix crashes when closing all exchanges for fabric.
Browse files Browse the repository at this point in the history
Our "close all exchanges except this special one for this fabric" API
messes up exchange refcounting, leading to use-after-free.

The fix is to reuse, as much as possible, the normal "session is going
away" flow to notify exchanges, and other session consumers, that the
sessions are in fact going away.

Fixes project-chip#19747
  • Loading branch information
bzbarsky-apple committed Jun 21, 2022
1 parent 2c7105c commit df4eee1
Show file tree
Hide file tree
Showing 17 changed files with 305 additions and 167 deletions.
22 changes: 21 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()->MarkForRemoval();
{
// Move the session out of the older for the removal, so it doesn't try
// to notify us in our destructor.
const auto & session = mSession.Get();
mSession.Release();
session.Value()->AsSecureSession()->MarkForRemoval();
}

#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,19 @@ ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isEphemeralEx
return ApplicationExchangeDispatch::Instance();
}

void ExchangeContext::AbortAllOtherCommunicationOnFabric()
{
if (!mSession || !mSession->IsSecureSession())
{
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
18 changes: 0 additions & 18 deletions src/messaging/ExchangeMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -376,23 +376,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

0 comments on commit df4eee1

Please sign in to comment.