From 3270365d6924bdedee458122004c59686ec013b2 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Mon, 15 Nov 2021 10:17:01 -0800 Subject: [PATCH] add check to make sure minInterval is less than maxInterval for subscribe request (#11771) --- src/app/ReadClient.cpp | 7 +++-- src/app/ReadHandler.cpp | 29 ++++++++++---------- src/app/tests/TestReadInteraction.cpp | 38 +++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index c57c3417331ec4..613057d62908d9 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -624,6 +624,8 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara SuccessOrExit(err); } + VerifyOrExit(aReadPrepareParams.mMinIntervalFloorSeconds < aReadPrepareParams.mMaxIntervalCeilingSeconds, + err = CHIP_ERROR_INVALID_ARGUMENT); request.MinIntervalSeconds(aReadPrepareParams.mMinIntervalFloorSeconds) .MaxIntervalSeconds(aReadPrepareParams.mMaxIntervalCeilingSeconds) .KeepSubscriptions(aReadPrepareParams.mKeepSubscriptions) @@ -648,10 +650,7 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara exit: if (err != CHIP_NO_ERROR) { - AbortExistingExchangeContext(); - } - if (err != CHIP_NO_ERROR) - { + ChipLogError(DataManagement, "Failed to send subscribe request: %" CHIP_ERROR_FORMAT, err.Format()); Shutdown(); } return err; diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 506f6a8e28eeb1..50ad988555c240 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -506,17 +506,17 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse() writer.Init(std::move(packet)); SubscribeResponseMessage::Builder response; - ReturnLogErrorOnFailure(response.Init(&writer)); + ReturnErrorOnFailure(response.Init(&writer)); response.SubscriptionId(mSubscriptionId) .MinIntervalFloorSeconds(mMinIntervalFloorSeconds) .MaxIntervalCeilingSeconds(mMaxIntervalCeilingSeconds) .EndOfSubscribeResponseMessage(); - ReturnLogErrorOnFailure(response.GetError()); + ReturnErrorOnFailure(response.GetError()); - ReturnLogErrorOnFailure(writer.Finalize(&packet)); + ReturnErrorOnFailure(writer.Finalize(&packet)); VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnLogErrorOnFailure(RefreshSubscribeSyncTimer()); + ReturnErrorOnFailure(RefreshSubscribeSyncTimer()); mInitialReport = false; MoveToState(HandlerState::GeneratingReports); if (mpDelegate != nullptr) @@ -531,11 +531,11 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP System::PacketBufferTLVReader reader; reader.Init(std::move(aPayload)); - ReturnLogErrorOnFailure(reader.Next()); + ReturnErrorOnFailure(reader.Next()); SubscribeRequestMessage::Parser subscribeRequestParser; - ReturnLogErrorOnFailure(subscribeRequestParser.Init(reader)); + ReturnErrorOnFailure(subscribeRequestParser.Init(reader)); #if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK - ReturnLogErrorOnFailure(subscribeRequestParser.CheckSchemaValidity()); + ReturnErrorOnFailure(subscribeRequestParser.CheckSchemaValidity()); #endif AttributePathIBs::Parser attributePathListParser; @@ -546,9 +546,9 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP } else if (err == CHIP_NO_ERROR) { - ReturnLogErrorOnFailure(ProcessAttributePathList(attributePathListParser)); + ReturnErrorOnFailure(ProcessAttributePathList(attributePathListParser)); } - ReturnLogErrorOnFailure(err); + ReturnErrorOnFailure(err); EventPaths::Parser eventPathListParser; err = subscribeRequestParser.GetEventPaths(&eventPathListParser); @@ -558,13 +558,14 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP } else if (err == CHIP_NO_ERROR) { - ReturnLogErrorOnFailure(ProcessEventPaths(eventPathListParser)); + ReturnErrorOnFailure(ProcessEventPaths(eventPathListParser)); } - ReturnLogErrorOnFailure(err); + ReturnErrorOnFailure(err); - ReturnLogErrorOnFailure(subscribeRequestParser.GetMinIntervalSeconds(&mMinIntervalFloorSeconds)); - ReturnLogErrorOnFailure(subscribeRequestParser.GetMaxIntervalSeconds(&mMaxIntervalCeilingSeconds)); - ReturnLogErrorOnFailure(Crypto::DRBG_get_bytes(reinterpret_cast(&mSubscriptionId), sizeof(mSubscriptionId))); + ReturnErrorOnFailure(subscribeRequestParser.GetMinIntervalSeconds(&mMinIntervalFloorSeconds)); + ReturnErrorOnFailure(subscribeRequestParser.GetMaxIntervalSeconds(&mMaxIntervalCeilingSeconds)); + VerifyOrReturnError(mMinIntervalFloorSeconds < mMaxIntervalCeilingSeconds, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(Crypto::DRBG_get_bytes(reinterpret_cast(&mSubscriptionId), sizeof(mSubscriptionId))); MoveToState(HandlerState::GeneratingReports); diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index b5079798c3d6c6..64af7373c589c1 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -276,6 +276,7 @@ class TestReadInteraction static void TestSubscribeEarlyShutdown(nlTestSuite * apSuite, void * apContext); static void TestSubscribeInvalidAttributePathRoundtrip(nlTestSuite * apSuite, void * apContext); static void TestReadInvalidAttributePathRoundtrip(nlTestSuite * apSuite, void * apContext); + static void TestSubscribeInvalidIterval(nlTestSuite * apSuite, void * apContext); private: static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, @@ -1111,6 +1112,42 @@ void TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip(nlTestSuite engine->Shutdown(); } +void TestReadInteraction::TestSubscribeInvalidIterval(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + // Shouldn't have anything in the retransmit table when starting the test. + NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + + GenerateEvents(apSuite, apContext); + + MockInteractionModelApp delegate; + auto * engine = chip::app::InteractionModelEngine::GetInstance(); + err = engine->Init(&ctx.GetExchangeManager(), &delegate); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse); + + ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice()); + chip::app::AttributePathParams attributePathParams[1]; + readPrepareParams.mpAttributePathParamsList = attributePathParams; + readPrepareParams.mpAttributePathParamsList[0].mEndpointId = kTestEndpointId; + readPrepareParams.mpAttributePathParamsList[0].mClusterId = kTestClusterId; + readPrepareParams.mpAttributePathParamsList[0].mAttributeId = 1; + + readPrepareParams.mAttributePathParamsListSize = 1; + + readPrepareParams.mSessionHandle = ctx.GetSessionBobToAlice(); + readPrepareParams.mMinIntervalFloorSeconds = 5; + readPrepareParams.mMaxIntervalCeilingSeconds = 5; + printf("\nSend subscribe request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId); + + err = chip::app::InteractionModelEngine::GetInstance()->SendSubscribeRequest(readPrepareParams, &delegate); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + engine->Shutdown(); +} + } // namespace app } // namespace chip @@ -1138,6 +1175,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestSubscribeEarlyShutdown", chip::app::TestReadInteraction::TestSubscribeEarlyShutdown), NL_TEST_DEF("TestSubscribeInvalidAttributePathRoundtrip", chip::app::TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip), NL_TEST_DEF("TestReadInvalidAttributePathRoundtrip", chip::app::TestReadInteraction::TestReadInvalidAttributePathRoundtrip), + NL_TEST_DEF("TestSubscribeInvalidIterval", chip::app::TestReadInteraction::TestSubscribeInvalidIterval), NL_TEST_SENTINEL() }; // clang-format on