Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust: Avoid location based variable analysis #18462

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+151 −43
Draft

Rust: Avoid location based variable analysis #18462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 100 additions & 35 deletions rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ module Impl {

/**
* A path expression that may access a local variable. These are paths that
* only consists of a simple name (i.e., without generic arguments,
* only consist of a simple name (i.e., without generic arguments,
* qualifiers, etc.).
*/
private class VariableAccessCand extends PathExprBase {
Expand Down Expand Up @@ -231,80 +231,151 @@ module Impl {
)
}

private Element getImmediateChildMin(Element e, int index) {
// A child may have multiple positions for different accessors,
// so always use the first
result = getImmediateChildAndAccessor(e, index, _) and
index = min(int i | result = getImmediateChildAndAccessor(e, i, _) | i)
}

private Element getImmediateChild(Element e, int index) {
result = rank[index + 1](Element res, int i | res = getImmediateChildMin(e, i) | res order by i)
}

pragma[nomagic]
private Element getImmediateLastChild(Element e) {
exists(int last |
result = getImmediateChild(e, last) and
not exists(getImmediateChild(e, last + 1))
)
}

/**
* Gets the pre-order numbering of `n`, where the immediately enclosing
* variable scope of `n` is `scope`.
*/
pragma[nomagic]
private int getPreOrderNumbering(VariableScope scope, Element n) {
n = scope and
result = 0
or
exists(Element parent |
not parent instanceof VariableScope
or
parent = scope
|
// first child of a previously numbered node
result = getPreOrderNumbering(scope, parent) + 1 and
n = getImmediateChild(parent, 0)
or
// non-first child of a previously numbered node
exists(Element child, int i |
result = getLastPreOrderNumbering(scope, child) + 1 and
child = getImmediateChild(parent, i) and
n = getImmediateChild(parent, i + 1)
)
)
}

/**
* Gets the pre-order numbering of the _last_ node nested under `n`, where the
* immediately enclosing variable scope of `n` (and the last node) is `scope`.
*/
pragma[nomagic]
private int getLastPreOrderNumbering(VariableScope scope, Element n) {
exists(Element leaf |
result = getPreOrderNumbering(scope, leaf) and
leaf != scope and
(
not exists(getImmediateChild(leaf, _))
or
leaf instanceof VariableScope
)
|
n = leaf
or
getImmediateLastChild(n) = leaf and
not n instanceof VariableScope
)
or
exists(Element mid |
mid = getImmediateLastChild(n) and
result = getLastPreOrderNumbering(scope, mid) and
not mid instanceof VariableScope and
not n instanceof VariableScope
)
}

/**
* Holds if `v` is named `name` and is declared inside variable scope
* `scope`, and `v` is bound starting from `(line, column)`.
* `scope`. The pre-order numbering of the binding site of `v`, amongst
* all nodes nester under `scope`, is `ord`.
*/
private predicate variableDeclInScope(
Variable v, VariableScope scope, string name, int line, int column
) {
private predicate variableDeclInScope(Variable v, VariableScope scope, string name, int ord) {
name = v.getName() and
(
parameterDeclInScope(v, scope) and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
or
exists(Pat pat | pat = getAVariablePatAncestor(v) |
scope =
any(MatchArmScope arm |
arm.getPat() = pat and
arm.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, arm)
)
or
exists(LetStmt let |
let.getPat() = pat and
scope = getEnclosingScope(let) and
// for `let` statements, variables are bound _after_ the statement, i.e.
// not in the RHS
let.getLocation().hasLocationFileInfo(_, _, _, line, column)
ord = getLastPreOrderNumbering(scope, let) + 1
)
or
exists(IfExpr ie, LetExpr let |
let.getPat() = pat and
ie.getCondition() = let and
scope = ie.getThen() and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
)
or
exists(ForExpr fe |
fe.getPat() = pat and
scope = fe.getLoopBody() and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
)
or
exists(WhileExpr we, LetExpr let |
let.getPat() = pat and
we.getCondition() = let and
scope = we.getLoopBody() and
scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
ord = getPreOrderNumbering(scope, scope)
)
)
)
}

