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

Fix issues found using default CodeQL settings. #2923

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

henrygab
Copy link

Describe the PR

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, 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 condition

This 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:

for (uint8_t i = 0; i < TOTAL_DRIVER_COUNT; i++) {
    usbd_class_driver_t const* driver = get_driver(i);
    // ...
}

The type of TOTAL_DRIVER_COUNT is int (uint8_t + size_t --> promoted to int).
The type of the loop variable was uint8_t.
Therefore, this would loop infinitely if TOTAL_DRIVER_COUNT ever 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_COUNT would be in the expected range of [ 0x01u .. 0xFFu ].

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.
@@ -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

Comparison is always true because q <= 255.
@@ -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

Comparison is always true because q <= 255.
@henrygab henrygab changed the title Fix issuse found using default CodeQL settings. Fix issues found using default CodeQL settings. Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant