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

Sort out where AttributeAccessInterface-backed attributes do sanity checks on the (endpoint, cluster) pair #10389

Closed
bzbarsky-apple opened this issue Oct 11, 2021 · 0 comments · Fixed by #11997

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

Attributes handled via the attribute store automatically handle invalid endpoint or cluster ids (i.e. nonexistent endpoint, or cluster not on endpoint), because the attribute descriptor is just not found.

But for things backed via AttributeAccessInterface this is not handed right now. As a result, for example, I can try to read the General Diagnostics cluster on endpoint 200 of chip-all-clusters-app, and that will happily succeed.

Proposed Solution

We need to add checks for the endpoint and cluster being valid. Plausible places to put these checks:

  1. Inside the ReadHandler.
  2. Inside the Reporting.cpp machinery.
  3. Inside ReadSingleClusterData.
  4. Inside findAttributeAccessOverride.
  5. Inside the AttributeAccessInterface implementations.

My preference there would be "definitely not (5)". There's a case to be made for (4), since that's the place where we are asking "is there an attribute access override for this cluster+endpoint pair?". The tradeoff seems to be that if we put the check inside (4) then we will find we don't match, return no access override, fall into the normal attribute store, discover we don't match again, and return error, which does the "do we match?" work twice. Same thing if we put it inside (1) or (2), actually: there we would end up doing the matching work twice for attributes that are not backed by an AttributeAccessInterface and do exist.

So I think I would go with (3), and in particular just do the check when we find a matching AttributeAccessInterface.

Thoughts?

Related question: should we be guaranteeing, by the time AttributeAccessInterface is called, that the given attribute exists on the given (endpoint, cluster) pair? Or leaving that up to the AttributeAccessInterface to enforce? Seems to me like it would be better to do the former, since then we can easily have a single source of truth (our topology) enforcing it for all attributes.

@erjiaqing @mrjerryjohns @vivien-apple @carol-apple @msandstedt @yufengwangca

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Nov 18, 2021
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Nov 20, 2021
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Nov 22, 2021
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Nov 24, 2021
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Nov 24, 2021
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Nov 24, 2021
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
andy31415 pushed a commit that referenced this issue Nov 24, 2021
…ist. (#11997)

In particular, writes already had this check, but we need it for reads.

Fixes #10389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant