Skip to content

Commit b8a5aa5

Browse files
MathiasVPMathiasVP
committed
C++: Fix a couple of range analysis issues:
1. The new query is expecting pointer arithmetic operations to generate range-analysis bounds, but this wasn't true on main. 2. The bounds generated by `boundFlowCond` was incorrectly inferred as non-strict when comparing a pointers (unlike when comparing values of integral types). This gave FPs in the new query. This also fixes a couple of missing results in existing queries that use the new range-analysis library.
1 parent d981f89 commit b8a5aa5

File tree

8 files changed

+43
-30
lines changed

8 files changed

+43
-30
lines changed

‎cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/Bound.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ private newtype TBound =
2828
i.(LoadInstruction).getSourceAddress() instanceof FieldAddressInstruction
2929
or
3030
i.getAUse() instanceof ArgumentOperand
31+
or
32+
i instanceof PointerArithmeticInstruction
33+
or
34+
i.getAUse() instanceof AddressOperand
3135
)
3236
}
3337

‎cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExpr.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,11 +178,11 @@ class SemRelationalExpr extends SemBinaryExpr {
178178
}
179179

180180
class SemAddExpr extends SemBinaryExpr {
181-
SemAddExpr() { opcode instanceof Opcode::Add }
181+
SemAddExpr() { opcode instanceof Opcode::Add or opcode instanceof Opcode::PointerAdd }
182182
}
183183

184184
class SemSubExpr extends SemBinaryExpr {
185-
SemSubExpr() { opcode instanceof Opcode::Sub }
185+
SemSubExpr() { opcode instanceof Opcode::Sub or opcode instanceof Opcode::PointerSub }
186186
}
187187

