Fixed size comparisons that could lead to infinite loops #18451
Conversation
src/terminal/adapter/SixelParser.cpp
Outdated
| 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.
There was a problem hiding this comment.
Unsure if CodeQL can detect a change like this but I do prefer your suggestion so lets give it a try
src/buffer/out/Row.cpp
Outdated
| // 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.
There was a problem hiding this comment.
Nice catch. Seems like colorStarts is used below for _attr.replace() and it requires a size_type which is 32-bit, so seems that it'll work.
There was a problem hiding this comment.
Looks like it was originally narrow casted to 16-bits so that it would work with x86 and x64, from looking at the error from the latest build. In x86 size_type is an unsigned short and in x64 it's an unsigned __int64.
I'll just convert finalColumnInRow to uint16_t when it's initialized and that should solve the issue.
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
added
the
No-Recent-Activity
microsoft-github-policy-service
bot
removed
Needs-Author-Feedback
src/terminal/adapter/charsets.hpp
Outdated
| for (auto i = L'\0'; i < _translationTable.size(); i++) | ||
| for (size_t i = L'\0'; i < _translationTable.size(); i++) |
There was a problem hiding this comment.
FYI, I don't think this change compiles. It might make more sense to cast the table size if that'll satisfy the CodeQL scan, i.e. something like this:
for (auto i = L'\0'; i < gsl::narrow_cast<wchar_t>(_translationTable.size()); i++)There's no risk of an infinite loop here anyway because this code is executed at compile time.
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