Skip to content

Commit

Permalink
Fix ExchangeContext leaks (#21846)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mrjerryjohns @pull
mrjerryjohns authored and pull[bot] committed Nov 13, 2023
1 parent d90cece commit 3731200
Show file tree
Hide file tree
Showing 13 changed files with 677 additions and 112 deletions.
92 changes: 83 additions & 9 deletions src/app/tests/TestCommandInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,10 @@ class TestCommandInteraction

static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerRejectMultipleCommands(nlTestSuite * apSuite, void * apContext);

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
static void TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext);
#endif

static void TestCommandSenderCommandSuccessResponseFlow(nlTestSuite * apSuite, void * apContext);
static void TestCommandSenderCommandAsyncSuccessResponseFlow(nlTestSuite * apSuite, void * apContext);
Expand Down Expand Up @@ -496,10 +499,21 @@ void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSu
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));

auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

err = commandHandler.SendCommandResponse();

NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);

//
// Ordinarily, the ExchangeContext will close itself upon sending the final message / error'ing out on a responder exchange
// when unwinding back from an OnMessageReceived callback. Since that isn't the case in this artificial setup here
// (where we created a responder exchange that's not responding to anything), we need
// to explicitly close it out. This is not expected in normal application logic.
//
exchange->Close();
}

void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSuite, void * apContext)
Expand Down Expand Up @@ -532,14 +546,16 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite
System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));
auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

err = commandHandler.PrepareCommand(path);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
err = commandHandler.FinishCommand();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
err = commandHandler.SendCommandResponse();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

commandHandler.Close();
}

Expand All @@ -565,7 +581,8 @@ void TestCommandInteraction::ValidateCommandHandlerWithSendCommand(nlTestSuite *
System::PacketBufferHandle commandPacket;

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));
auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

AddInvokeResponseData(apSuite, apContext, &commandHandler, aNeedStatusCode);
err = commandHandler.Finalize(commandPacket);
Expand All @@ -580,6 +597,14 @@ void TestCommandInteraction::ValidateCommandHandlerWithSendCommand(nlTestSuite *
err = invokeResponseMessageParser.CheckSchemaValidity();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#endif

//
// Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an
// OnMessageReceived callback and not having sent a subsequent message. Since that isn't the case in this artificial setup here
// (where we created a responder exchange that's not responding to anything), we need to explicitly close it out. This is not
// expected in normal application logic.
//
exchange->Close();
}

void TestCommandInteraction::TestCommandHandlerWithSendSimpleCommandData(nlTestSuite * apSuite, void * apContext)
Expand Down Expand Up @@ -625,7 +650,8 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite *
System::PacketBufferHandle commandPacket;

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));
auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

auto path = MakeTestCommandPath();

Expand All @@ -642,6 +668,14 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite *
err = invokeResponseMessageParser.CheckSchemaValidity();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#endif

//
// Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an
// OnMessageReceived callback and not having sent a subsequent message. Since that isn't the case in this artificial setup here
// (where we created a responder exchange that's not responding to anything), we need to explicitly close it out. This is not
// expected in normal application logic.
//
exchange->Close();
}

void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext)
Expand All @@ -652,7 +686,8 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite
System::PacketBufferHandle commandPacket;

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));
auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

auto path = MakeTestCommandPath();

Expand All @@ -669,6 +704,14 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite
err = invokeResponseMessageParser.CheckSchemaValidity();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#endif

//
// Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an
// OnMessageReceived callback and not having sent a subsequent message. Since that isn't the case in this artificial setup here
// (where we created a responder exchange that's not responding to anything), we need to explicitly close it out. This is not
// expected in normal application logic.
//
exchange->Close();
}

// Command Sender sends invoke request, command handler drops invoke response, then test injects status response message with
Expand Down Expand Up @@ -1001,7 +1044,8 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTe
System::PacketBufferHandle commandPacket;

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));
auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

auto path = MakeTestCommandPath();

Expand All @@ -1022,6 +1066,14 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTe
err = invokeResponseMessageParser.CheckSchemaValidity();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#endif

//
// Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an
// OnMessageReceived callback and not having sent a subsequent message. Since that isn't the case in this artificial setup here
// (where we created a responder exchange that's not responding to anything), we need to explicitly close it out. This is not
// expected in normal application logic.
//
exchange->Close();
}

void TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext)
Expand Down Expand Up @@ -1060,7 +1112,8 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n
System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);

TestExchangeDelegate delegate;
commandHandler.mExchangeCtx.Grab(ctx.NewExchangeToAlice(&delegate));
auto exchange = ctx.NewExchangeToAlice(&delegate, false);
commandHandler.mExchangeCtx.Grab(exchange);

chip::isCommandDispatched = false;
GenerateInvokeRequest(apSuite, apContext, commandDatabuf, messageIsTimed, kTestCommandIdNoData);
Expand All @@ -1075,6 +1128,15 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n
NL_TEST_ASSERT(apSuite, status == Protocols::InteractionModel::Status::Success);
}
NL_TEST_ASSERT(apSuite, chip::isCommandDispatched == (messageIsTimed == transactionIsTimed));

//
// Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an
// OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest
// above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial
// setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it
// out. This is not expected in normal application logic.
//
exchange->Close();
}
}
}
Expand Down Expand Up @@ -1284,6 +1346,11 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommands(nlTestSuit
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
}

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
//
// This test needs a special unit-test only API being exposed in ExchangeContext to be able to correctly simulate
// the release of a session on the exchange.
//
void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
Expand All @@ -1303,10 +1370,14 @@ void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestS
// Verify that async command handle has been allocated
NL_TEST_ASSERT(apSuite, asyncCommandHandle.Get() != nullptr);

