From 9168a56e3f8a6a0ca4ac8644b7c4871a63c8c3ea Mon Sep 17 00:00:00 2001 From: yunhanw Date: Fri, 12 Aug 2022 12:21:45 -0700 Subject: [PATCH 1/2] Improve Invalid Action return in CommandHandler Improve the error reporting from ProcessGroupCommandDataIB and ProcessCommandDataIB: per spec some of their failures should also be InvalidAction. --- src/app/CommandHandler.cpp | 78 ++++++++++++++++++++++++++------------ src/app/CommandHandler.h | 4 +- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 12c61a643debb1..6fe2d299ef61e3 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -127,14 +127,19 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa VerifyOrReturnError(TLV::AnonymousTag() == invokeRequestsReader.GetTag(), Status::InvalidAction); CommandDataIB::Parser commandData; VerifyOrReturnError(commandData.Init(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction); - + Status status = Status::Success; if (mExchangeCtx->IsGroupExchangeContext()) { - VerifyOrReturnError(ProcessGroupCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure); + status = ProcessGroupCommandDataIB(commandData); } else { - VerifyOrReturnError(ProcessCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure); + status = ProcessCommandDataIB(commandData); + ; + } + if (status != Status::Success) + { + return status; } } @@ -238,7 +243,7 @@ constexpr uint8_t sNoFields[] = { }; } // anonymous namespace -CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement) +Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement) { CHIP_ERROR err = CHIP_NO_ERROR; CommandPathIB::Parser commandPath; @@ -248,16 +253,16 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand // NOTE: errors may occur before the concrete command path is even fully decoded. err = aCommandElement.GetPath(&commandPath); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); err = commandPath.GetClusterId(&concretePath.mClusterId); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); err = commandPath.GetCommandId(&concretePath.mCommandId); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); err = commandPath.GetEndpointId(&concretePath.mEndpointId); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); { Status commandExists = mpCallback->CommandExists(concretePath); @@ -266,7 +271,11 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", ChipLogValueMEI(concretePath.mCommandId), ChipLogValueMEI(concretePath.mClusterId), concretePath.mEndpointId); - return AddStatus(concretePath, commandExists); + if (AddStatus(concretePath, commandExists) != CHIP_NO_ERROR) + { + return Status::Failure; + } + return Status::Success; } } @@ -281,10 +290,18 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { if (err != CHIP_ERROR_ACCESS_DENIED) { - return AddStatus(concretePath, Status::Failure); + if (AddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR) + { + return Status::Failure; + } + return Status::Success; } // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status - return AddStatus(concretePath, Status::UnsupportedAccess); + if (AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR) + { + return Status::Failure; + } + return Status::Success; } } @@ -292,7 +309,11 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { // TODO: when wildcard invokes are supported, discard a // wildcard-expanded path instead of returning a status. - return AddStatus(concretePath, Status::NeedsTimedInteraction); + if (AddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR) + { + return Status::Failure; + } + return Status::Success; } if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId)) @@ -303,7 +324,11 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { // TODO: when wildcard invokes are supported, discard a // wildcard-expanded path instead of returning a status. - return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess); + if (AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR) + { + return Status::Failure; + } + return Status::Success; } } @@ -328,15 +353,19 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand exit: if (err != CHIP_NO_ERROR) { - return AddStatus(concretePath, Status::InvalidCommand); + if (AddStatus(concretePath, Status::InvalidCommand) != CHIP_NO_ERROR) + { + return Status::Failure; + } + return Status::Success; } // We have handled the error status above and put the error status in response, now return success status so we can process // other commands in the invoke request. - return CHIP_NO_ERROR; + return Status::Success; } -CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement) +Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement) { CHIP_ERROR err = CHIP_NO_ERROR; CommandPathIB::Parser commandPath; @@ -351,13 +380,13 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo Credentials::GroupDataProvider::EndpointIterator * iterator; err = aCommandElement.GetPath(&commandPath); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); err = commandPath.GetClusterId(&clusterId); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); err = commandPath.GetCommandId(&commandId); - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); groupId = mExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId(); fabric = GetAccessingFabricIndex(); @@ -373,8 +402,9 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo ChipLogValueMEI(clusterId), ChipLogValueMEI(commandId)); commandDataReader.Init(sNoFields); err = commandDataReader.Next(); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); } - SuccessOrExit(err); + VerifyOrReturnError(err == CHIP_NO_ERROR, Status::Failure); // Per spec, we do the "is this a timed command?" check for every path, but // since all paths that fail it just get silently discarded we can do it @@ -384,7 +414,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo if (CommandNeedsTimedInvoke(clusterId, commandId)) { // Group commands are never timed. - ExitNow(); + return Status::Success; } // No check for `CommandIsFabricScoped` unlike in `ProcessCommandDataIB()` since group commands @@ -392,7 +422,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo // Find which endpoints can process the command, and dispatch to them. iterator = groupDataProvider->IterateEndpoints(fabric); - VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError(iterator != nullptr, Status::Failure); while (iterator->Next(mapping)) { @@ -444,9 +474,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo } } iterator->Release(); - -exit: - return CHIP_NO_ERROR; + return Status::Success; } CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 18ffed48e9ec1a..bb96fe486f9f44 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -350,13 +350,13 @@ class CommandHandler : public Messaging::ExchangeDelegate * ProcessCommandDataIB is only called when a unicast invoke command request is received * It requires the endpointId in its command path to be able to dispatch the command */ - CHIP_ERROR ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement); + Protocols::InteractionModel::Status ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement); /** * ProcessGroupCommandDataIB is only called when a group invoke command request is received * It doesn't need the endpointId in it's command path since it uses the GroupId in message metadata to find it */ - CHIP_ERROR ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement); + Protocols::InteractionModel::Status ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement); CHIP_ERROR SendCommandResponse(); CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus); From 53cd8c2d3df4e1f8dffc50ad30d7a57bd6896b8b Mon Sep 17 00:00:00 2001 From: yunhanw Date: Fri, 12 Aug 2022 15:33:10 -0700 Subject: [PATCH 2/2] address comments --- src/app/CommandHandler.cpp | 37 ++++++------------------------------- 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 6fe2d299ef61e3..7e742b54cf8aff 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -135,7 +135,6 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa else { status = ProcessCommandDataIB(commandData); - ; } if (status != Status::Success) { @@ -271,11 +270,7 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", ChipLogValueMEI(concretePath.mCommandId), ChipLogValueMEI(concretePath.mClusterId), concretePath.mEndpointId); - if (AddStatus(concretePath, commandExists) != CHIP_NO_ERROR) - { - return Status::Failure; - } - return Status::Success; + return AddStatus(concretePath, commandExists) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -290,18 +285,10 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem { if (err != CHIP_ERROR_ACCESS_DENIED) { - if (AddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR) - { - return Status::Failure; - } - return Status::Success; + return AddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status - if (AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR) - { - return Status::Failure; - } - return Status::Success; + return AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -309,11 +296,7 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem { // TODO: when wildcard invokes are supported, discard a // wildcard-expanded path instead of returning a status. - if (AddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR) - { - return Status::Failure; - } - return Status::Success; + return AddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId)) @@ -324,11 +307,7 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem { // TODO: when wildcard invokes are supported, discard a // wildcard-expanded path instead of returning a status. - if (AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR) - { - return Status::Failure; - } - return Status::Success; + return AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -353,11 +332,7 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem exit: if (err != CHIP_NO_ERROR) { - if (AddStatus(concretePath, Status::InvalidCommand) != CHIP_NO_ERROR) - { - return Status::Failure; - } - return Status::Success; + return AddStatus(concretePath, Status::InvalidCommand) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } // We have handled the error status above and put the error status in response, now return success status so we can process