188188
class SemMulExpr extends SemBinaryExpr {

‎cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticOpcode.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,18 @@ module Opcode {
6565
override string toString() { result = "Add" }
6666
}
6767

68+
class PointerAdd extends Opcode, TPointerAdd {
69+
override string toString() { result = "PointerAdd" }
70+
}
71+
6872
class Sub extends Opcode, TSub {
6973
override string toString() { result = "Sub" }
7074
}
7175

76+
class PointerSub extends Opcode, TPointerSub {
77+
override string toString() { result = "PointerSub" }
78+
}
79+
7280
class Mul extends Opcode, TMul {
7381
override string toString() { result = "Mul" }
7482
}

‎cpp/ql/lib/experimental/semmle/code/cpp/semantic/analysis/RangeAnalysis.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,9 @@ private SemGuard boundFlowCond(
223223
else resultIsStrict = testIsTrue.booleanNot()
224224
) and
225225
(
226-
if getTrackedTypeForSsaVariable(v) instanceof SemIntegerType
226+
if
227+
getTrackedTypeForSsaVariable(v) instanceof SemIntegerType or
228+
getTrackedTypeForSsaVariable(v) instanceof SemAddressType
227229
then
228230
upper = true and strengthen = -1
229231
or

‎cpp/ql/test/experimental/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@
2020
| test.cpp:62:10:62:13 | Load: iter | test.cpp:60:17:60:17 | ValueNumberBound | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
2121
| test.cpp:62:10:62:13 | Load: iter | test.cpp:60:17:60:17 | ValueNumberBound | 3 | true | CompareLT: ... < ... | test.cpp:61:32:61:51 | test.cpp:61:32:61:51 |
2222
| test.cpp:62:10:62:13 | Load: iter | test.cpp:61:39:61:51 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:61:32:61:51 | test.cpp:61:32:61:51 |
23+
| test.cpp:62:10:62:13 | Load: iter | test.cpp:61:48:61:50 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:61:32:61:51 | test.cpp:61:32:61:51 |
2324
| test.cpp:67:10:67:13 | Load: iter | test.cpp:60:17:60:17 | ValueNumberBound | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
2425
| test.cpp:67:10:67:13 | Load: iter | test.cpp:60:17:60:17 | ValueNumberBound | 3 | true | CompareLT: ... < ... | test.cpp:66:32:66:41 | test.cpp:66:32:66:41 |
2526
| test.cpp:67:10:67:13 | Load: iter | test.cpp:61:32:61:35 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:66:32:66:41 | test.cpp:66:32:66:41 |
2627
| test.cpp:67:10:67:13 | Load: iter | test.cpp:61:39:61:51 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:66:32:66:41 | test.cpp:66:32:66:41 |
28+
| test.cpp:67:10:67:13 | Load: iter | test.cpp:61:48:61:50 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:66:32:66:41 | test.cpp:66:32:66:41 |
2729
| test.cpp:77:12:77:12 | Load: i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
2830
| test.cpp:77:12:77:12 | Load: i | test.cpp:72:15:72:15 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:76:20:76:24 | test.cpp:76:20:76:24 |
2931
| test.cpp:77:12:77:12 | Load: i | test.cpp:72:22:72:22 | ValueNumberBound | -1 | true | CompareLT: ... < ... | test.cpp:76:20:76:24 | test.cpp:76:20:76:24 |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/OverrunWriteProductFlow.expected

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:26:18:26:23 | string | test.cpp:26:31:26:39 | (size_t)... |
2-
| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:30:18:30:23 | string | test.cpp:30:31:30:39 | (size_t)... |
1+
| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:26:18:26:23 | Load | test.cpp:26:31:26:39 | Convert |
2+
| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:30:18:30:23 | Load | test.cpp:30:31:30:39 | Convert |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/array-access/ArrayAccessProductFlow.expected

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,20 @@
1-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | arr | test.cpp:4:24:4:27 | size |
2-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | arr | test.cpp:4:24:4:27 | size |
3-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | arr | test.cpp:5:25:5:28 | size |
4-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | arr | test.cpp:5:25:5:28 | size |
5-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | arr | test.cpp:9:26:9:29 | size |
6-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:5:25:5:28 | size | test.cpp:10:9:10:11 | arr | test.cpp:5:25:5:28 | size |
7-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:5:25:5:28 | size | test.cpp:10:9:10:11 | arr | test.cpp:5:25:5:28 | size |
8-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:5:25:5:28 | size | test.cpp:10:9:10:11 | arr | test.cpp:9:26:9:29 | size |
9-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:9:26:9:29 | size | test.cpp:10:9:10:11 | arr | test.cpp:9:26:9:29 | size |
10-
| test.cpp:4:17:4:22 | call to malloc | test.cpp:9:26:9:29 | size | test.cpp:10:9:10:11 | arr | test.cpp:9:26:9:29 | size |
11-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | p | test.cpp:55:5:55:19 | Store |
12-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | p | test.cpp:55:16:55:19 | size |
13-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | p | test.cpp:55:16:55:19 | size |
14-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | p | test.cpp:56:20:56:23 | size |
15-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:56:20:56:23 | size | test.cpp:63:13:63:13 | p | test.cpp:56:20:56:23 | size |
16-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:56:20:56:23 | size | test.cpp:63:13:63:13 | p | test.cpp:56:20:56:23 | size |
17-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:58:29:58:32 | size | test.cpp:63:13:63:13 | p | test.cpp:58:29:58:32 | size |
18-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:58:29:58:32 | size | test.cpp:63:13:63:13 | p | test.cpp:58:29:58:32 | size |
19-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:62:30:62:33 | size | test.cpp:63:13:63:13 | p | test.cpp:62:30:62:33 | size |
20-
| test.cpp:56:13:56:18 | call to malloc | test.cpp:62:30:62:33 | size | test.cpp:63:13:63:13 | p | test.cpp:62:30:62:33 | size |
21-
| test.cpp:70:14:70:19 | call to malloc | test.cpp:69:17:69:20 | size | test.cpp:83:14:83:14 | p | test.cpp:82:31:82:34 | size |
22-
| test.cpp:70:14:70:19 | call to malloc | test.cpp:69:17:69:20 | size | test.cpp:93:14:93:14 | p | test.cpp:88:30:88:33 | size |
23-
| test.cpp:70:14:70:19 | call to malloc | test.cpp:69:17:69:20 | size | test.cpp:93:14:93:14 | p | test.cpp:92:31:92:34 | size |
1+
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | Load | test.cpp:5:25:5:28 | Load |
2+
| test.cpp:4:17:4:22 | call to malloc | test.cpp:4:24:4:27 | size | test.cpp:10:9:10:11 | Load | test.cpp:9:26:9:29 | Load |
3+
| test.cpp:4:17:4:22 | call to malloc | test.cpp:5:25:5:28 | size | test.cpp:10:9:10:11 | Load | test.cpp:5:25:5:28 | Load |
4+
| test.cpp:4:17:4:22 | call to malloc | test.cpp:5:25:5:28 | size | test.cpp:10:9:10:11 | Load | test.cpp:9:26:9:29 | Load |
5+
| test.cpp:4:17:4:22 | call to malloc | test.cpp:9:26:9:29 | size | test.cpp:10:9:10:11 | Load | test.cpp:9:26:9:29 | Load |
6+
| test.cpp:22:13:22:18 | call to malloc | test.cpp:21:16:21:19 | size | test.cpp:35:13:35:13 | Load | test.cpp:30:29:30:32 | Load |
7+
| test.cpp:22:13:22:18 | call to malloc | test.cpp:21:16:21:19 | size | test.cpp:35:13:35:13 | Load | test.cpp:34:30:34:33 | Load |
8+
| test.cpp:22:13:22:18 | call to malloc | test.cpp:21:16:21:19 | size | test.cpp:45:13:45:13 | Load | test.cpp:40:29:40:32 | Load |
9+
| test.cpp:22:13:22:18 | call to malloc | test.cpp:21:16:21:19 | size | test.cpp:45:13:45:13 | Load | test.cpp:44:30:44:33 | Load |
10+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | Load | test.cpp:55:5:55:19 | Store |
11+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | Load | test.cpp:55:5:55:19 | Store |
12+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | Load | test.cpp:55:16:55:19 | Load |
13+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | Load | test.cpp:56:20:56:23 | Load |
14+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | Load | test.cpp:58:29:58:32 | Load |
15+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:55:16:55:19 | size | test.cpp:63:13:63:13 | Load | test.cpp:62:30:62:33 | Load |
16+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:58:29:58:32 | size | test.cpp:63:13:63:13 | Load | test.cpp:58:29:58:32 | Load |
17+
| test.cpp:56:13:56:18 | call to malloc | test.cpp:62:30:62:33 | size | test.cpp:63:13:63:13 | Load | test.cpp:62:30:62:33 | Load |
18+
| test.cpp:70:14:70:19 | call to malloc | test.cpp:69:17:69:20 | size | test.cpp:83:14:83:14 | Load | test.cpp:82:31:82:34 | Load |
19+
| test.cpp:70:14:70:19 | call to malloc | test.cpp:69:17:69:20 | size | test.cpp:93:14:93:14 | Load | test.cpp:88:30:88:33 | Load |
20+
| test.cpp:70:14:70:19 | call to malloc | test.cpp:69:17:69:20 | size | test.cpp:93:14:93:14 | Load | test.cpp:92:31:92:34 | Load |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/array-access/test.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ void test2(int size) {
3232
}
3333

3434
for (int i = 0; i <= arr.size; i++) {
35-
arr.p[i] = i; // BAD [NOT DETECTED]
35+
arr.p[i] = i; // BAD
3636
}
3737
}
3838

@@ -42,7 +42,7 @@ void test3_callee(array_t arr) {
4242
}
4343

4444
for (int i = 0; i <= arr.size; i++) {
45-
arr.p[i] = i; // BAD [NOT DETECTED]
45+
arr.p[i] = i; // BAD
4646
}
4747
}
4848

0 commit comments

Comments
 (0)