Skip to content

Commit

Permalink
Fix
Browse files Browse the repository at this point in the history
1. Once message is passed to OnReadInitialRequest in read handler, if
   anything bad in incoming message, handler would send status report
   with invalid action, if there is not enough resource for paths in IM
   engines, we would send status report with exhansted path.
2. Fix fake build in linux,  it seems this build don't have IM schema check.
Usually without CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK, if schema error
happens, we would get a little bit more detailed error, here, we add
additional error check in TestReadHandlerInvalidAttributePath.
  • Loading branch information
yunhanw-google committed Aug 8, 2022
1 parent c098a67 commit 2493526
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 22 deletions.
8 changes: 2 additions & 6 deletions src/app/InteractionModelEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -458,15 +458,11 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
return Status::ResourceExhausted;
}

CHIP_ERROR err = handler->OnInitialRequest(std::move(aPayload));
if (err == CHIP_ERROR_NO_MEMORY)
{
return Status::ResourceExhausted;
}
handler->OnInitialRequest(std::move(aPayload));

// TODO: Should probably map various TLV errors into InvalidAction, here
// or inside the read handler.
return StatusIB(err).mStatus;
return Status::Success;
}

Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext,
Expand Down
19 changes: 8 additions & 11 deletions src/app/ReadHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ void ReadHandler::Close()
mManagementCallback.OnDone(*this);
}

CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
void ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
{
CHIP_ERROR err = CHIP_NO_ERROR;
System::PacketBufferHandle response;
Expand All @@ -104,15 +104,19 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)

if (err != CHIP_NO_ERROR)
{
Status status = Status::InvalidAction;
if (err == CHIP_IM_GLOBAL_STATUS(PathsExhausted))
{
status = Status::PathsExhausted;
}
StatusResponse::Send(status, mExchangeCtx.Get(), /* aExpectResponse = */ false);
Close();
}
else
{
// Force us to be in a dirty state so we get processed by the reporting
mFlags.Set(ReadHandlerFlags::ForceDirty);
}

return err;
}

CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload,
Expand Down Expand Up @@ -301,14 +305,7 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa

ReturnErrorOnFailure(readRequestParser.Init(reader));
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
err = readRequestParser.CheckSchemaValidity();
if (err != CHIP_NO_ERROR)
{
// The actual error we want to return to our consumer is an IM "invalid
// action" error, not whatever internal error CheckSchemaValidity
// happens to come up with.
return CHIP_IM_GLOBAL_STATUS(InvalidAction);
}
ReturnErrorOnFailure(readRequestParser.CheckSchemaValidity());
#endif

err = readRequestParser.GetAttributeRequests(&attributePathListParser);
Expand Down
2 changes: 1 addition & 1 deletion src/app/ReadHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
* @retval #CHIP_NO_ERROR On success.
*
*/
CHIP_ERROR OnInitialRequest(System::PacketBufferHandle && aPayload);
void OnInitialRequest(System::PacketBufferHandle && aPayload);

/**
* Send ReportData to initiator
Expand Down
17 changes: 13 additions & 4 deletions src/app/tests/TestReadInteraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex
err = writer.Finalize(&readRequestbuf);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

err = readHandler.OnInitialRequest(std::move(readRequestbuf));
err = readHandler.ProcessReadRequest(std::move(readRequestbuf));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}

Expand Down Expand Up @@ -659,11 +659,16 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu
err = writer.Finalize(&readRequestbuf);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

err = readHandler.OnInitialRequest(std::move(readRequestbuf));
NL_TEST_ASSERT(apSuite, err == CHIP_IM_GLOBAL_STATUS(InvalidAction));
err = readHandler.ProcessReadRequest(std::move(readRequestbuf));
ChipLogError(DataManagement, "The error is %" CHIP_ERROR_FORMAT, err.Format());
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB);
#else
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_END_OF_TLV);
#endif // CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK

//
// In the call above to OnInitialRequest, the handler will not actually close out the EC since
// In the call above to ProcessReadRequest, the handler will not actually close out the EC since
// it expects the ExchangeManager to do so automatically given it's not calling WillSend() on the EC,
// and is not sending a response back.
//
Expand Down Expand Up @@ -3449,7 +3454,11 @@ void TestReadInteraction::TestReadHandlerMalformedReadRequest2(nlTestSuite * apS
Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
NL_TEST_ASSERT(apSuite, delegate.mError == CHIP_IM_GLOBAL_STATUS(InvalidAction));
#else
NL_TEST_ASSERT(apSuite, delegate.mError == CHIP_IM_GLOBAL_STATUS(InvalidAction));
#endif
}
NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
engine->Shutdown();
Expand Down

0 comments on commit 2493526

Please sign in to comment.