Skip to content

Commit c117a1e

Browse files
authored
C++: demote VeryLikelyOverrunWrite cast results
There were some false positives where something like int x; // ... sprintf(buff, "%ld", (long)x); was considered as if the parameter had a non-trivial range analysis only because the range of `int` is smaller than the range for `long`, without any non-trivial range analysis actually done on `x`. These will now be reported by `OverrunWrite` instead.
1 parent 630982c commit c117a1e

File tree

3 files changed

+12
-8
lines changed

3 files changed

+12
-8
lines changed

cpp/ql/lib/semmle/code/cpp/commons/Printf.qll

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,9 +1197,9 @@ class FormatLiteral extends Literal {
11971197
// The second case uses range analysis to deduce a length that's shorter than the length
11981198
// of the number -2^31.
11991199
exists(Expr arg, float lower, float upper |
1200-
arg = this.getUse().getConversionArgument(n).getFullyConverted() and
1201-
lower = lowerBound(arg) and
1202-
upper = upperBound(arg)
1200+
arg = this.getUse().getConversionArgument(n) and
1201+
lower = lowerBound(arg.getFullyConverted()) and
1202+
upper = upperBound(arg.getFullyConverted())
12031203
|
12041204
valueBasedBound =
12051205
max(int cand |
@@ -1216,6 +1216,8 @@ class FormatLiteral extends Literal {
12161216
else cand = lengthInBase10(upper)
12171217
)
12181218
) and
1219+
// we don't want to call this on `arg.getFullyConverted()` as we want
1220+
// to detect non-trivial range analysis without taking into account up-casting
12191221
reason = getEstimationReasonForIntegralExpression(arg)
12201222
) and
12211223
len = valueBasedBound.minimum(typeBasedBound)
@@ -1229,9 +1231,9 @@ class FormatLiteral extends Literal {
12291231
// The second case uses range analysis to deduce a length that's shorter than
12301232
// the length of the number 2^31 - 1.
12311233
exists(Expr arg, float lower, float upper |
1232-
arg = this.getUse().getConversionArgument(n).getFullyConverted() and
1233-
lower = lowerBound(arg) and
1234-
upper = upperBound(arg)
1234+
arg = this.getUse().getConversionArgument(n) and
1235+
lower = lowerBound(arg.getFullyConverted()) and
1236+
upper = upperBound(arg.getFullyConverted())
12351237
|
12361238
valueBasedBound =
12371239
lengthInBase10(max(float cand |
@@ -1241,6 +1243,8 @@ class FormatLiteral extends Literal {
12411243
or
12421244
cand = upper
12431245
)) and
1246+
// we don't want to call this on `arg.getFullyConverted()` as we want
1247+
// to detect non-trivial range analysis without taking into account up-casting
12441248
reason = getEstimationReasonForIntegralExpression(arg)
12451249
) and
12461250
len = valueBasedBound.minimum(typeBasedBound)

cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,5 @@
44
| tests.cpp:316:2:316:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. |
55
| tests.cpp:327:2:327:8 | call to sprintf | This 'call to sprintf' operation requires 12 bytes but the destination is only 4 bytes. |
66
| tests.cpp:329:3:329:9 | call to sprintf | This 'call to sprintf' operation requires 12 bytes but the destination is only 4 bytes. |
7+
| tests.cpp:350:2:350:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. |
8+
| tests.cpp:363:2:363:8 | call to sprintf | This 'call to sprintf' operation requires 5 bytes but the destination is only 4 bytes. |

cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/VeryLikelyOverrunWrite.expected

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,5 @@
77
| tests.cpp:343:2:343:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. |
88
| tests.cpp:345:2:345:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 2 bytes. |
99
| tests.cpp:347:2:347:8 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. |
10-
| tests.cpp:350:2:350:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. |
1110
| tests.cpp:354:2:354:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. |
1211
| tests.cpp:358:2:358:8 | call to sprintf | This 'call to sprintf' operation requires 4 bytes but the destination is only 3 bytes. |
13-
| tests.cpp:363:2:363:8 | call to sprintf | This 'call to sprintf' operation requires 5 bytes but the destination is only 4 bytes. |

0 commit comments

Comments
 (0)