Fix issues found using default CodeQL settings.

[henrygab]

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

[henrygab]

thank you for your PR and patient, I was off for TET (Lunar New Year).

Welcome back, and I hope your time off was all that you hoped!

I think we may just cast TOTAL_DRIVER_COUNT to uint8_t instead. To ensure TOTAL_DRIVER_COUNT is less than 255, we can do an TU_ASSERT() in tud_init() so that we only run the check once.

#define TOTAL_DRIVER_COUNT    ((uint8_t) (_app_driver_count + BUILTIN_DRIVER_COUNT))

Could you help me understand the proposed assertion, which I understand to be: TU_ASSERT(TOTAL_DRIVER_COUNT < 255)?

If I understand correctly, if _app_driver_count is 253 (for example), and BUILTIN_DRIVER_COUNT is 4, then TOTAL_DRIVER_COUNT would be evaluate to 1, and the assertion would not fire?

[hathach]

I made the change proposal for the idea here https://github.com/hathach/tinyusb/pull/2987 . We check/assert when app driver count is intialized and give up if total driver count overflows 8-bit. The rest should function as it is. Let me know if this makes sense to you

[henrygab]

It's been a couple months since I worked on this. If I understand your PR, you are trying to explicitly keep that macro to type uint8_t, and chose to use TU_ASSERT() to cause the firmware to hang at runtime if this is exceeded.

If this reduces the CodeQL warnings, then I'm OK with your alternate method, and you can close this PR.

[hathach]

It's been a couple months since I worked on this. If I understand your PR, you are trying to explicitly keep that macro to type uint8_t, and chose to use TU_ASSERT() to cause the firmware to hang at runtime if this is exceeded.

If this reduces the CodeQL warnings, then I'm OK with your alternate method, and you can close this PR.

thanks, I am always working on something and got pending PR. the _app_driver_count is only assigned when initialization with tud_init() and the sum is checked that not exceed 255. So I think it is safe to cast later on. Maybe we can use the _total_driver_count variable instead. I will check again later.

[hathach]

closed since I still prefer the simpler alt https://github.com/hathach/tinyusb/pull/2987. Thank you @henrygab for bring this issue into attention.