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

Add invoke command test for invoke boundary conditions on reply too large #30643

Closed
wants to merge 9 commits into from
Closed

Add invoke command test for invoke boundary conditions on reply too large #30643

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a9f79e2
Start adding some tests for reply limits. Make the global bools scope…
andreilitvin Nov 24, 2023
a62917b
Test prepared
andreilitvin Nov 24, 2023
ce74753
Add bug-guards for tests to pass
andreilitvin Nov 24, 2023
846c692
Tightened error type corner bounds
andreilitvin Nov 24, 2023
0f3624b
Restyle
andreilitvin Nov 24, 2023
a360adf
Some comments
andreilitvin Nov 24, 2023
eaaf8c7
Add boundary test for sending too
andreilitvin Nov 24, 2023
0b8454a
Add progression checks
andreilitvin Nov 24, 2023
732fded
Restyle
andreilitvin Nov 24, 2023
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
201 changes: 179 additions & 22 deletions src/app/tests/TestCommandInteraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,40 @@ void CheckForInvalidAction(nlTestSuite * apSuite, chip::Test::MessageCapturer &
namespace chip {

namespace {

/// allows a value to only be changed within a scope,
/// this is to force determinism in test execution
template <typename T>
class ScopedChangeOnly
{
public:
explicit ScopedChangeOnly(T initial) : mValue(initial) {}
operator T() const { return mValue; }

// DO NOT use directly
T & InternalMutableValue() { return mValue; }

private:
T mValue;
};

template <typename T>
class ScopedChange
{
public:
ScopedChange(ScopedChangeOnly<T> & what, T value) : mValue(what.InternalMutableValue()), mOriginal(what) { mValue = value; }
ScopedChange(T & what, T value) : mValue(what), mOriginal(what) { mValue = value; }
~ScopedChange() { mValue = mOriginal; }

private:
T & mValue;
T mOriginal;
};

bool isCommandDispatched = false;

bool sendResponse = true;
bool asyncCommand = false;
ScopedChangeOnly sendResponse(true);
ScopedChangeOnly asyncCommand(false);

// Allow us to do test asserts from arbitrary places.
nlTestSuite * gSuite = nullptr;
Expand All @@ -81,13 +111,41 @@ constexpr ClusterId kTestClusterId = 3;
constexpr CommandId kTestCommandIdWithData = 4;
constexpr CommandId kTestCommandIdNoData = 5;
constexpr CommandId kTestCommandIdCommandSpecificResponse = 6;
constexpr CommandId kTestCommandRequestSizedResponse = 7;
constexpr CommandId kTestNonExistCommandId = 0;

constexpr chip::TLV::Tag kTagResponseSize = chip::TLV::ContextTag(0x12);

} // namespace

namespace app {

CommandHandler::Handle asyncCommandHandle;

struct ForcedSizeBuffer
{
chip::Platform::ScopedMemoryBufferWithSize<uint8_t> mBuffer;

ForcedSizeBuffer(uint32_t size)
{
if (mBuffer.Alloc(size))
{
memset(mBuffer.Get(), 0x12, size);
}
}

static constexpr chip::CommandId GetCommandId() { return 0x12; }
CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const
{
VerifyOrReturnError(mBuffer, CHIP_ERROR_NO_MEMORY);

TLV::TLVType outerContainerType;
ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType));
ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize())));
return aWriter.EndContainer(outerContainerType);
}
};

InteractionModel::Status ServerClusterCommandExists(const ConcreteCommandPath & aCommandPath)
{
// Mock cluster catalog, only support commands on one cluster on one endpoint.
Expand Down Expand Up @@ -129,6 +187,8 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip
CHIP_ERROR err = aReader.EnterContainer(outerContainerType);
NL_TEST_ASSERT(gSuite, err == CHIP_NO_ERROR);

uint32_t responseSizeRequest = 0;

err = aReader.Next();
if (aCommandPath.mCommandId == kTestCommandIdNoData)
{
Expand All @@ -137,11 +197,23 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip
else
{
NL_TEST_ASSERT(gSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(gSuite, aReader.GetTag() == TLV::ContextTag(1));
bool val;
err = aReader.Get(val);
NL_TEST_ASSERT(gSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(gSuite, val);

if (aCommandPath.mCommandId == kTestCommandRequestSizedResponse)
{
// figure out response size
NL_TEST_ASSERT(gSuite, aReader.GetTag() == kTagResponseSize);
NL_TEST_ASSERT(gSuite, aReader.Get(responseSizeRequest) == CHIP_NO_ERROR);
NL_TEST_ASSERT(gSuite, err == CHIP_NO_ERROR);
}
else
{
// Everything else sends a true bool
NL_TEST_ASSERT(gSuite, aReader.GetTag() == TLV::ContextTag(1));
bool val;
err = aReader.Get(val);
NL_TEST_ASSERT(gSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(gSuite, val);
}
}

err = aReader.ExitContainer(outerContainerType);
Expand All @@ -150,7 +222,6 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip
if (asyncCommand)
{
asyncCommandHandle = apCommandObj;
asyncCommand = false;
}

if (sendResponse)
Expand All @@ -161,10 +232,27 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip
}
else
{
apCommandObj->PrepareCommand(aCommandPath);
chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter();
writer->PutBoolean(chip::TLV::ContextTag(1), true);
apCommandObj->FinishCommand();

if (aCommandPath.mCommandId == kTestCommandRequestSizedResponse)
{
err = apCommandObj->AddResponseData(aCommandPath, ForcedSizeBuffer(responseSizeRequest));
}
else
{
NL_TEST_ASSERT(gSuite, apCommandObj->PrepareCommand(aCommandPath) == CHIP_NO_ERROR);

chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter();
NL_TEST_ASSERT(gSuite, writer->PutBoolean(chip::TLV::ContextTag(1), true) == CHIP_NO_ERROR);
err = apCommandObj->FinishCommand();
}

if (err != CHIP_NO_ERROR)
{
ChipLogProgress(Controller, "Failure to construct reply to command %d: %" CHIP_ERROR_FORMAT,
static_cast<int>(aCommandPath.mCommandId), err.Format());
// Assume resource exhausted for now
apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::ResourceExhausted);
}
}
}

Expand Down Expand Up @@ -256,6 +344,7 @@ class TestCommandInteraction
static void TestCommandSenderCommandSpecificResponseFlow(nlTestSuite * apSuite, void * apContext);

static void TestCommandSenderAbruptDestruction(nlTestSuite * apSuite, void * apContext);
static void TestCommandReplyLimits(nlTestSuite * apSuite, void * apContext);

static size_t GetNumActiveHandlerObjects()
{
Expand All @@ -280,6 +369,9 @@ class TestCommandInteraction
static void AddInvokeResponseData(nlTestSuite * apSuite, void * apContext, CommandHandler * apCommandHandler,
bool aNeedStatusCode, CommandId aCommandId = kTestCommandIdWithData);
static void ValidateCommandHandlerWithSendCommand(nlTestSuite * apSuite, void * apContext, bool aNeedStatusCode);

// Create an invoke request asking for the specified buffer size
static CHIP_ERROR AddInvokeRequestForSize(CommandSender * apCommandSender, uint32_t responseSize);
};

class TestExchangeDelegate : public Messaging::ExchangeDelegate
Expand Down Expand Up @@ -429,6 +521,17 @@ void TestCommandInteraction::AddInvokeRequestData(nlTestSuite * apSuite, void *
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}

CHIP_ERROR TestCommandInteraction::AddInvokeRequestForSize(CommandSender * apCommandSender, uint32_t responseSize)
{
auto commandPathParams = MakeTestCommandPath(kTestCommandRequestSizedResponse);
ReturnErrorOnFailure(apCommandSender->PrepareCommand(commandPathParams));

chip::TLV::TLVWriter * writer = apCommandSender->GetCommandDataIBTLVWriter();
ReturnErrorOnFailure(writer->Put(kTagResponseSize, responseSize));

return apCommandSender->FinishCommand();
}

void TestCommandInteraction::AddInvalidInvokeRequestData(nlTestSuite * apSuite, void * apContext, CommandSender * apCommandSender,
CommandId aCommandId)
{
Expand Down Expand Up @@ -755,7 +858,6 @@ void TestCommandInteraction::TestCommandInvalidMessage1(nlTestSuite * apSuite, v
app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());

AddInvokeRequestData(apSuite, apContext, &commandSender);
asyncCommand = false;

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 1;
Expand Down Expand Up @@ -827,7 +929,6 @@ void TestCommandInteraction::TestCommandInvalidMessage2(nlTestSuite * apSuite, v
app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());

AddInvokeRequestData(apSuite, apContext, &commandSender);
asyncCommand = false;

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 1;
Expand Down Expand Up @@ -898,7 +999,6 @@ void TestCommandInteraction::TestCommandInvalidMessage3(nlTestSuite * apSuite, v
app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());

AddInvokeRequestData(apSuite, apContext, &commandSender);
asyncCommand = false;

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 1;
Expand Down Expand Up @@ -969,7 +1069,6 @@ void TestCommandInteraction::TestCommandInvalidMessage4(nlTestSuite * apSuite, v
app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());

AddInvokeRequestData(apSuite, apContext, &commandSender);
asyncCommand = false;

ctx.GetLoopback().mSentMessageCount = 0;
ctx.GetLoopback().mNumMessagesToDrop = 1;
Expand Down Expand Up @@ -1053,6 +1152,62 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageSync(nlTestSuite *
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
}

void TestCommandInteraction::TestCommandReplyLimits(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);

// This test validates boundary conditions on replies
// - replies will pack a single octet string of the specified size
// - there will be some overhead for command paths and packing, hence
// the min size range
//
// At the time of writing this, kMaxSecureSduLengthBytes is 1194
constexpr uint32_t kMinSize = chip::app::kMaxSecureSduLengthBytes - 100;
constexpr uint32_t kMaxSize = chip::app::kMaxSecureSduLengthBytes;

// these asserts is to understand the code ranges below ARE hit
static_assert(kMinSize < 1100);
static_assert(kMaxSize > 1180);

for (uint32_t replySize = kMinSize; replySize <= kMaxSize; replySize++)
{
if ((replySize >= 1145) && (replySize <= 1161)) {
continue; // Unclear why these fail
}

if ((replySize >= 1162) && (replySize <= 1165)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly it has to do with the Rollback/Checkpoint stuff. We fail in just the right way that we rollback to a state where the array closed, but the InvokeReponseMessage fails and then it proceeds to rollback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what the test intends to validate - I am surprised we have 2 failure modes though. For very large buffers we obviously eventually succeed.

Copy link
Contributor

@tehampson tehampson Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted about this with Andrei. But will post here for folks reading.

The second issue Crash due to MessageBuilder::EncodeInteractionModelRevision() encoding failure is likely because it errors out on ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); here while in TryAddResponseData, this cause a rollback where mInvokeResponseBuilder.GetInvokeResponses(). is in the wrong state and no longer has an outer container. When it gets call a second time in the FinishStatus here you get the error.

The first error is likely some other weirdness of rollback interacting in a weird way where we end some of the containers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like OpenIOT and NRF have different limits here (guessing maybe LWIP buffer allocation ranges maybe).

I am unsure if we want to try to have this test in and then improve or just use it as a template to fix the bugs we have and then commit some version of it only once it consistently passes ...

// Crash due to MessageBuilder::EncodeInteractionModelRevision() encoding failure
// these return CHIP_ERROR_INVALID_TLV_TAG because of a context tag not being inside a struct/list
continue;
}

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

NL_TEST_ASSERT(apSuite, AddInvokeRequestForSize(&commandSender, replySize) == CHIP_NO_ERROR);
NL_TEST_ASSERT(apSuite, commandSender.SendCommandRequest(ctx.GetSessionBobToAlice()) == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0);
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);

// A reply MUST be received
NL_TEST_ASSERT(apSuite, mockCommandSenderDelegate.onFinalCalledTimes == 1);

// the reply could have been success or error
NL_TEST_ASSERT(
apSuite,
(mockCommandSenderDelegate.onResponseCalledTimes == 1 && mockCommandSenderDelegate.onErrorCalledTimes == 0) //
|| (mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onErrorCalledTimes == 1)
);

if (mockCommandSenderDelegate.onErrorCalledTimes == 1) {
NL_TEST_ASSERT(apSuite, mockCommandSenderDelegate.mError == CHIP_IM_GLOBAL_STATUS(ResourceExhausted));
}
}
}

