Fixed size comparisons that could lead to infinite loops #18451
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
j4james
reviewed
Jan 23, 2025
Comment on lines
611
to
+615
| if (_conformanceLevel <= 3 && _maxColors > 2 && _colorTableChanged) [[unlikely]] | ||
| { | ||
| for (IndexType tableIndex = 0; tableIndex < _maxColors; tableIndex++) | ||
| // _maxColors is 64-bit and tableIndex is 8-bit. If _maxColors is higher | ||
| // than the 8-bit integer limit, we will loop indefinitely. | ||
| for (IndexType tableIndex = 0; tableIndex < (saturated_cast<uint8_t>(_maxColors)); tableIndex++) |
There was a problem hiding this comment.
In this situation _maxColors will never be greater than 16, because that's the maximum value for conformance level 3 (which we're checking in the condition above). Would it satisfy the CodeQL scan if we also added a _maxColors <= 16 check to that condition? That seems like a preferable fix to me, because it makes it clear what values we're actually dealing with here. Suggesting that _maxColors has a 64-bit range here is very misleading.
lhecker
reviewed
Jan 23, 2025
Comment on lines
+436
to
+438
| // finalColumnInRow is 32-bit and currentIndex is 16-bit. If finalColumnInRow is higher | ||
| // than the 16-bit integer limit, we will loop indefinitely. | ||
| while (it && currentIndex <= saturated_cast<uint16_t>(finalColumnInRow)) |
There was a problem hiding this comment.
I have not tested this, but looking at just the code, it seems to me that simply removing the narrow_cast above would have the same effect of fixing this bug.
DHowett
requested changes
Jan 24, 2025
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
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.
Summary of the Pull Request
After taking in 1.22, our CodeQL process caught a few locations where we were performing integer comparisons of different sizes which could lead to an infinite loop if the larger integer goes out of range of the smaller integer.
For example, comparing
uint8_t<size_tworks untilsize_tgoes above 256, where no matter how much we changeuin8_twe will never reach 256References and Relevant Issues
CodeQL issues:
https://liquid.microsoft.com/codeql/issues/5f2b05d5-9e87-4df4-b493-f00e710d38df?copilot_promptid=E91B0CE9-0C1B-4AC2-8A46-33F49B67E058
https://liquid.microsoft.com/codeql/issues/76268284-2d4b-4b10-8aff-a947ecc1a576?copilot_promptid=E91B0CE9-0C1B-4AC2-8A46-33F49B67E058
https://liquid.microsoft.com/codeql/issues/452f966b-5b99-420e-96c0-153caea2a0b4?copilot_promptid=E91B0CE9-0C1B-4AC2-8A46-33F49B67E058
Detailed Description of the Pull Request / Additional comments
I used
saturated_cast<>for these changes to make overflow values equal to the max value of the smallest integer.Validation Steps Performed
PR Checklist