// Close the exchange associated with the handle and verify the handle can be gracefully released
asyncCommandHandle.Get()->mExchangeCtx->Close();
// Mimick closure of the exchange that would happen on a session release and verify that releasing the handle there-after
// is handled gracefully.
asyncCommandHandle.Get()->mExchangeCtx->GetSessionHolder().Release();
asyncCommandHandle.Get()->mExchangeCtx->OnSessionReleased();

asyncCommandHandle = nullptr;
}
#endif

} // namespace app
} // namespace chip
Expand All @@ -1332,7 +1403,10 @@ const nlTest sTests[] =
NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand),
NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg),
NL_TEST_DEF("TestCommandHandlerRejectMultipleCommands", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleCommands),

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
NL_TEST_DEF("TestCommandHandlerReleaseWithExchangeClosed", chip::app::TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed),
#endif

NL_TEST_DEF("TestCommandSenderCommandSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow),
NL_TEST_DEF("TestCommandSenderCommandAsyncSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow),
Expand Down
6 changes: 3 additions & 3 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

{
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr);
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false);
ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read);

GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/);
Expand Down Expand Up @@ -635,7 +635,7 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

{
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr);
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false);
ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read);

GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/);
Expand Down Expand Up @@ -1405,7 +1405,7 @@ void TestReadInteraction::TestProcessSubscribeRequest(nlTestSuite * apSuite, voi
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr);
Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false);

{
ReadHandler readHandler(*engine, exchangeCtx, chip::app::ReadHandler::InteractionType::Read);
Expand Down
78 changes: 48 additions & 30 deletions src/messaging/ExchangeContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,6 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp

bool isStandaloneAck =
(protocolId == Protocols::SecureChannel::Id) && msgType == to_underlying(Protocols::SecureChannel::MsgType::StandaloneAck);
if (!isStandaloneAck)
{
// If we were waiting for a message send, this is it. Standalone acks
// are not application-level sends, which is why we don't allow those to
// clear the WillSendMessage flag.
mFlags.Clear(Flags::kFlagWillSendMessage);
}

VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INTERNAL);
VerifyOrReturnError(mSession, CHIP_ERROR_CONNECTION_ABORTED);
Expand Down Expand Up @@ -208,19 +201,36 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
return CHIP_ERROR_MISSING_SECURE_SESSION;
}

// Create a new scope for `err`, to avoid shadowing warning previous `err`.
CHIP_ERROR err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get().Value(), mExchangeId,
IsInitiator(), GetReliableMessageContext(), reliableTransmissionRequested,
protocolId, msgType, std::move(msgBuf));
CHIP_ERROR err;

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
if (mInjectedFailures.Has(InjectedFailureType::kFailOnSend))
{
err = CHIP_ERROR_SENDING_BLOCKED;
}
else
{
#endif
err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get().Value(), mExchangeId, IsInitiator(),
GetReliableMessageContext(), reliableTransmissionRequested, protocolId, msgType,
std::move(msgBuf));
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
}
#endif

if (err != CHIP_NO_ERROR && IsResponseExpected())
{
CancelResponseTimer();
SetResponseExpected(false);
}

// Standalone acks are not application-level message sends.
if (err == CHIP_NO_ERROR && !isStandaloneAck)
if (!isStandaloneAck && err == CHIP_NO_ERROR)
{
//
// Once we've sent the message successfully, we can clear out the WillSendMessage flag.
//
mFlags.Clear(Flags::kFlagWillSendMessage);
MessageHandled();
}

Expand All @@ -230,6 +240,11 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp

void ExchangeContext::DoClose(bool clearRetransTable)
{
if (mFlags.Has(Flags::kFlagClosed))
{
return;
}

mFlags.Set(Flags::kFlagClosed);

// Clear protocol callbacks
Expand Down Expand Up @@ -336,7 +351,11 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons
ExchangeContext::~ExchangeContext()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0);
VerifyOrDie(!IsAckPending());

//
// Ensure that DoClose has been called by the time we get here. If not, we have a leak somewhere.
//
VerifyOrDie(mFlags.Has(Flags::kFlagClosed));

#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 @@ -398,28 +417,27 @@ void ExchangeContext::OnSessionReleased()
// decrease our refcount without worrying about use-after-free.
ExchangeHandle ref(*this);

if (IsResponseExpected())
//
// If a send is not expected (either because we're waiting for a response OR
// we're in the middle of processing a OnMessageReceived call), we can go ahead
// and notify our delegate and abort the exchange since we still own the ref.
//
if (!IsSendExpected())
{
// 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);
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 */);
}
}
Expand Down
17 changes: 17 additions & 0 deletions src/messaging/ExchangeContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,24 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
*/
bool IsSendExpected() const { return mFlags.Has(Flags::kFlagWillSendMessage); }

#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
SessionHolder & GetSessionHolder() { return mSession; }

enum class InjectedFailureType : uint8_t
{
kFailOnSend = 0x01
};

void InjectFailure(InjectedFailureType failureType) { mInjectedFailures.Set(failureType); }

void ClearInjectedFailures() { mInjectedFailures.ClearAll(); }
#endif

private:
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
BitFlags<InjectedFailureType> mInjectedFailures;
#endif

class ExchangeSessionHolder : public SessionHolderWithDelegate
{
public:
Expand Down
Loading

0 comments on commit 3731200

Please sign in to comment.