Skip to content

Commit 33e6d63

Browse files
hvitvedhvitved
committed
Rust: Avoid location-based variable analysis
1 parent d03b284 commit 33e6d63

File tree

10 files changed

+336
-187
lines changed

10 files changed

+336
-187
lines changed

‎rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

+120-35
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ module Impl {
165165

166166
/**
167167
* A path expression that may access a local variable. These are paths that
168-
* only consists of a simple name (i.e., without generic arguments,
168+
* only consist of a simple name (i.e., without generic arguments,
169169
* qualifiers, etc.).
170170
*/
171171
private class VariableAccessCand extends PathExprBase {
@@ -231,80 +231,169 @@ module Impl {
231231
)
232232
}
233233

234+
/** A subset of `Element`s for which we want to compute pre-order numbers. */
235+
private class RelevantElement extends Element {
236+
RelevantElement() {
237+
this instanceof VariableScope or
238+
this instanceof VariableAccessCand or
239+
this instanceof LetStmt or
240+
getImmediateChildAndAccessor(this, _, _) instanceof RelevantElement
241+
}
242+
243+
pragma[nomagic]
244+
private RelevantElement getChild(int index) {
245+
result = getImmediateChildAndAccessor(this, index, _)
246+
}
247+
248+
pragma[nomagic]
249+
private RelevantElement getImmediateChildMin(int index) {
250+
// A child may have multiple positions for different accessors,
251+
// so always use the first
252+
result = this.getChild(index) and
253+
index = min(int i | result = this.getChild(i) | i)
254+
}
255+
256+
pragma[nomagic]
257+
RelevantElement getImmediateChild(int index) {
258+
result =
259+
rank[index + 1](Element res, int i | res = this.getImmediateChildMin(i) | res order by i)
260+
}
261+
262+
pragma[nomagic]
263+
RelevantElement getImmediateLastChild() {
264+
exists(int last |
265+
result = this.getImmediateChild(last) and
266+
not exists(this.getImmediateChild(last + 1))
267+
)
268+
}
269+
}
270+
271+
/**
272+
* Gets the pre-order numbering of `n`, where the immediately enclosing
273+
* variable scope of `n` is `scope`.
274+
*/
275+
pragma[nomagic]
276+
private int getPreOrderNumbering(VariableScope scope, RelevantElement n) {
277+
n = scope and
278+
result = 0
279+
or
280+
exists(RelevantElement parent |
281+
not parent instanceof VariableScope
282+
or
283+
parent = scope
284+
|
285+
// first child of a previously numbered node
286+
result = getPreOrderNumbering(scope, parent) + 1 and
287+
n = parent.getImmediateChild(0)
288+
or
289+
// non-first child of a previously numbered node
290+
exists(RelevantElement child, int i |
291+
result = getLastPreOrderNumbering(scope, child) + 1 and
292+
child = parent.getImmediateChild(i) and
293+
n = parent.getImmediateChild(i + 1)
294+
)
295+
)
296+
}
297+
298+
/**
299+
* Gets the pre-order numbering of the _last_ node nested under `n`, where the
300+
* immediately enclosing variable scope of `n` (and the last node) is `scope`.
301+
*/
302+
pragma[nomagic]
303+
private int getLastPreOrderNumbering(VariableScope scope, RelevantElement n) {
304+
exists(RelevantElement leaf |
305+
result = getPreOrderNumbering(scope, leaf) and
306+
leaf != scope and
307+
(
308+
not exists(leaf.getImmediateChild(_))
309+
or
310+
leaf instanceof VariableScope
311+
)
312+
|
313+
n = leaf
314+
or
315+
n.getImmediateLastChild() = leaf and
316+
not n instanceof VariableScope
317+
)
318+
or
319+
exists(RelevantElement mid |
320+
mid = n.getImmediateLastChild() and
321+
result = getLastPreOrderNumbering(scope, mid) and
322+
not mid instanceof VariableScope and
323+
not n instanceof VariableScope
324+
)
325+
}
326+
234327
/**
235328
* Holds if `v` is named `name` and is declared inside variable scope
236-
* `scope`, and `v` is bound starting from `(line, column)`.
329+
* `scope`. The pre-order numbering of the binding site of `v`, amongst
330+
* all nodes nester under `scope`, is `ord`.
237331
*/
238-
private predicate variableDeclInScope(
239-
Variable v, VariableScope scope, string name, int line, int column
240-
) {
332+
private predicate variableDeclInScope(Variable v, VariableScope scope, string name, int ord) {
241333
name = v.getName() and
242334
(
243335
parameterDeclInScope(v, scope) and
244-
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
336+
ord = getPreOrderNumbering(scope, scope)
245337
or
246338
exists(Pat pat | pat = getAVariablePatAncestor(v) |
247339
scope =
248340
any(MatchArmScope arm |
249341
arm.getPat() = pat and
250-
arm.getLocation().hasLocationFileInfo(_, line, column, _, _)
342+
ord = getPreOrderNumbering(scope, arm)
251343
)
252344
or
253345
exists(LetStmt let |
254346
let.getPat() = pat and
255347
scope = getEnclosingScope(let) and
256348
// for `let` statements, variables are bound _after_ the statement, i.e.
257349
// not in the RHS
258-
let.getLocation().hasLocationFileInfo(_, _, _, line, column)
350+
ord = getLastPreOrderNumbering(scope, let) + 1
259351
)
260352
or
261353
exists(IfExpr ie, LetExpr let |
262354
let.getPat() = pat and
263355
ie.getCondition() = let and
264356
scope = ie.getThen() and
265-
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
357+
ord = getPreOrderNumbering(scope, scope)
266358
)
267359
or
268360
exists(ForExpr fe |
269361
fe.getPat() = pat and
270362
scope = fe.getLoopBody() and
271-
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
363+
ord = getPreOrderNumbering(scope, scope)
272364
)
273365
or
274366
exists(WhileExpr we, LetExpr let |
275367
let.getPat() = pat and
276368
we.getCondition() = let and
277369
scope = we.getLoopBody() and
278-
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
370+
ord = getPreOrderNumbering(scope, scope)
279371
)
280372
)
281373
)
282374
}
283375

284376
/**
285-
* Holds if `cand` may access a variable named `name` at
286-
* `(startline, startcolumn, endline, endcolumn)` in the variable scope
287-
* `scope`.
377+
* Holds if `cand` may access a variable named `name` at pre-order number `ord`
378+
* in the variable scope `scope`.
288379
*
289380
* `nestLevel` is the number of nested scopes that need to be traversed
290381
* to reach `scope` from `cand`.
291382
*/
292383
private predicate variableAccessCandInScope(
293-
VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int startline,
294-
int startcolumn, int endline, int endcolumn
384+
VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int ord
295385
) {
296386
name = cand.getName() and
297387
scope = [cand.(VariableScope), getEnclosingScope(cand)] and
298-
cand.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn) and
388+
ord = getPreOrderNumbering(scope, cand) and
299389
nestLevel = 0
300390
or
301391
exists(VariableScope inner |
302-
variableAccessCandInScope(cand, inner, name, nestLevel - 1, _, _, _, _) and
392+
variableAccessCandInScope(cand, inner, name, nestLevel - 1, _) and
303393
scope = getEnclosingScope(inner) and
304-
// Use the location of the inner scope as the location of the access, instead of the
305-
// actual access location. This allows us to collapse multiple accesses in inner
306-
// scopes to a single entity
307-
inner.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn)
394+
// Use the pre-order number of the inner scope as the number of the access. This allows
395+
// us to collapse multiple accesses in inner scopes to a single entity
396+
ord = getPreOrderNumbering(scope, inner)
308397
)
309398
}
310399