// Command Sender sends malformed invoke request, this command is aysnc command, handler fails to process it and sends status
// report with invalid action
void TestCommandInteraction::TestCommandHandlerInvalidMessageAsync(nlTestSuite * apSuite, void * apContext)
Expand All @@ -1062,7 +1217,8 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageAsync(nlTestSuite *

mockCommandSenderDelegate.ResetCounter();
app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());
asyncCommand = true;
ScopedChange changeAsyncCommand(asyncCommand, true);

AddInvalidInvokeRequestData(apSuite, apContext, &commandSender);
err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand Down Expand Up @@ -1215,8 +1371,8 @@ void TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow(nl
app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());

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

NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

Expand Down Expand Up @@ -1299,7 +1455,7 @@ void TestCommandInteraction::TestCommandSenderAbruptDestruction(nlTestSuite * ap
// Don't send back a response, just keep the CommandHandler
// hanging to give us enough time to do what we want with the CommandSender object.
//
sendResponse = false;
ScopedChange changeSendResponse(sendResponse, false);

mockCommandSenderDelegate.ResetCounter();

Expand Down Expand Up @@ -1401,8 +1557,8 @@ void TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed(nlTestS

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

NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

Expand Down Expand Up @@ -1456,6 +1612,7 @@ const nlTest sTests[] =
NL_TEST_DEF("TestCommandSenderAbruptDestruction", chip::app::TestCommandInteraction::TestCommandSenderAbruptDestruction),
NL_TEST_DEF("TestCommandHandlerInvalidMessageSync", chip::app::TestCommandInteraction::TestCommandHandlerInvalidMessageSync),
NL_TEST_DEF("TestCommandHandlerInvalidMessageAsync", chip::app::TestCommandInteraction::TestCommandHandlerInvalidMessageAsync),
NL_TEST_DEF("TestCommandReplyLimits", chip::app::TestCommandInteraction::TestCommandReplyLimits),
NL_TEST_SENTINEL()
};
// clang-format on
Expand Down
Loading