GH-26: Fix infinite loop on invalid next configuration parameter. #27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ameter. When 'Parameter Number' and 'Next Parameter Number' are equal, configuration parameter discovery can not complete. For example, this Z-Wave frame will set Node Interview into an infinite loop: 70 : COMMAND_CLASS_CONFIGURATION 0F : CONFIGURATION_PROPERTIES_REPORT_V4 00 : Parameter Number 1 (MSB) 71 : Parameter Number 2 (LSB) 09 : Format 0x01 Unsigned Integer, size 0x1 01 : Min Value 64 : Max Value 0A : Default Value 00 : Next Parameter Number (MSB) 71 : Next Parameter Number (LSB) 02 : No Bulk support 0x1, Not Advanced Parameter 0x0 Signed-off-by: Nenad Kljajic <nkljajic@control4.com>
|
ok this change looks good, I'll give a try, meanwhile a unit test covering this would be welcome |
|
Great ! I see this building on your side: We can give a try on our codebase cc: @silabs-borisl |
silabs-borisl
added a commit
to silabs-borisl/UnifySDK
that referenced
this pull request
Feb 16, 2024
ZPC implementation only Forwarded: SiliconLabs#27 Bug-SiliconLabs: UIC-3066 Bug-Github: SiliconLabs#27
silabs-borisl
added a commit
to silabs-borisl/UnifySDK
that referenced
this pull request
Feb 16, 2024
ZPC implementation only Forwarded: SiliconLabs#27 Bug-SiliconLabs: UIC-3068 Bug-Github: SiliconLabs#27
silabs-borisl
added a commit
to silabs-borisl/UnifySDK
that referenced
this pull request
Feb 16, 2024
ZPC implementation only Forwarded: SiliconLabs#27 Bug-SiliconLabs: UIC-3067 Bug-Github: SiliconLabs#27
silabs-borisl
added a commit
to silabs-borisl/UnifySDK
that referenced
this pull request
Feb 16, 2024
Forwarded: SiliconLabs#27 Bug-SiliconLabs: UIC-3042 Bug-Github: SiliconLabs#27
nobriot
reviewed
Mar 23, 2024
| @@ -723,6 +723,16 @@ static sl_status_t | |||
| if (frame_length >= (current_index + 2)) { | |||
| next_id = (configuration_parameter_id_t)((frame[current_index] << 8) | |||
| | frame[current_index + 1]); | |||
| if (next_id == parameter_id) { | |||
There was a problem hiding this comment.
Wouldn't there be the same issue if a device report a next parameter number lower than the current?
Suggested change
| if (next_id == parameter_id) { | |
| if (next_id <= parameter_id) { |
And of course also adjust the log message.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When 'Parameter Number' and 'Next Parameter Number' are equal, configuration parameter discovery can not complete.
For example, this RX Z-Wave frame will set Node Interview into an infinite loop:
70 : COMMAND_CLASS_CONFIGURATION
0F : CONFIGURATION_PROPERTIES_REPORT_V4
00 : Parameter Number 1 (MSB)
71 : Parameter Number 2 (LSB)
09 : Format 0x01 Unsigned Integer, size 0x1
01 : Min Value
64 : Max Value
0A : Default Value
00 : Next Parameter Number (MSB)
71 : Next Parameter Number (LSB)
02 : No Bulk support 0x1, Not Advanced Parameter 0x0
Change
Detect case when next and current parameter numbers match and exit discovery loop.
Checklist