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.

Sign up for GitHub

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

ExchangeContext: Implement a safer verion Abort/Close #19775

Closed
wants to merge 1 commit into from
Closed

ExchangeContext: Implement a safer verion Abort/Close #19775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
141 changes: 69 additions & 72 deletions src/messaging/ExchangeContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,16 +228,38 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
}
}

void ExchangeContext::DoClose(bool clearRetransTable)
void ExchangeContext::DoClose(bool clearRetransTable, bool shouldCallResponseTimout)
{
// Hold a ref to ourselves so we can make calls into our delegate that might
// decrease our refcount without worrying about use-after-free.
ExchangeHandle ref(*this);

VerifyOrDie(!mFlags.Has(Flags::kFlagClosed));
mFlags.Set(Flags::kFlagClosed);
// This releases refcount assigned in constructor. The initial refcount is associated with kFlagClosed. The DoClose function is
// not re-entrant, it is triggered once and only once when kFlagClosed is changing from false to ture.
Release();

// Clear protocol callbacks
if (mDelegate != nullptr)
bool callResponseTimout = false;

if (IsResponseExpected())
{
mDelegate->OnExchangeClosing(this);
// The application has closed the exchange, call its response timeout callback.
CancelResponseTimer();
SetResponseExpected(false);

// If we trigger the callback here and the user calls Close or Abort in the callback, it becomes too complicated to handle.
// Defer OnResponseTimeout callback until we have committed our state.
callResponseTimout = true;
}
else
{
// Either we're expecting a send or we are in our "just allocated, first send has not happened yet" state.
if (IsSendExpected())
{
mFlags.Clear(Flags::kFlagWillSendMessage);
}
}
mDelegate = nullptr;

// Closure of an exchange context is based on ref counting. The Protocol, when it calls DoClose(), indicates that
// it is done with the exchange context and the message layer sets all callbacks to NULL and does not send anything
Expand All @@ -252,42 +274,51 @@ void ExchangeContext::DoClose(bool clearRetransTable)
mExchangeMgr->GetReliableMessageMgr()->ClearRetransTable(this);
}

// Cancel the response timer.
CancelResponseTimer();
// Call deferred OnResponseTimeout
if (mDelegate != nullptr && callResponseTimout && shouldCallResponseTimout)
mDelegate->OnResponseTimeout(this);

// Check for mDelegate again, because mDelegate can be cleared during OnResponseTimeout event
if (mDelegate != nullptr)
mDelegate->OnExchangeClosing(this);

mDelegate = nullptr;
}

/**
* Gracefully close an exchange context. This call decrements the
* reference count and releases the exchange when the reference
* count goes to zero.
*
*/
void ExchangeContext::Close()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() > 0);
if (mFlags.Has(Flags::kFlagClosed))
{
return;
}

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec - close[" ChipLogFormatExchange "], %s", ChipLogValueExchange(this), __func__);
#endif
if (IsResponseExpected())
{
// Close should not be called on a exchange which is waiting for a response. In normal procedure, close can only be called
// when an exchange is finishing its work. There is no chance that an exchange is finishing its work while waiting for a
// response.
//
// This is caused by a faulty implementation of an application, leave a error message for the application to fix it.
ChipLogError(ExchangeManager, "Close called when waiting for response on exchange " ChipLogFormatExchange,
ChipLogValueExchange(this));
}

DoClose(false);
Release();
// The graceful shutdown should take care of retransmitting the last message using MRP.
// Do not trigger OnResponseTimeout when user is closing the exchange.
DoClose(false /* clearRetransTable */, false /* shouldCallResponseTimout */);
}
/**
* Abort the Exchange context immediately and release all
* references to it.
*
*/

void ExchangeContext::Abort()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() > 0);

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec - abort[" ChipLogFormatExchange "], %s", ChipLogValueExchange(this), __func__);
#endif
if (mFlags.Has(Flags::kFlagClosed))
{
// Exchange is already being closed. It may occur when Abort is called on a closed exchange.
mExchangeMgr->GetReliableMessageMgr()->ClearRetransTable(this);
return;
}

DoClose(true);
Release();
// Do not trigger OnResponseTimeout when user is aborting the exchange.
DoClose(true /* clearRetransTable */, false /* shouldCallResponseTimout */);
}

void ExchangeContextDeletor::Release(ExchangeContext * ec)
Expand All @@ -307,6 +338,8 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons
mSession.Grab(session);
mFlags.Set(Flags::kFlagInitiator, Initiator);
mFlags.Set(Flags::kFlagEphemeralExchange, isEphemeralExchange);
if (!isEphemeralExchange && Initiator)
WillSendMessage();
mDelegate = delegate;

SetAckPending(false);
Expand All @@ -333,11 +366,6 @@ ExchangeContext::~ExchangeContext()
UpdateSEDIntervalMode(false);
#endif

// Ideally, in this scenario, the retransmit table should
// be clear of any outstanding messages for this context and
// the boolean parameter passed to DoClose() should not matter.

