Skip to content

Commit 15d5ad7

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#12822 from MathiasVP/promote-redundant-null-check-simple
C++: Promote `cpp/redundant-null-check-simple` to Code Scanning
2 parents 3f8ac1a + b7bbdb7 commit 15d5ad7

File tree

3 files changed

+211
-14
lines changed

3 files changed

+211
-14
lines changed

‎cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql

Lines changed: 151 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,30 +3,32 @@
33
* @description Checking a pointer for nullness after dereferencing it is
44
* likely to be a sign that either the check can be removed, or
55
* it should be moved before the dereference.
6-
* @kind problem
6+
* @kind path-problem
77
* @problem.severity error
8+
* @precision high
89
* @id cpp/redundant-null-check-simple
910
* @tags reliability
1011
* correctness
1112
* external/cwe/cwe-476
1213
*/
1314

14-
/*
15-
* Note: this query is not assigned a precision yet because we don't want it
16-
* to be included in query suites until its performance is well understood.
17-
*/
18-
1915
import cpp
2016
import semmle.code.cpp.ir.IR
2117
import semmle.code.cpp.ir.ValueNumbering
18+
import PathGraph
2219

20+
/** An instruction that represents a null pointer. */
2321
class NullInstruction extends ConstantValueInstruction {
2422
NullInstruction() {
2523
this.getValue() = "0" and
2624
this.getResultIRType() instanceof IRAddressType
2725
}
2826
}
2927

28+
/**
29+
* Holds if `checked` is an instruction that is checked against a null value,
30+
* and `bool` is the instruction that represents the result of the comparison
31+
*/
3032
predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
3133
bool =
3234
any(CompareInstruction cmp |
@@ -49,14 +51,155 @@ predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
4951
)
5052
}
5153

52-
pragma[noinline]
54+
pragma[nomagic]
5355
predicate candidateResult(LoadInstruction checked, ValueNumber value, IRBlock dominator) {
5456
explicitNullTestOfInstruction(checked, _) and
5557
not checked.getAst().isInMacroExpansion() and
5658
value.getAnInstruction() = checked and
5759
dominator.dominates(checked.getBlock())
5860
}
5961