@@ -375,15 +464,12 @@ module Impl {
375464
}
376465

377466
pragma[nomagic]
378-
predicate rankBy(
379-
string name, VariableScope scope, int startline, int startcolumn, int endline, int endcolumn
380-
) {
381-
variableDeclInScope(this.asVariable(), scope, name, startline, startcolumn) and
382-
endline = -1 and
383-
endcolumn = -1
467+
predicate rankBy(string name, VariableScope scope, int ord, int kind) {
468+
variableDeclInScope(this.asVariable(), scope, name, ord) and
469+
kind = 0
384470
or
385-
variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, startline, startcolumn,
386-
endline, endcolumn)
471+
variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, ord) and
472+
kind = 1
387473
}
388474
}
389475

@@ -396,11 +482,10 @@ module Impl {
396482

397483
int getRank(VariableScope scope, string name, VariableOrAccessCand v) {
398484
v =
399-
rank[result](VariableOrAccessCand v0, int startline, int startcolumn, int endline,
400-
int endcolumn |
401-
v0.rankBy(name, scope, startline, startcolumn, endline, endcolumn)
485+
rank[result](VariableOrAccessCand v0, int ord, int kind |
486+
v0.rankBy(name, scope, ord, kind)
402487
|
403-
v0 order by startline, startcolumn, endline, endcolumn
488+
v0 order by ord, kind
404489
)
405490
}
406491
}
@@ -440,7 +525,7 @@ module Impl {
440525
exists(int rnk |
441526
variableReachesRank(scope, name, v, rnk) and
442527
rnk = rankVariableOrAccess(scope, name, TVariableOrAccessCandVariableAccessCand(cand)) and
443-
variableAccessCandInScope(cand, scope, name, nestLevel, _, _, _, _)
528+
variableAccessCandInScope(cand, scope, name, nestLevel, _)
444529
)
445530
}
446531

‎rust/ql/test/library-tests/dataflow/strings/inline-taint-flow.expected

+30
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
models
22
| 1 | Summary: lang:alloc; <crate::string::String>::as_str; Argument[self]; ReturnValue; taint |
33
| 2 | Summary: lang:alloc; crate::fmt::format; Argument[0]; ReturnValue; taint |
4+
| 3 | Summary: lang:core; crate::hint::must_use; Argument[0]; ReturnValue; value |
45
edges
56
| main.rs:26:9:26:9 | s | main.rs:27:19:27:25 | s[...] | provenance | |
67
| main.rs:26:13:26:22 | source(...) | main.rs:26:9:26:9 | s | provenance | |
@@ -27,6 +28,19 @@ edges
2728
| main.rs:77:9:77:18 | formatted3 | main.rs:78:10:78:19 | formatted3 | provenance | |
2829
| main.rs:77:22:77:75 | ...::format(...) | main.rs:77:9:77:18 | formatted3 | provenance | |
2930
| main.rs:77:34:77:74 | MacroExpr | main.rs:77:22:77:75 | ...::format(...) | provenance | MaD:2 |
31+
| main.rs:82:9:82:10 | s1 | main.rs:86:18:86:25 | MacroExpr | provenance | |
32+
| main.rs:82:9:82:10 | s1 | main.rs:87:18:87:32 | MacroExpr | provenance | |
33+
| main.rs:82:14:82:23 | source(...) | main.rs:82:9:82:10 | s1 | provenance | |
34+
| main.rs:86:10:86:26 | res | main.rs:86:18:86:25 | { ... } | provenance | |
35+
| main.rs:86:18:86:25 | ...::format(...) | main.rs:86:10:86:26 | res | provenance | |
36+
| main.rs:86:18:86:25 | ...::must_use(...) | main.rs:86:10:86:26 | MacroExpr | provenance | |
37+
| main.rs:86:18:86:25 | MacroExpr | main.rs:86:18:86:25 | ...::format(...) | provenance | MaD:2 |
38+
| main.rs:86:18:86:25 | { ... } | main.rs:86:18:86:25 | ...::must_use(...) | provenance | MaD:3 |
39+
| main.rs:87:10:87:33 | res | main.rs:87:18:87:32 | { ... } | provenance | |
40+
| main.rs:87:18:87:32 | ...::format(...) | main.rs:87:10:87:33 | res | provenance | |
41+
| main.rs:87:18:87:32 | ...::must_use(...) | main.rs:87:10:87:33 | MacroExpr | provenance | |
42+
| main.rs:87:18:87:32 | MacroExpr | main.rs:87:18:87:32 | ...::format(...) | provenance | MaD:2 |
43+
| main.rs:87:18:87:32 | { ... } | main.rs:87:18:87:32 | ...::must_use(...) | provenance | MaD:3 |
3044
nodes
3145
| main.rs:26:9:26:9 | s | semmle.label | s |
3246
| main.rs:26:13:26:22 | source(...) | semmle.label | source(...) |
@@ -58,6 +72,20 @@ nodes
5872
| main.rs:77:22:77:75 | ...::format(...) | semmle.label | ...::format(...) |
5973
| main.rs:77:34:77:74 | MacroExpr | semmle.label | MacroExpr |
6074
| main.rs:78:10:78:19 | formatted3 | semmle.label | formatted3 |
75+
| main.rs:82:9:82:10 | s1 | semmle.label | s1 |
76+
| main.rs:82:14:82:23 | source(...) | semmle.label | source(...) |
77+
| main.rs:86:10:86:26 | MacroExpr | semmle.label | MacroExpr |
78+
| main.rs:86:10:86:26 | res | semmle.label | res |
79+
| main.rs:86:18:86:25 | ...::format(...) | semmle.label | ...::format(...) |
80+
| main.rs:86:18:86:25 | ...::must_use(...) | semmle.label | ...::must_use(...) |
81+
| main.rs:86:18:86:25 | MacroExpr | semmle.label | MacroExpr |
82+
| main.rs:86:18:86:25 | { ... } | semmle.label | { ... } |
83+
| main.rs:87:10:87:33 | MacroExpr | semmle.label | MacroExpr |
84+
| main.rs:87:10:87:33 | res | semmle.label | res |
85+
| main.rs:87:18:87:32 | ...::format(...) | semmle.label | ...::format(...) |
86+
| main.rs:87:18:87:32 | ...::must_use(...) | semmle.label | ...::must_use(...) |
87+
| main.rs:87:18:87:32 | MacroExpr | semmle.label | MacroExpr |
88+
| main.rs:87:18:87:32 | { ... } | semmle.label | { ... } |
6189
subpaths
6290
testFailures
6391
#select
@@ -67,3 +95,5 @@ testFailures
6795
| main.rs:71:10:71:19 | formatted1 | main.rs:68:13:68:22 | source(...) | main.rs:71:10:71:19 | formatted1 | $@ | main.rs:68:13:68:22 | source(...) | source(...) |
6896
| main.rs:74:10:74:19 | formatted2 | main.rs:68:13:68:22 | source(...) | main.rs:74:10:74:19 | formatted2 | $@ | main.rs:68:13:68:22 | source(...) | source(...) |
6997
| main.rs:78:10:78:19 | formatted3 | main.rs:76:17:76:32 | source_usize(...) | main.rs:78:10:78:19 | formatted3 | $@ | main.rs:76:17:76:32 | source_usize(...) | source_usize(...) |
98+
| main.rs:86:10:86:26 | MacroExpr | main.rs:82:14:82:23 | source(...) | main.rs:86:10:86:26 | MacroExpr | $@ | main.rs:82:14:82:23 | source(...) | source(...) |
99+
| main.rs:87:10:87:33 | MacroExpr | main.rs:82:14:82:23 | source(...) | main.rs:87:10:87:33 | MacroExpr | $@ | main.rs:82:14:82:23 | source(...) | source(...) |

‎rust/ql/test/library-tests/dataflow/strings/main.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ fn format_macro() {
8383
let s2 = "2";
8484
let s3 = "3";
8585

86-
sink(format!("{}", s1)); // $ MISSING: hasTaintFlow=34
87-
sink(format!("{s1} and {s3}")); // $ MISSING: hasTaintFlow=34
86+
sink(format!("{}", s1)); // $ hasTaintFlow=34
87+
sink(format!("{s1} and {s3}")); // $ hasTaintFlow=34
8888
sink(format!("{s2} and {s3}"));
8989
}
9090

0 commit comments

Comments
 (0)