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

Write attributes paths are not validated unlike read #19000

Closed
fessehaeve opened this issue May 31, 2022 · 8 comments
Closed

Write attributes paths are not validated unlike read #19000

fessehaeve opened this issue May 31, 2022 · 8 comments

Comments

@fessehaeve
Copy link
Contributor

fessehaeve commented May 31, 2022

Problem

./chip-tool any read-by-id 0x11110001 0x1111f001 1 1 fails with INVALID_ACTION but
./chip-tool any write-by-id 0x11110001 0x1111f001 "abc" 1 1 succeeds.

This is concerning the valid range for cluster id and attribute id.

Proposed Solution

Do we need to have this check?

@mrjerryjohns
Copy link
Contributor

On master as of today, the write does not actually succeed - it returns back an UNSUPPORTED_CLUSTER error.

This however isn't presumably the right error. The WriteHandler logic today does not actually validate that the Attribute and Cluster IDs are indeed well formed. This should be added.

@CamWms
Copy link

CamWms commented Jun 29, 2022

Working on PR now. Should the action abort, or generate a new error: INVALID_PATH?
For now the IM aborts, just as it would for a illegal wildcards, group or missing elements in the path.
https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5425

@mrjerryjohns
Copy link
Contributor

@bzbarsky-apple I'm going to recommend we not fix this for v1.0, and defer this, since we don't actually have an issue with spec compliance.

@mrjerryjohns mrjerryjohns added V1.X and removed V1.0 spec Mismatch between spec and implementation labels Jul 12, 2022
@CamWms
Copy link

CamWms commented Jul 14, 2022

I have a PR #5425 to check if paths are well-formed, but IDs should not be checked for in-range by the SDK code except if the range is within the MS range. The valid range is to direct new ID allocation, not for operational checks. We may expand or reduce the range in the future. If the SDK does not recognize an ID (in or outside the current range), then that is enough.

@stale
Copy link

stale bot commented Jan 13, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Jan 13, 2023
@bzbarsky-apple bzbarsky-apple removed the stale Stale issue or PR label Jan 13, 2023
@stale
Copy link

stale bot commented Aug 7, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Aug 7, 2023
@stale
Copy link

stale bot commented Oct 15, 2023

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Oct 15, 2023
@stale stale bot removed the stale Stale issue or PR label Oct 16, 2023
@bzbarsky-apple
Copy link
Contributor

Fixed by #29461

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants