Description
Here is a succinct version of the code involved:
size_t min_frame_size;
size_t max_frame_size;
...
if (min_frame_size < 0 ||
min_frame_size >= (intmax_t)SIZE_MAX) {
...
}
...
if (max_frame_size < 1024 ||
max_frame_size >= (intmax_t)SIZE_MAX) {
...
In a PR that a colleague had opened, CodeQL correctly flagged both min_frame_size >= (intmax_t)SIZE_MAX and max_frame_size >= (intmax_t)SIZE_MAX and gave technically correct descriptions of the problem.
However, the colleague who saw this thought it had flagged a bug, but was wrong in explaining why. More specifically, when he told a number of us about this, we initially thought that CodeQL just got lucky by having a false positive on something that needed correction, but not for the reason CodeQL originally found it.
The explanations offered in the PR were:
Comparison is always true because min_frame_size >= 0.Comparison is always true because max_frame_size >= 1024.
I went back and forth from initially believing him, to thinking that CodeQL was right, to thinking I was wrong to say that CodeQL was right when my initial explanation that CodeQL was right was found to be wrong to confirming that CodeQL was right. Honestly, I felt that I had made a spectacle of myself midway through this process when my initial explanation that CodeQL was right turned out to be incorrect.
The reason why CodeQL is right is that on a 32-bit system, SIZE_MAX is 4294967295U. Cast this to a signed integer, and you are comparing against -1.
On a 32-bit system, his code was doing min_frame_size >= -1 and max_frame_size >= -1, which are always true. The same is true for 64-bit systems, although with the higher magnitude value 2^64 - 1 becoming -1.
When the first condition is false, then we know that min_frame_size >= 0 in the first one or max_frame_size >= 1024 in the second, so CodeQL was correct to point this out. However, without a more detailed explanation, it is easy for programmers to misunderstand the report.
I had already been aware of this since I read about this exact bug from something published by pvs-studio.com a month ago, but I could not remember it at first and after remembering that I had previously seen something on that site that resembled this, I could not find it when I looked. Had I actually found that explanation, it would have saved me some minor grief from thinking that I had made a spectacle of myself by digging deeper into it.
I suspect that there is a better explanation provided in the code scanning reports that only those with write access to the repository can see, but even if that is a better explanation, it is not helpful here because the contributor who opened the PR that CodeQL flagged cannot see that. This might be less of a problem if #11021 were fixed.