DoClose(false);
mExchangeMgr = nullptr;

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
Expand Down Expand Up @@ -373,40 +401,12 @@ void ExchangeContext::OnSessionReleased()
{
if (mFlags.Has(Flags::kFlagClosed))
{
// Exchange is already being closed. It may occur when closing an exchange after sending
// RemoveFabric response which triggers removal of all sessions for the given fabric.
// Exchange is already closed.
mExchangeMgr->GetReliableMessageMgr()->ClearRetransTable(this);
return;
}

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

if (IsResponseExpected())
{
// If we're waiting on a response, we now know it's never going to show up
// and we should notify our delegate accordingly.
CancelResponseTimer();
// We want to Abort, not just Close, so that RMP bits are cleared, so
// don't let NotifyResponseTimeout close us.
NotifyResponseTimeout(/* aCloseIfNeeded = */ false);
Abort();
}
else
{
// Either we're expecting a send or we are in our "just allocated, first
// send has not happened yet" state.
//
// Just mark ourselves as closed. The consumer is responsible for
// releasing us. See documentation for
// ExchangeDelegate::OnExchangeClosing.
if (IsSendExpected())
{
mFlags.Clear(Flags::kFlagWillSendMessage);
}
DoClose(true /* clearRetransTable */);
}
DoClose(true /* clearRetransTable */, true /* shouldCallResponseTimout */);
}

CHIP_ERROR ExchangeContext::StartResponseTimer()
Expand Down Expand Up @@ -440,10 +440,10 @@ void ExchangeContext::HandleResponseTimeout(System::Layer * aSystemLayer, void *
if (ec == nullptr)
return;

ec->NotifyResponseTimeout(/* aCloseIfNeeded = */ true);
ec->NotifyResponseTimeout();
}

void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
void ExchangeContext::NotifyResponseTimeout()
{
SetResponseExpected(false);

Expand All @@ -455,10 +455,7 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
delegate->OnResponseTimeout(this);
}

if (aCloseIfNeeded)
{
MessageHandled();
}
MessageHandled();
}

CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const PayloadHeader & payloadHeader, MessageFlags msgFlags,
Expand Down
12 changes: 5 additions & 7 deletions src/messaging/ExchangeContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,10 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,

uint16_t GetExchangeId() const { return mExchangeId; }

/*
* In order to use reference counting (see refCount below) we use a hold/free paradigm where users of the exchange
* can hold onto it while it's out of their direct control to make sure it isn't closed before everyone's ready.
* A customized version of reference counting is used since there are some extra stuff to do within Release.
*/
/// @brief Gracefully close an exchange context. This function can be call at any time on a valid exchange.
void Close();

/// @brief Abort the Exchange context immediately. This function can be call at any time on a valid exchange.
void Abort();

// Applies a suggested response timeout value based on the session type and the given upper layer processing time for
Expand Down Expand Up @@ -239,14 +237,14 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
* response. If aCloseIfNeeded is true, check whether the exchange needs to
* be closed.
*/
void NotifyResponseTimeout(bool aCloseIfNeeded);
void NotifyResponseTimeout();

CHIP_ERROR StartResponseTimer();

void CancelResponseTimer();
static void HandleResponseTimeout(System::Layer * aSystemLayer, void * aAppState);

void DoClose(bool clearRetransTable);
void DoClose(bool clearRetransTable, bool shouldCallResponseTimout);

/**
* We have handled an application-level message in some way and should
Expand Down
24 changes: 13 additions & 11 deletions src/messaging/ExchangeDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,19 @@ class DLL_EXPORT ExchangeDelegate

/**
* @brief
* This function is the protocol callback to invoke when the associated
* exchange context is being closed.
*
* If the exchange was in a state where it was expecting a message to be
* sent due to an earlier WillSendMessage call or because the exchange has
* just been created as an initiator, the consumer is holding a reference
* to the exchange and it's the consumer's responsibility to call
* Release() on the exchange at some point. The usual way this happens is
* that the consumer tries to send its message, that fails, and the
* consumer calls Close() on the exchange. Calling Close() after an
* OnExchangeClosing() notification is allowed in this situation.
* This function is the protocol callback to invoke when the associated exchange context is being closed.
*
* The exchange is getting released soon after this callback. If there is no message pending in the retrans queue in MRP, the
* exchange will be released immediately after this callback, otherwise the exchange is released after the retrans entry is
* removed, by either receiving an acknowledgment or give up. There is no way to prevent the exchange from releasing at this
* stage.
*
* The user do not need to call Close or Abort to release the reference when this callback is triggered, the reference is
* already released. The user can call Abort if it want to clear the retrans entry of the last message, and speed up releasing
* the exchange.
*
* The user MUST release all pointers to the exchange in the callback, because the exchange is getting release soon, or else
* they become dangling pointer.
*
* @param[in] ec A pointer to the ExchangeContext object.
*/
Expand Down
3 changes: 2 additions & 1 deletion src/messaging/ReliableMessageContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ class ReliableMessageContext
/// When set, signifies that this exchange is waiting for a call to SendMessage.
kFlagWillSendMessage = (1u << 6),

/// When set, we have had Close() or Abort() called on us already.
/// When set, signifies that the exchange is closed. All applications have already lost their refs to the exchange, and the
/// only possible ref is the retrans entry in MPR.
kFlagClosed = (1u << 7),

/// When set, signifies that the exchange is requesting Sleepy End Device active mode.
Expand Down