CodeQL -- Actual fixes #2720
Merged
CodeQL -- Actual fixes #2720
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
Code was previously performing arithmetic in various loop check conditions. Integer promotion rules could cause unintended comparisons. `spiffs` defined `fs->block_count` as `uint32_t`, but defined `spiffs_page_ix` as `uint16_t`. Various overflow checks detected by CodeQL and fixed by checking for those conditions before looping.
The compiler warning is incorrect. Since `calloc()` zero's memory, can remove redundant line setting value to zero, giving quieter builds.
henrygab
commented
Jan 15, 2025
| @@ -535,26 +535,61 @@ static s32_t spiffs_page_consistency_check_i(spiffs *fs) { | |||
|
|
|||
| s32_t res = SPIFFS_OK; | |||
| spiffs_page_ix pix_offset = 0; | |||
| // Avoid arithmetic in loop conditions (integer promotion rules can cause unintended consequences) | |||
There was a problem hiding this comment.
Please have second set of eyes review these checks.
This is supported for GCC >= version 13 See GCC bug 85487: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85487
iceman1001
reviewed
Jan 15, 2025
iceman1001
reviewed
Jan 15, 2025
iceman1001
reviewed
Jan 15, 2025
iceman1001
reviewed
Jan 15, 2025
iceman1001
reviewed
Jan 15, 2025
iceman1001
reviewed
Jan 15, 2025
Found by iceman1001's code review ... THANK YOU!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
So we stuck at that stupid |
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.
Many of the CodeQL bugs of type "Comparison between A of TypeA and B of wider type TypeB" were caused by having arithmetic within a
for()loop's comparison clause. Not only were the operations redundant, but they could cause unintended consequences (e.g., integer promotion rules). Relatively simple to fix (use larger of two sizes for loop variable, compare unsigned vs. unsigned, etc.).The other reason for many of these was that spiffs, in the definition of
struct spiffs_t, defined fieldblock_countof typeuint32_t, but defined fieldfree_cursor_block_ixto be of typespiffs_block_ix. In PM3,spiffs_block_ixis defined asuint16_t. Thus, a variable of typespiffs_block_ixcannot directly loop againstblock_count(potential infinite loops).Inline "conversation"s added below, both to explain some changes, and to mark code that should have a second set of eyes review.