Fix multiplication overflow for function 1 and 2 #87
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
The original code calculated required buffer space for decoded values by mutiplying `byte_count` by 8. Since `byte_count` was given the type `u8`, the multiplication result would overflowed if `byte_count` is greater than 31. In other words, if more than 248 coils/discrete inputs were requested. This PR cast `byte_count` to u16 before it is used to calculate the required buffer space, thus prevents the overflow.
uklotzde
requested changes
Aug 12, 2021
src/codec/mod.rs
Outdated
| @@ -234,15 +234,15 @@ impl TryFrom<Bytes> for Response { | |||
| let x = &bytes[2..]; | |||
| // Here we have not information about the exact requested quantity so we just | |||
| // unpack the whole byte. | |||
| let quantity = u16::from(byte_count * 8); | |||
| let quantity = byte_count as u16 * 8; | |||
There was a problem hiding this comment.
Using u16::from(byte_count) * 8 would be safer than casting the value with as.
There was a problem hiding this comment.
Thanks for the feedback! I will fix this and try to add a test case later today.
There was a problem hiding this comment.
This is fixed and test cases are added in the latest commits. Please let me know if the fixes are preferred to be squashed.
|
You can ignore the clippy warnings. These will finally be fixed by #88. |
|
@flosse Backport this fix and publish v0.4.1? |
just in case of accidental lossy conversion
Both test cases assume the spec'd max quantity, which is 2000, of items are requested, which is actually far more than the number that would cause multiplication overflow, which was 249.
uklotzde
approved these changes
Aug 13, 2021
|
I'll stash the commits and fix the formatting issues while rebasing. |
|
Merged. |
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.
The original code calculated required buffer space for decoded values by
mutiplying
byte_countby 8. Sincebyte_countwas given the typeu8, the multiplication result would overflowed ifbyte_countisgreater than 31. In other words, if more than 248 coils/discrete inputs
were requested.
This PR cast
byte_countto u16 before it is used to calculate therequired buffer space, thus prevents the overflow.