Fix issues found using default CodeQL settings. #2923
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
When relying projects enable CodeQL, they will by default see ten (10x) issues of the following type: `Comparison of narrow type with wide type in loop condition` The type of `TOTAL_DRIVER_COUNT` is `int` (`uint8_t` + `size_t` --> `int`) The type of the loop variable was `uint8_t`. The loop is gated by a check of `(uint8_t)i < TOTAL_DRIVER_COUNT`. As a result, a high severity alert is generated by CodeQL. This fix does the following: 1. Fix the CodeQL alert by changing the loop variable type to use `int`. 2. Explicitly limiting the loop to MAXIMUM_TOTAL_DRIVER_COUNT. 3. Sets the originally named variable to the loop variable (but c-style cast to type `uint8_t`). In addition, because there was not previously any code to ensure that `TOTAL_DRIVER_COUNT` would be in the expected range of `[0x01..0xFF]`, added a `TU_ASSERT()` at the appropriate location to ensure this condition.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -544,7 +556,8 @@ | |||
| } | |||
|
|
|||
| static void configuration_reset(uint8_t rhport) { | |||
| for (uint8_t i = 0; i < TOTAL_DRIVER_COUNT; i++) { | |||
| for (int q = 0; (q < TOTAL_DRIVER_COUNT) && (q <= MAXIMUM_TOTAL_DRIVER_COUNT); q++) { | |||
Check warning
Code scanning / CodeQL
Comparison result is always the same Warning
There was a problem hiding this comment.
Belt and suspenders: Leave in the check for (q <= MAXIMUM_TOTAL_DRIVER_COUNT) ... this is not a bug.
| @@ -1193,7 +1207,8 @@ | |||
|
|
|||
| case DCD_EVENT_SOF: | |||
| // SOF driver handler in ISR context | |||
| for (uint8_t i = 0; i < TOTAL_DRIVER_COUNT; i++) { | |||
| for (int q = 0; (q < TOTAL_DRIVER_COUNT) && (q <= MAXIMUM_TOTAL_DRIVER_COUNT); q++) { | |||
Check warning
Code scanning / CodeQL
Comparison result is always the same Warning
There was a problem hiding this comment.
Belt and suspenders: Leave in the check for (q <= MAXIMUM_TOTAL_DRIVER_COUNT) ... this is not a bug.
henrygab
changed the title
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.
Describe the PR
This fix does the following:
int.uint8_t).In addition, because there was not previously any code to ensure that
TOTAL_DRIVER_COUNTwould be in the expected range of[0x01..0xFF], added aTU_ASSERT()at the appropriate location to ensure this condition, and immediately stop execution if a configuration would ever exceed this.There is no change to the logical flow, except for that additional
TU_ASSERT().What is solved
When relying projects enable CodeQL (with default settings), and build tinyUSB,
ten (10x) high-severity alerts of the following type will be generated:
Comparison of narrow type with wide type in loop conditionThis PR updates the code to prevent these CodeQL alerts,
which otherwise show up in all projects relying on TinyUSB.
Additional context
The alerts note that the loop variable (e.g.,
i) is of a smaller size (e.g.,uint8_t) than what it is being compared against (e.g.,int). As an example, code which would generate this alert is:The type of
TOTAL_DRIVER_COUNTisint(uint8_t+size_t--> promoted toint).The type of the loop variable was
uint8_t.Therefore, this would loop infinitely if
TOTAL_DRIVER_COUNTever exceeds 0xFFu.As a result, a high severity alert is generated by CodeQL.
It is understood that the count of drivers is low (built-in is only ~12 right now), and unlikely to ever exceed 0xFFu.
However, there was no code that ensured that
TOTAL_DRIVER_COUNTwould be in the expected range of[ 0x01u .. 0xFFu ].