Skip to content

Commit 1ae52b6

Browse files
MathiasVPMathiasVP
committed
C++: Speedup 'MissingCheckScanf'.
1 parent 664fdc3 commit 1ae52b6

File tree

1 file changed

+180
-77
lines changed

1 file changed

+180
-77
lines changed

‎cpp/ql/src/Critical/MissingCheckScanf.ql

Lines changed: 180 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -20,81 +20,187 @@ import semmle.code.cpp.dataflow.DataFlow
2020
import semmle.code.cpp.ir.IR
2121
import semmle.code.cpp.ir.ValueNumbering
2222

23-
/** An expression appearing as an output argument to a `scanf`-like call */
24-
class ScanfOutput extends Expr {
25-
ScanfFunctionCall call;
26-
int varargIndex;
27-
Instruction instr;
28-
ValueNumber valNum;
29-
30-
ScanfOutput() {
31-
this = call.getOutputArgument(varargIndex).getFullyConverted() and
32-
instr.getConvertedResultExpression() = this and
33-
valueNumber(instr) = valNum
34-
}
35-
36-
ScanfFunctionCall getCall() { result = call }
37-
38-
/**
39-
* Returns the smallest possible `scanf` return value that would indicate
40-
* success in writing this output argument.
41-
*/
42-
int getMinimumGuardConstant() {
43-
result =
44-
varargIndex + 1 -
45-
count(ScanfFormatLiteral f, int n |
46-
// Special case: %n writes to an argument without reading any input.
47-
// It does not increase the count returned by `scanf`.
48-
n <= varargIndex and f.getUse() = call and f.getConversionChar(n) = "n"
49-
)
50-
}
51-
52-
predicate hasGuardedAccess(Access e, boolean isGuarded) {
53-
e = this.getAnAccess() and
54-
if
55-
exists(int value, int minGuard | minGuard = this.getMinimumGuardConstant() |
56-
e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
57-
or
58-
e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
59-
or
60-
e.getBasicBlock() = blockGuardedBy(value, "<=", call) and minGuard <= value
61-
)
62-
then isGuarded = true
63-
else isGuarded = false
64-
}
65-
66-
/**
67-
* Get a subsequent access of the same underlying storage,
68-
* but before it gets reset or reused in another `scanf` call.
69-
*/
70-
Access getAnAccess() {
71-
exists(Instruction dst |
72-
this.bigStep() = dst and
73-
dst.getAst() = result and
74-
valueNumber(dst) = valNum
23+
/**
24+
* Holds if `call` is a `scanf`-like function that may write to `output` at index `index`.
25+
*
26+
* Furthermore, `instr` is the instruction that defines the address of the `index`'th argument
27+
* of `call`, and `vn` is the value number of `instr.`
28+
*/
29+
predicate isSource(ScanfFunctionCall call, int index, Instruction instr, ValueNumber vn, Expr output) {
30+
output = call.getOutputArgument(index).getFullyConverted() and
31+
instr.getConvertedResultExpression() = output and
32+
vn.getAnInstruction() = instr
33+
}
34+
35+
/**
36+
* Holds if `instr` is control-flow reachable in 0 or more steps from an a call
37+
* to a call to a `scanf`-like function.
38+
*/
39+
pragma[nomagic]
40+
predicate fwdFlow0(Instruction instr) {
41+
isSource(_, _, instr, _, _)
42+
or
43+
exists(Instruction prev |
44+
fwdFlow0(prev) and
45+
prev.getASuccessor() = instr
46+
)
47+
}
48+
49+
/**
50+
* Holds if `instr` is part of the IR translation of `access` that
51+
* is not an expression being deallocated, and `instr` has value
52+
* number `vn`.
53+
*/
54+
predicate isSink(Instruction instr, Access access, ValueNumber vn) {
55+
instr.getAst() = access and
56+
not any(DeallocationExpr dealloc).getFreedExpr() = access and
57+
vn.getAnInstruction() = instr
58+
}
59+
60+
/**
61+
* Holds if `instr` is part of a path from a call to a `scanf`-like function
62+
* to a use of the written variable.
63+
*/
64+
pragma[nomagic]
65+
predicate revFlow0(Instruction instr) {
66+
fwdFlow0(instr) and
67+
(
68+
isSink(instr, _, _)
69+
or
70+
exists(Instruction succ | revFlow0(succ) | instr.getASuccessor() = succ)
71+
)
72+
}
73+
74+
/**
75+
* Holds if `instr` is part of a path from a call `to a `scanf`-like function
76+
* that writes to a variable with value number `vn`, without passing through
77+
* redefinitions of the variable.
78+
*/
79+
pragma[nomagic]
80+
private predicate fwdFlow(Instruction instr, ValueNumber vn) {
81+
revFlow0(instr) and
82+
(
83+
isSource(_, _, instr, vn, _)
84+
or
85+
exists(Instruction prev |
86+
fwdFlow(prev, vn) and
87+
prev.getASuccessor() = instr and
88+
not isBarrier(instr, vn)
89+
)
90+
)
91+
}
92+
93+
/**
94+
* Holds if `instr` is part of a path from a call `to a `scanf`-like function
95+
* that writes to a variable with value number `vn`, without passing through
96+
* redefinitions of the variable.
97+
*/
98+
pragma[nomagic]
99+
predicate revFlow(Instruction instr, ValueNumber vn) {
100+
fwdFlow(instr, vn) and
101+
(
102+
isSink(instr, _, vn)
103+
or
104+
exists(Instruction succ | revFlow(succ, vn) |
105+
instr.getASuccessor() = succ and
106+
not isBarrier(succ, vn)
75107
)
76-
}
108+
)
109+
}
77110

78-
private Instruction bigStep() {
79-
result = this.smallStep(instr)
111+
/**
112+
* A type that bundles together a reachable instruction with the appropriate
113+
* value number (i.e., the value number that's transferred from the source
114+
* to the sink).
115+
*/
116+
newtype TNode = MkNode(Instruction instr, ValueNumber vn) { revFlow(instr, vn) }
117+
118+
class Node extends MkNode {
119+
ValueNumber vn;
120+
Instruction instr;
121+
122+
Node() { this = MkNode(instr, vn) }
123+
124+
final string toString() { result = instr.toString() }
125+
126+
final Node getASuccessor() { result = MkNode(instr.getASuccessor(), vn) }
127+
128+
final Location getLocation() { result = instr.getLocation() }
129+
}
130+
131+
/**
132+
* Holds if `instr` is an instruction with value number `vn` that is
133+
* used in a store operation, or is overwritten by another call to
134+
* a `scanf`-like function.
135+
*/
136+
private predicate isBarrier(Instruction instr, ValueNumber vn) {
137+
// We only need to compute barriers for instructions that we
138+
// managed to hit during the initial flow stage.
139+
revFlow0(pragma[only_bind_into](instr)) and
140+
valueNumber(instr) = vn and
141+
exists(Expr e | instr.getAst() = e |
142+
instr = any(StoreInstruction s).getDestinationAddress()
80143
or
81-
exists(Instruction i | i = this.bigStep() | result = this.smallStep(i))
82-
}
83-
84-
private Instruction smallStep(Instruction i) {
85-
instr.getASuccessor*() = i and
86-
i.getASuccessor() = result and
87-
not this.isBarrier(result)
88-
}
89-
90-
private predicate isBarrier(Instruction i) {
91-
valueNumber(i) = valNum and
92-
exists(Expr e | i.getAst() = e |
93-
i = any(StoreInstruction s).getDestinationAddress()
144+
isSource(_, _, _, _, [e, e.getParent().(AddressOfExpr)])
145+
)
146+
}
147+
148+
/** Holds if `n1` steps to `n2` in a single step. */
149+
predicate isSuccessor(Node n1, Node n2) { n1.getASuccessor() = n2 }
150+
151+
predicate hasFlow(Node n1, Node n2) = fastTC(isSuccessor/2)(n1, n2)
152+
153+
Node getNode(Instruction instr, ValueNumber vn) { result = MkNode(instr, vn) }
154+
155+
/**
156+
* Holds if `source` is the `index`'th argument to the `scanf`-like call `call`, and `sink` is
157+
* an instruction that is part of the translation of `access` which is a transitive
158+
* control-flow successor of `call`.
159+
*
160+
* Furthermore, `source` and `sink` have identical global value numbers.
161+
*/
162+
predicate hasFlow(
163+
Instruction source, ScanfFunctionCall call, int index, Instruction sink, Access access
164+
) {
165+
exists(ValueNumber vn |
166+
isSource(call, index, source, vn, _) and
167+
hasFlow(getNode(source, vn), getNode(sink, vn)) and
168+
isSink(sink, access, vn)
169+
)
170+
}
171+
172+
/**
173+
* Gets the smallest possible `scanf` return value of `call` that would indicate
174+
* success in writing the output argument at index `index`.
175+
*/
176+
int getMinimumGuardConstant(ScanfFunctionCall call, int index) {
177+
isSource(call, index, _, _, _) and
178+
result =
179+
index + 1 -
180+
count(ScanfFormatLiteral f, int n |
181+
// Special case: %n writes to an argument without reading any input.
182+
// It does not increase the count returned by `scanf`.
183+
n <= index and f.getUse() = call and f.getConversionChar(n) = "n"
184+
)
185+
}
186+
187+
/**
188+
* Holds the access to `e` isn't guarded by a check that ensures that `call` returned
189+
* at least `minGuard`.
190+
*/
191+
predicate hasNonGuardedAccess(ScanfFunctionCall call, Access e, int minGuard) {
192+
exists(int index |
193+
hasFlow(_, call, index, _, e) and
194+
minGuard = getMinimumGuardConstant(call, index)
195+
|
196+
not exists(int value |
197+
e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
94198
or
95-
[e, e.getParent().(AddressOfExpr)] instanceof ScanfOutput
199+
e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
200+
or
201+
e.getBasicBlock() = blockGuardedBy(value, "<=", call) and minGuard <= value
96202
)
97-
}
203+
)
98204
}
99205

100206
/** Returns a block guarded by the assertion of `value op call` */
@@ -112,12 +218,9 @@ BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
112218
)
113219
}
114220

115-
from ScanfOutput output, ScanfFunctionCall call, Access access
116-
where
117-
output.getCall() = call and
118-
output.hasGuardedAccess(access, false) and
119-
not exists(DeallocationExpr dealloc | dealloc.getFreedExpr() = access)
221+
from ScanfFunctionCall call, Access access, int minGuard
222+
where hasNonGuardedAccess(call, access, minGuard)
120223
select access,
121224
"This variable is read, but may not have been written. " +
122-
"It should be guarded by a check that the $@ returns at least " +
123-
output.getMinimumGuardConstant() + ".", call, call.toString()
225+
"It should be guarded by a check that the $@ returns at least " + minGuard + ".", call,
226+
call.toString()

0 commit comments

Comments
 (0)