Fix integer overflow undefined behaviour #177
Conversation
Even though delta's type is PCRE2_SIZE, the computation for delta uses all integers at the right hand side. This means that there is a potential integer overflow. The if below then checks whether a computation equivalent to delta is larger than INT_MAX: the overflow check. However, since integer overflow is undefined behaviour, the compiler may assume it never happens. Therefore, the overflow check can be assumed to always be false even though there is casting because according to the compiler if an overflow doesn't happen for ints, it surely does not happen for INT64_OR_DOUBLE... I found this issue using the stack static analysis tool. I verified that the compiler actually optimizes away the check by looking at the IR. Fix it by computing delta with the casts first, and use that same delta in the if.
|
LINK_SIZE is a very tiny integer (usually 2), so the check (as mentioned in the comment above) is just being extra paranoid. I suspect though, that if your compiler is optimizing away that check, it might be because of a bug with it, or you should be configuring your pcre2 to use double (as a 64bit int) instead. |
| if ((INT64_OR_DOUBLE)replicate* | ||
| (INT64_OR_DOUBLE)(1 LINK_SIZE) > | ||
| (INT64_OR_DOUBLE)INT_MAX || | ||
| INT64_OR_DOUBLE delta = (INT64_OR_DOUBLE)replicate*(INT64_OR_DOUBLE)(1 LINK_SIZE); |
There was a problem hiding this comment.
if a broken int64 cast is being used here, this is likely to make the problem worst by potentially halving the width of delta.
it migth be better IMHO to only cast "replicate" to ensure the multiplication is not being done with plain ints
There was a problem hiding this comment.
a maybe cleaner way to approach this sort of check pushed (not tested) in the following branch.
note that similar logic is used in several other places, so not sure why the static analyzer used didn't pick on those.
also, details on the system/compiler that was affected (so a fix could be tested) would be ideal.
There was a problem hiding this comment.
Your approach is indeed cleaner.
System details: Linux 6.1.3, Clang 14.0.6
There was a problem hiding this comment.
could you confirm the check is indeed getting removed?, still can't reproduce that with the same compiler as shown by the following simplified code:
$ cat i.c
#include <stdio.h>
#include <limits.h>
#include <stdlib.h>
int main(int argc, char *argv[])
{
int a = atoi(argv[1]);
size_t m;
m = a * 2;
if ((long)a * 2 > INT_MAX)
printf("OVERFLOW\n");
printf("%zu\n", m);
return 0;
}
$ clang --version
Debian clang version 14.0.6
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ clang -O3 -o i i.c
$ ./i 2147483647
OVERFLOW
18446744073709551614
There was a problem hiding this comment.
That example gives the same result on my system, so this PR was probably a mistake.
Break the check in two to better protect from overflow in systems that have a 32bit PCRE2_SIZE. As a side effect make the overflow check happen before the actual code to avoid compilers optimizing it out under the assumption an integer overflow wouldn't had happened because of undefined behaviour. Fixes: PCRE2Project#177
Break the check in two to better protect from overflow in systems that have a 32bit PCRE2_SIZE. As a side effect make the overflow check happen before the actual code to avoid compilers optimizing it out under the assumption an integer overflow wouldn't had happened because of undefined behaviour. Fixes: PCRE2Project#177
Break the check in two to better protect from overflow in systems that have a 32bit PCRE2_SIZE. As a side effect make the overflow check happen before the actual code to avoid compilers optimizing it out under the assumption an integer overflow wouldn't had happened because of undefined behaviour. Fixes: PCRE2Project#177
|
What is the status of this, please? Should I implement the one-line patch in the conversation above? |
|
This PR could be closed, but I think we should do some related improvements anyway, as I still think that there might be some truth to the original static analyzer report |
|
Can we close this now? |
|
OK, I' closing this. |
Even though delta's type is PCRE2_SIZE, the computation for delta uses all integers at the right hand side.
This means that there is a potential integer overflow. The if below then checks whether a computation equivalent to delta is larger than INT_MAX: the overflow check.
However, since integer overflow is undefined behaviour, the compiler may assume it never happens. Therefore, the overflow check can be assumed to always be false even though there is casting because according to the compiler if an overflow doesn't happen for ints, it surely does not happen for INT64_OR_DOUBLE...
I found this issue using the stack static analysis tool. I verified that the compiler actually optimizes away the check by looking at the IR.
Fix it by computing delta with the casts first, and use that same delta in the if.