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

Fix ExchangeContext leaks #21846

Merged
merged 8 commits into from
Aug 29, 2022
Merged

Fix ExchangeContext leaks #21846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5fd3b71
ExchangeHolder + ExchangeContext: Management Fixes
mrjerryjohns Aug 12, 2022
4a2660c
Update src/messaging/ExchangeHolder.h
woody-apple Aug 16, 2022
a33f94e
Review feedback
mrjerryjohns Aug 26, 2022
66f534c
Merge branch 'master' into fix-exchange-closing
mrjerryjohns Aug 26, 2022
d4bb284
Minor fix to TestExchangeHolder
mrjerryjohns Aug 26, 2022
b6bbca4
Made the use of the test-only ExchangeContext::GetSessionHolder API c…
mrjerryjohns Aug 26, 2022
4146763
Merge branch 'master' into fix-exchange-closing
mrjerryjohns Aug 26, 2022
1c72c8e
Review feedback
mrjerryjohns Aug 27, 2022
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
92 changes: 83 additions & 9 deletions src/app/tests/TestCommandInteraction.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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