62+
/**
63+
* This module constructs a pretty edges relation out of the results produced by
64+
* the `candidateResult` predicate: We create a path using the instruction successor-
65+
* relation from the dereference to the null check. To avoid generating very long paths,
66+
* we compact the edges relation so that `edges(i1, i2)` only holds when `i2` is the first
67+
* instruction that is control-flow reachable from `i1` with the same global value number
68+
* as `i1`.
69+
*/
70+
module PathGraph {
71+
/**
72+
* Holds if `deref` is a load instruction that loads a value
73+
* from the address `address`. This predicate is restricted to
74+
* those pairs for which we will end up reporting a result.
75+
*/
76+
private predicate isSource(Instruction address, LoadInstruction deref) {
77+
exists(ValueNumber sourceValue |
78+
candidateResult(_, sourceValue, deref.getBlock()) and
79+
sourceValue.getAnInstruction() = address and
80+
deref.getSourceAddress() = address
81+
)
82+
}
83+
84+
/**
85+
* Holds if `checked` has global value number `vn` and is an instruction that is
86+
* used in a check against a null value.
87+
*/
88+
private predicate isSink(LoadInstruction checked, ValueNumber vn) {
89+
candidateResult(checked, vn, _)
90+
}
91+
92+
/** Holds if `i` is control-flow reachable from a relevant `LoadInstruction`. */
93+
private predicate fwdFlow(Instruction i) {
94+
isSource(i, _)
95+
or
96+
exists(Instruction mid |
97+
fwdFlow(mid) and
98+
mid.getASuccessor() = i
99+
)
100+
}
101+
102+
/**
103+
* Holds if `i` is part of a path from a relevant `LoadInstruction` to a
104+
* check against a null value that compares a value against an instruction
105+
* with global value number `vn`.
106+
*/
107+
private predicate revFlow(Instruction i, ValueNumber vn) {
108+
fwdFlow(i) and
109+
(
110+
isSink(i, vn)
111+
or
112+
exists(Instruction mid |
113+
revFlow(mid, vn) and
114+
i.getASuccessor() = mid
115+
)
116+
)
117+
}
118+
119+
/**
120+
* Gets a first control-flow successor of `i` that has the same
121+
* global value number as `i`.
122+
*/
123+
private Instruction getASuccessor(Instruction i) {
124+
exists(ValueNumber vn |
125+
vn.getAnInstruction() = i and
126+
result = getASuccessorWithValueNumber(i, vn)
127+
)
128+
}
129+
130+
/**
131+
* Gets a first control-flow successor of `i` that has the same
132+
* global value number as `i`. Furthermore, `i` has global value
133+
* number `vn`.
134+
*/
135+
private Instruction getASuccessorWithValueNumber(Instruction i, ValueNumber vn) {
136+
revFlow(i, vn) and
137+
result = getASuccessorWithValueNumber0(vn, i.getASuccessor()) and
138+
vn.getAnInstruction() = i
139+
}
140+
141+
pragma[nomagic]
142+
private Instruction getASuccessorWithValueNumber0(ValueNumber vn, Instruction i) {
143+
result = getASuccessorIfDifferentValueNumberTC(vn, i) and
144+
vn.getAnInstruction() = result
145+
}
146+
147+
/**
148+
* Computes the reflexive transitive closure of `getASuccessorIfDifferentValueNumber`.
149+
*/
150+
private Instruction getASuccessorIfDifferentValueNumberTC(ValueNumber vn, Instruction i) {
151+
revFlow(i, vn) and
152+
(
153+
i = result and
154+
vn.getAnInstruction() = i
155+
or
156+
exists(Instruction mid |
157+
mid = getASuccessorIfDifferentValueNumber(vn, i) and
158+
result = getASuccessorIfDifferentValueNumberTC(vn, mid)
159+
)
160+
)
161+
}
162+
163+
/**
164+
* Gets an instruction that is a control-flow successor of `i` and which is not assigned
165+
* the global value number `vn`.
166+
*/
167+
private Instruction getASuccessorIfDifferentValueNumber(ValueNumber vn, Instruction i) {
168+
revFlow(i, vn) and
169+
revFlow(result, vn) and
170+
not vn.getAnInstruction() = i and
171+
pragma[only_bind_into](result) = pragma[only_bind_into](i).getASuccessor()
172+
}
173+
174+
query predicate nodes(Instruction i, string key, string val) {
175+
revFlow(i, _) and
176+
key = "semmle.label" and
177+
val = i.getAst().toString()
178+
}
179+
180+
/**
181+
* The control-flow successor relation, compacted by stepping
182+
* over instruction that don't preserve the global value number.
183+
*
184+
* There is one exception to the above preservation rule: The
185+
* initial step from the `LoadInstruction` (that is, the sink)
186+
* steps to the first control-flow reachable instruction that
187+
* has the same value number as the load instruction's address
188+
* operand.
189+
*/
190+
query predicate edges(Instruction i1, Instruction i2) {
191+
getASuccessor(i1) = i2
192+
or
193+
// We could write `isSource(i2, i1)` here, but that would
194+
// include a not-very-informative step from `*p` to `p`.
195+
// So we collapse `*p` -> `p` -> `q` to `*p` -> `q`.
196+
exists(Instruction mid |
197+
isSource(mid, i1) and
198+
getASuccessor(mid) = i2
199+
)
200+
}
201+
}
202+
60203
from LoadInstruction checked, LoadInstruction deref, ValueNumber sourceValue, IRBlock dominator
61204
where
62205
candidateResult(checked, sourceValue, dominator) and
@@ -67,5 +210,5 @@ where
67210
// the pointer was null. To follow this idea to its full generality, we
68211
// should also give an alert when `check` post-dominates `deref`.
69212
deref.getBlock() = dominator
70-
select checked, "This null check is redundant because $@ in any case.", deref,
213+
select checked, deref, checked, "This null check is redundant because $@ in any case.", deref,
71214
"the value is dereferenced"

‎cpp/ql/src/change-notes/2023-04-13-redundant-null-check.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* The query `cpp/redundant-null-check-simple` has been promoted to Code Scanning. The query finds cases where a pointer is compared to null after it has already been dereferenced. Such comparisons likely indicate a bug at the place where the pointer is dereferenced, or where the pointer is compared to null.

