Skip to content

Commit

Permalink
Fix crash on fail-safe timeout due to closed exchange (#21590)
Browse files Browse the repository at this point in the history
* Fix crash on fail-safe timeout due to closed exchange

When CommandHandler is released it dereferences an exchange
holder to figure out if the status response can be sent.
The exchange holder, however, can be nulled out at this
point.

Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>

* Address review comment
  • Loading branch information
Damian-Nordic authored and pull[bot] committed Oct 20, 2023
1 parent 42df350 commit b214e43
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 4 deletions.
15 changes: 11 additions & 4 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,19 @@ void CommandHandler::DecrementHoldOff()
return;
}

if (!mExchangeCtx->IsGroupExchangeContext() && !mSentStatusResponse)
if (!mSentStatusResponse)
{
CHIP_ERROR err = SendCommandResponse();
if (err != CHIP_NO_ERROR)
if (!mExchangeCtx)
{
ChipLogProgress(DataManagement, "Skipping command response: exchange context is null");
}
else if (!mExchangeCtx->IsGroupExchangeContext())
{
ChipLogError(DataManagement, "Failed to send command response: %" CHIP_ERROR_FORMAT, err.Format());
CHIP_ERROR err = SendCommandResponse();
if (err != CHIP_NO_ERROR)
{
ChipLogError(DataManagement, "Failed to send command response: %" CHIP_ERROR_FORMAT, err.Format());
}
}
}

Expand Down
26 changes: 26 additions & 0 deletions src/app/tests/TestCommandInteraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ class TestCommandInteraction
static void TestCommandHandlerWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerRejectMultipleCommands(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext);

static void TestCommandSenderCommandSuccessResponseFlow(nlTestSuite * apSuite, void * apContext);
static void TestCommandSenderCommandAsyncSuccessResponseFlow(nlTestSuite * apSuite, void * apContext);
Expand Down Expand Up @@ -927,6 +928,30 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommands(nlTestSuit
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
}

void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
CHIP_ERROR err = CHIP_NO_ERROR;

app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());

AddInvokeRequestData(apSuite, apContext, &commandSender);
asyncCommandHandle = nullptr;
asyncCommand = true;
err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice());

NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();

// 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();
asyncCommandHandle = nullptr;
}

} // namespace app
} // namespace chip

Expand All @@ -948,6 +973,7 @@ 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),
NL_TEST_DEF("TestCommandHandlerReleaseWithExchangeClosed", chip::app::TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed),

NL_TEST_DEF("TestCommandSenderCommandSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow),
NL_TEST_DEF("TestCommandSenderCommandAsyncSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow),
Expand Down

0 comments on commit b214e43

Please sign in to comment.