/**
* Holds if `cand` may access a variable named `name` at
* `(startline, startcolumn, endline, endcolumn)` in the variable scope
* `scope`.
* Holds if `cand` may access a variable named `name` at pre-order number `ord`
* in the variable scope `scope`.
*
* `nestLevel` is the number of nested scopes that need to be traversed
* to reach `scope` from `cand`.
*/
private predicate variableAccessCandInScope(
VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int startline,
int startcolumn, int endline, int endcolumn
VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int ord
) {
name = cand.getName() and
scope = [cand.(VariableScope), getEnclosingScope(cand)] and
cand.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn) and
ord = getPreOrderNumbering(scope, cand) and
nestLevel = 0
or
exists(VariableScope inner |
variableAccessCandInScope(cand, inner, name, nestLevel - 1, _, _, _, _) and
variableAccessCandInScope(cand, inner, name, nestLevel - 1, _) and
scope = getEnclosingScope(inner) and
// Use the location of the inner scope as the location of the access, instead of the
// actual access location. This allows us to collapse multiple accesses in inner
// scopes to a single entity
inner.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn)
// Use the pre-order number of the inner scope as the number of the access. This allows
// us to collapse multiple accesses in inner scopes to a single entity
ord = getPreOrderNumbering(scope, inner)
)
}

Expand Down Expand Up @@ -375,15 +446,10 @@ module Impl {
}

pragma[nomagic]
predicate rankBy(
string name, VariableScope scope, int startline, int startcolumn, int endline, int endcolumn
) {
variableDeclInScope(this.asVariable(), scope, name, startline, startcolumn) and
endline = -1 and
endcolumn = -1
predicate rankBy(string name, VariableScope scope, int ord) {
variableDeclInScope(this.asVariable(), scope, name, ord)
or
variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, startline, startcolumn,
endline, endcolumn)
variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, ord)
}
}