‎cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,56 @@
1-
| RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | the value is dereferenced |
2-
| RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | the value is dereferenced |
3-
| RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:19:7:19:9 | Load: * ... | the value is dereferenced |
4-
| RedundantNullCheckSimple.cpp:48:12:48:12 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:51:10:51:11 | Load: * ... | the value is dereferenced |
5-
| RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | the value is dereferenced |
6-
| RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | the value is dereferenced |
1+
nodes
2+
| RedundantNullCheckSimple.cpp:3:3:3:3 | VariableAddress: x | semmle.label | x |
3+
| RedundantNullCheckSimple.cpp:3:3:3:8 | Store: ... = ... | semmle.label | ... = ... |
4+
| RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | semmle.label | * ... |
5+
| RedundantNullCheckSimple.cpp:3:8:3:8 | Load: p | semmle.label | p |
6+
| RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | semmle.label | p |
7+
| RedundantNullCheckSimple.cpp:4:7:4:7 | VariableAddress: p | semmle.label | p |
8+
| RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | semmle.label | * ... |
9+
| RedundantNullCheckSimple.cpp:10:11:10:12 | Store: * ... | semmle.label | * ... |
10+
| RedundantNullCheckSimple.cpp:10:12:10:12 | Load: p | semmle.label | p |
11+
| RedundantNullCheckSimple.cpp:11:7:11:7 | Load: x | semmle.label | x |
12+
| RedundantNullCheckSimple.cpp:11:7:11:7 | VariableAddress: x | semmle.label | x |
13+
| RedundantNullCheckSimple.cpp:11:7:11:13 | CompareGT: ... > ... | semmle.label | ... > ... |
14+
| RedundantNullCheckSimple.cpp:11:7:11:13 | ConditionalBranch: ... > ... | semmle.label | ... > ... |
15+
| RedundantNullCheckSimple.cpp:11:11:11:13 | Constant: 100 | semmle.label | 100 |
16+
| RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | semmle.label | p |
17+
| RedundantNullCheckSimple.cpp:13:8:13:8 | VariableAddress: p | semmle.label | p |
18+
| RedundantNullCheckSimple.cpp:19:3:19:3 | VariableAddress: x | semmle.label | x |
19+
| RedundantNullCheckSimple.cpp:19:3:19:9 | Store: ... = ... | semmle.label | ... = ... |
20+
| RedundantNullCheckSimple.cpp:19:7:19:9 | Load: * ... | semmle.label | * ... |
21+
| RedundantNullCheckSimple.cpp:19:8:19:9 | Load: * ... | semmle.label | * ... |
22+
| RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... | semmle.label | * ... |
23+
| RedundantNullCheckSimple.cpp:20:8:20:8 | Load: p | semmle.label | p |
24+
| RedundantNullCheckSimple.cpp:20:8:20:8 | VariableAddress: p | semmle.label | p |
25+
| RedundantNullCheckSimple.cpp:78:3:78:3 | VariableAddress: x | semmle.label | x |
26+
| RedundantNullCheckSimple.cpp:78:3:78:10 | Store: ... = ... | semmle.label | ... = ... |
27+
| RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | semmle.label | * ... |
28+
| RedundantNullCheckSimple.cpp:78:8:78:10 | Load: * ... | semmle.label | * ... |
29+
| RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... | semmle.label | * ... |
30+
| RedundantNullCheckSimple.cpp:79:8:79:9 | Load: pp | semmle.label | pp |
31+
| RedundantNullCheckSimple.cpp:79:8:79:9 | VariableAddress: pp | semmle.label | pp |
32+
| RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | semmle.label | * ... |
33+
| RedundantNullCheckSimple.cpp:92:13:92:18 | Store: * ... | semmle.label | * ... |
34+
| RedundantNullCheckSimple.cpp:92:18:92:18 | Load: p | semmle.label | p |
35+
| RedundantNullCheckSimple.cpp:93:9:93:10 | Load: sp | semmle.label | sp |
36+
| RedundantNullCheckSimple.cpp:93:9:93:10 | VariableAddress: sp | semmle.label | sp |
37+
| RedundantNullCheckSimple.cpp:93:13:93:13 | FieldAddress: p | semmle.label | p |
38+
| RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p | semmle.label | p |
39+
edges
40+
| RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p |
41+
| RedundantNullCheckSimple.cpp:3:8:3:8 | Load: p | RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p |
42+
| RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p |
43+
| RedundantNullCheckSimple.cpp:10:12:10:12 | Load: p | RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p |
44+
| RedundantNullCheckSimple.cpp:19:7:19:9 | Load: * ... | RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... |
45+
| RedundantNullCheckSimple.cpp:19:8:19:9 | Load: * ... | RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... |
46+
| RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... |
47+
| RedundantNullCheckSimple.cpp:78:8:78:10 | Load: * ... | RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... |
48+
| RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p |
49+
| RedundantNullCheckSimple.cpp:92:18:92:18 | Load: p | RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p |
50+
#select
51+
| RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | the value is dereferenced |
52+
| RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | the value is dereferenced |
53+
| RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... | RedundantNullCheckSimple.cpp:19:7:19:9 | Load: * ... | RedundantNullCheckSimple.cpp:20:7:20:8 | Load: * ... | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:19:7:19:9 | Load: * ... | the value is dereferenced |
54+
| RedundantNullCheckSimple.cpp:48:12:48:12 | Load: p | RedundantNullCheckSimple.cpp:51:10:51:11 | Load: * ... | RedundantNullCheckSimple.cpp:48:12:48:12 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:51:10:51:11 | Load: * ... | the value is dereferenced |
55+
| RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... | RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | the value is dereferenced |
56+
| RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p | RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p | This null check is redundant because $@ in any case. | RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | the value is dereferenced |

0 commit comments

Comments
 (0)