From 77144c7d150dde56c03145895e366edd5925f1c2 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Mon, 15 Aug 2022 16:18:50 -0700 Subject: [PATCH] Improve Invalid Action return in CommandHandler (#21868) * Improve Invalid Action return in CommandHandler Improve the error reporting from ProcessGroupCommandDataIB and ProcessCommandDataIB: per spec some of their failures should also be InvalidAction. * address comments --- src/app/CommandHandler.cpp | 53 ++++++++++++++++++++------------------ src/app/CommandHandler.h | 4 +-- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 12c61a643debb1..7e742b54cf8aff 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -127,14 +127,18 @@ 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 +242,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 +252,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 +270,7 @@ 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); + return AddStatus(concretePath, commandExists) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -281,10 +285,10 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { if (err != CHIP_ERROR_ACCESS_DENIED) { - return AddStatus(concretePath, Status::Failure); + 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 - return AddStatus(concretePath, Status::UnsupportedAccess); + return AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -292,7 +296,7 @@ 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); + return AddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId)) @@ -303,7 +307,7 @@ 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); + return AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -328,15 +332,15 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand exit: if (err != CHIP_NO_ERROR) { - return AddStatus(concretePath, Status::InvalidCommand); + 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 // 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 +355,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 +377,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 +389,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 +397,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 +449,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);