C++: Use Guards library in Overflow.qll #7521
Conversation
Code changes LGTM, perhaps we should do a DCA run to check for wider results and performance changes?
| @@ -5,64 +5,27 @@ | ||
|
|
||
| import cpp | ||
| import semmle.code.cpp.controlflow.Dominance | ||
| // `GlobalValueNumbering` is only imported to prevent IR re-evaluation. |
There was a problem hiding this comment.
Is this no longer true?
There was a problem hiding this comment.
Oh, I experimented with using GVN but it didn't work out well. I'll re-add that line
Replaces the ad-hoc guard handling with the Guards library. Fixes an observed false positive pattern, and (hopefully) means some pragmas are no longer necessary for performance.
cc6c418
to
355fc0a
|
DCA run: github/codeql-dca-main#2269 |
|
The DCA run looks fine. |
Quick diff query here: https://lgtm.com/query/1144214176301359103/
- the four lost results are exactly what we want to see
- the new result is a surprise, but I'm not sure its genuinely new - I think I've seen it before. In any case it looks correct up to the lack of understanding the return value of
read.
|
The old version used |
| @@ -63,6 +87,14 @@ nodes | ||
| | test5.cpp:17:6:17:18 | call to getTaintedInt | test5.cpp:9:7:9:9 | buf | test5.cpp:17:6:17:18 | call to getTaintedInt | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value | | ||
| | test5.cpp:19:6:19:6 | y | test5.cpp:9:7:9:9 | buf | test5.cpp:19:6:19:6 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value | | ||
| | test5.cpp:19:6:19:6 | y | test5.cpp:9:7:9:9 | buf | test5.cpp:19:6:19:6 | y | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test5.cpp:9:7:9:9 | buf | User-provided value | | ||
| | test5.cpp:30:17:30:23 | tainted | test5.cpp:9:7:9:9 | buf | test5.cpp:30:17:30:23 | tainted | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value | |
There was a problem hiding this comment.
It's a shame we don't catch this one anymore. I remember those imaxabs having a way-too-good influence on the Samate score (even though it's probably quite useful on real-world code).
There was a problem hiding this comment.
That's caused by a typo in guardedAbs. Unfortunately we also lose the good result on line 40 by fixing it.
There was a problem hiding this comment.
Oh, great! (I mean, it's not great that we lose the good result on line 40, but it's great that we don't gain any additional FPs
This replaces a partial reimplementation of the Guards library in Overflow.qll with uses of
GuardCondition::ensuresLt, which handles cases where the controlled expression is not syntactically included in the controlling statement.The text was updated successfully, but these errors were encountered: