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.

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

CommandHandler does not send required status responses in many cases #8030

Closed
bzbarsky-apple opened this issue Jun 30, 2021 · 2 comments
Closed
Assignees
Labels
Interaction Model Work p1 priority 1 work spec Mismatch between spec and implementation V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

CommandHandler::ProcessCommandDataElement will send a status response in failure cases.

But if we don't reach that function, we will fail to send spec-required status responses. Specifically, if Command::ProcessCommandMessage errors out before calling ProcessCommandDataElement (which it can do if the command list is missing in the TLV or has the wrong tag), we will never reach ProcessCommandDataElement.

Proposed Solution

Add tests and fix. Perhaps hoist the status-response-sending to Command::ProcessCommandMessage?

@yunhanw-google yunhanw-google self-assigned this Jun 30, 2021
@yunhanw-google yunhanw-google added V1.0 and removed V1.X labels Jun 9, 2022
@bzbarsky-apple bzbarsky-apple added the spec Mismatch between spec and implementation label Jun 9, 2022
@bzbarsky-apple
Copy link
Contributor Author

So at this point the logic flow is:

  1. InteractionModelEngine::OnMessageReceived will send a StatusResponse with Failure (not the right error code for our purposes here) if it calls something that fails.
  2. It calls OnInvokeCommandRequest.
  3. That calls CommandHandler::OnInvokeCommandRequest.
  4. That calls CommandHandler::ProcessInvokeRequest.

We should either propagate the status bits down into all that or something.

Note also a similar TODO comment at the end of InteractionModelEngine::OnReadInitialRequest.

@yunhanw-google
Copy link
Contributor

#21557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interaction Model Work p1 priority 1 work spec Mismatch between spec and implementation V1.0
Projects
None yet
4 participants