Expand All @@ -396,11 +462,10 @@ module Impl {

int getRank(VariableScope scope, string name, VariableOrAccessCand v) {
v =
rank[result](VariableOrAccessCand v0, int startline, int startcolumn, int endline,
int endcolumn |
v0.rankBy(name, scope, startline, startcolumn, endline, endcolumn)
rank[result](VariableOrAccessCand v0, int ord |
v0.rankBy(name, scope, ord)
|
v0 order by startline, startcolumn, endline, endcolumn
v0 order by ord
)
}
}
Expand Down Expand Up @@ -440,7 +505,7 @@ module Impl {
exists(int rnk |
variableReachesRank(scope, name, v, rnk) and
rnk = rankVariableOrAccess(scope, name, TVariableOrAccessCandVariableAccessCand(cand)) and
variableAccessCandInScope(cand, scope, name, nestLevel, _, _, _, _)
variableAccessCandInScope(cand, scope, name, nestLevel, _)
)
}

Expand Down
30 changes: 30 additions & 0 deletions rust/ql/test/library-tests/dataflow/strings/inline-taint-flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
models
| 1 | Summary: lang:alloc; <crate::string::String>::as_str; Argument[self]; ReturnValue; taint |
| 2 | Summary: lang:alloc; crate::fmt::format; Argument[0]; ReturnValue; taint |
| 3 | Summary: lang:core; crate::hint::must_use; Argument[0]; ReturnValue; value |
edges
| main.rs:26:9:26:9 | s | main.rs:27:19:27:25 | s[...] | provenance | |
| main.rs:26:13:26:22 | source(...) | main.rs:26:9:26:9 | s | provenance | |
Expand All @@ -27,6 +28,19 @@ edges
| main.rs:77:9:77:18 | formatted3 | main.rs:78:10:78:19 | formatted3 | provenance | |
| main.rs:77:22:77:75 | ...::format(...) | main.rs:77:9:77:18 | formatted3 | provenance | |
| main.rs:77:34:77:74 | MacroExpr | main.rs:77:22:77:75 | ...::format(...) | provenance | MaD:2 |
| main.rs:82:9:82:10 | s1 | main.rs:86:18:86:25 | MacroExpr | provenance | |
| main.rs:82:9:82:10 | s1 | main.rs:87:18:87:32 | MacroExpr | provenance | |
| main.rs:82:14:82:23 | source(...) | main.rs:82:9:82:10 | s1 | provenance | |
| main.rs:86:10:86:26 | res | main.rs:86:18:86:25 | { ... } | provenance | |
| main.rs:86:18:86:25 | ...::format(...) | main.rs:86:10:86:26 | res | provenance | |
| main.rs:86:18:86:25 | ...::must_use(...) | main.rs:86:10:86:26 | MacroExpr | provenance | |
| main.rs:86:18:86:25 | MacroExpr | main.rs:86:18:86:25 | ...::format(...) | provenance | MaD:2 |
| main.rs:86:18:86:25 | { ... } | main.rs:86:18:86:25 | ...::must_use(...) | provenance | MaD:3 |
| main.rs:87:10:87:33 | res | main.rs:87:18:87:32 | { ... } | provenance | |
| main.rs:87:18:87:32 | ...::format(...) | main.rs:87:10:87:33 | res | provenance | |
| main.rs:87:18:87:32 | ...::must_use(...) | main.rs:87:10:87:33 | MacroExpr | provenance | |
| main.rs:87:18:87:32 | MacroExpr | main.rs:87:18:87:32 | ...::format(...) | provenance | MaD:2 |
| main.rs:87:18:87:32 | { ... } | main.rs:87:18:87:32 | ...::must_use(...) | provenance | MaD:3 |
nodes
| main.rs:26:9:26:9 | s | semmle.label | s |
| main.rs:26:13:26:22 | source(...) | semmle.label | source(...) |
Expand Down Expand Up @@ -58,6 +72,20 @@ nodes
| main.rs:77:22:77:75 | ...::format(...) | semmle.label | ...::format(...) |
| main.rs:77:34:77:74 | MacroExpr | semmle.label | MacroExpr |
| main.rs:78:10:78:19 | formatted3 | semmle.label | formatted3 |
| main.rs:82:9:82:10 | s1 | semmle.label | s1 |
| main.rs:82:14:82:23 | source(...) | semmle.label | source(...) |
| main.rs:86:10:86:26 | MacroExpr | semmle.label | MacroExpr |
| main.rs:86:10:86:26 | res | semmle.label | res |
| main.rs:86:18:86:25 | ...::format(...) | semmle.label | ...::format(...) |
| main.rs:86:18:86:25 | ...::must_use(...) | semmle.label | ...::must_use(...) |
| main.rs:86:18:86:25 | MacroExpr | semmle.label | MacroExpr |
| main.rs:86:18:86:25 | { ... } | semmle.label | { ... } |
| main.rs:87:10:87:33 | MacroExpr | semmle.label | MacroExpr |
| main.rs:87:10:87:33 | res | semmle.label | res |
| main.rs:87:18:87:32 | ...::format(...) | semmle.label | ...::format(...) |
| main.rs:87:18:87:32 | ...::must_use(...) | semmle.label | ...::must_use(...) |
| main.rs:87:18:87:32 | MacroExpr | semmle.label | MacroExpr |
| main.rs:87:18:87:32 | { ... } | semmle.label | { ... } |
subpaths
testFailures
#select
Expand All @@ -67,3 +95,5 @@ testFailures
| 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(...) |
| 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(...) |
| 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(...) |
| 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(...) |
| 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(...) |
4 changes: 2 additions & 2 deletions rust/ql/test/library-tests/dataflow/strings/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ fn format_macro() {
let s2 = "2";
let s3 = "3";

sink(format!("{}", s1)); // $ MISSING: hasTaintFlow=34
sink(format!("{s1} and {s3}")); // $ MISSING: hasTaintFlow=34
sink(format!("{}", s1)); // $ hasTaintFlow=34
sink(format!("{s1} and {s3}")); // $ hasTaintFlow=34
sink(format!("{s2} and {s3}"));
}

Expand Down
5 changes: 5 additions & 0 deletions rust/ql/test/library-tests/variables/Ssa.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ definition
| variables.rs:523:9:523:9 | z | variables.rs:523:9:523:9 | z |
| variables.rs:532:10:532:18 | SelfParam | variables.rs:532:15:532:18 | self |
| variables.rs:554:9:554:22 | var_from_macro | variables.rs:554:9:554:22 | var_from_macro |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro |
read
| variables.rs:3:14:3:14 | s | variables.rs:3:14:3:14 | s | variables.rs:4:20:4:20 | s |
| variables.rs:7:14:7:14 | i | variables.rs:7:14:7:14 | i | variables.rs:8:20:8:20 | i |
Expand Down Expand Up @@ -284,6 +285,7 @@ read
| variables.rs:523:9:523:9 | z | variables.rs:523:9:523:9 | z | variables.rs:524:20:524:20 | z |
| variables.rs:532:10:532:18 | SelfParam | variables.rs:532:15:532:18 | self | variables.rs:533:6:533:9 | self |
| variables.rs:554:9:554:22 | var_from_macro | variables.rs:554:9:554:22 | var_from_macro | variables.rs:556:15:556:28 | var_from_macro |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro |
firstRead
| variables.rs:3:14:3:14 | s | variables.rs:3:14:3:14 | s | variables.rs:4:20:4:20 | s |
| variables.rs:7:14:7:14 | i | variables.rs:7:14:7:14 | i | variables.rs:8:20:8:20 | i |
Expand Down Expand Up @@ -395,6 +397,7 @@ firstRead
| variables.rs:523:9:523:9 | z | variables.rs:523:9:523:9 | z | variables.rs:524:20:524:20 | z |
| variables.rs:532:10:532:18 | SelfParam | variables.rs:532:15:532:18 | self | variables.rs:533:6:533:9 | self |
| variables.rs:554:9:554:22 | var_from_macro | variables.rs:554:9:554:22 | var_from_macro | variables.rs:556:15:556:28 | var_from_macro |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro |
lastRead
| variables.rs:3:14:3:14 | s | variables.rs:3:14:3:14 | s | variables.rs:4:20:4:20 | s |
| variables.rs:7:14:7:14 | i | variables.rs:7:14:7:14 | i | variables.rs:8:20:8:20 | i |
Expand Down Expand Up @@ -507,6 +510,7 @@ lastRead
| variables.rs:523:9:523:9 | z | variables.rs:523:9:523:9 | z | variables.rs:524:20:524:20 | z |
| variables.rs:532:10:532:18 | SelfParam | variables.rs:532:15:532:18 | self | variables.rs:533:6:533:9 | self |
| variables.rs:554:9:554:22 | var_from_macro | variables.rs:554:9:554:22 | var_from_macro | variables.rs:556:15:556:28 | var_from_macro |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro |
adjacentReads
| variables.rs:35:9:35:10 | x3 | variables.rs:35:9:35:10 | x3 | variables.rs:36:15:36:16 | x3 | variables.rs:38:9:38:10 | x3 |
| variables.rs:43:9:43:10 | x4 | variables.rs:43:9:43:10 | x4 | variables.rs:44:15:44:16 | x4 | variables.rs:49:15:49:16 | x4 |
Expand Down Expand Up @@ -659,3 +663,4 @@ assigns
| variables.rs:519:9:519:9 | x | variables.rs:519:13:519:14 | 16 |
| variables.rs:523:9:523:9 | z | variables.rs:523:13:523:14 | 17 |
| variables.rs:554:9:554:22 | var_from_macro | variables.rs:555:9:555:25 | MacroExpr |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:23:555:24 | 37 |
2 changes: 2 additions & 0 deletions rust/ql/test/library-tests/variables/variables.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ variableAccess
| variables.rs:533:6:533:9 | self | variables.rs:532:15:532:18 | self |
| variables.rs:539:3:539:3 | a | variables.rs:538:11:538:11 | a |
| variables.rs:541:13:541:13 | a | variables.rs:538:11:538:11 | a |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro |
| variables.rs:556:15:556:28 | var_from_macro | variables.rs:554:9:554:22 | var_from_macro |
variableWriteAccess
| variables.rs:23:5:23:6 | x2 | variables.rs:21:13:21:14 | x2 |
Expand Down Expand Up @@ -436,6 +437,7 @@ variableReadAccess
| variables.rs:533:6:533:9 | self | variables.rs:532:15:532:18 | self |
| variables.rs:539:3:539:3 | a | variables.rs:538:11:538:11 | a |
| variables.rs:541:13:541:13 | a | variables.rs:538:11:538:11 | a |
| variables.rs:555:9:555:25 | var_in_macro | variables.rs:555:9:555:25 | var_in_macro |
| variables.rs:556:15:556:28 | var_from_macro | variables.rs:554:9:554:22 | var_from_macro |
variableInitializer
| variables.rs:16:9:16:10 | x1 | variables.rs:16:14:16:16 | "a" |
Expand Down
16 changes: 11 additions & 5 deletions rust/ql/test/library-tests/variables/variables.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ query predicate capturedAccess(VariableAccess va) { va.isCapture() }
module VariableAccessTest implements TestSig {
string getARelevantTag() { result = ["", "write_", "read_"] + "access" }

private predicate declAt(Variable v, string filepath, int line) {
v.getLocation().hasLocationInfo(filepath, _, _, line, _)
private predicate declAt(Variable v, string filepath, int line, boolean inMacro) {
v.getLocation().hasLocationInfo(filepath, _, _, line, _) and
if v.getPat().isInMacroExpansion() then inMacro = true else inMacro = false
}

private predicate commmentAt(string text, string filepath, int line) {
Expand All @@ -30,10 +31,15 @@ module VariableAccessTest implements TestSig {
}

private predicate decl(Variable v, string value) {
exists(string filepath, int line | declAt(v, filepath, line) |
commmentAt(value, filepath, line)
exists(string filepath, int line, boolean inMacro | declAt(v, filepath, line, inMacro) |
commmentAt(value, filepath, line) and
inMacro = false
or
not commmentAt(_, filepath, line) and
(
not commmentAt(_, filepath, line)
or
inMacro = true
) and
value = v.getName()
)
}
Expand Down
2 changes: 1 addition & 1 deletion rust/ql/test/library-tests/variables/variables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ macro_rules! let_in_macro {

fn macro_invocation() {
let var_from_macro =
let_in_macro!(37); // $ MISSING: read_access=var_in_macro
let_in_macro!(37); // $ read_access=var_in_macro
print_i64(var_from_macro); // $ read_access=var_from_macro
}

Expand Down
Loading