Skip to content

Commit 69a173c

Browse files
hvitvedhvitved
committed
wip
1 parent 21201f1 commit 69a173c

File tree

12 files changed

+398
-227
lines changed

12 files changed

+398
-227
lines changed

‎rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll

+49-13
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,41 @@ private import Cfg
66
private import codeql.rust.controlflow.internal.ControlFlowGraphImpl as ControlFlowGraphImpl
77
private import codeql.ssa.Ssa as SsaImplCommon
88

9+
/** Holds if `v` is introduced like `let v : i64;`. */
10+
private predicate isUnitializedLet(IdentPat pat, Variable v) {
11+
pat = v.getPat() and
12+
exists(LetStmt let |
13+
let = v.getLetStmt() and
14+
not let.hasInitializer()
15+
)
16+
}
17+
18+
/** Holds if `write` writes to variable `v`. */
19+
predicate variableWrite(AstNode write, Variable v) {
20+
exists(IdentPat pat |
21+
pat = write and
22+
pat = v.getPat() and
23+
not isUnitializedLet(pat, v)
24+
)
25+
or
26+
exists(VariableAccess access |
27+
access = write and
28+
access.getVariable() = v
29+
|
30+
access instanceof VariableWriteAccess
31+
or
32+
access = any(CompoundAssignmentExpr cae).getLhs()
33+
)
34+
}
35+
936
private Expr withParens(Expr e) {
1037
result = e
1138
or
1239
result.(ParenExpr).getExpr() = withParens(e)
1340
}
1441

15-
private predicate isUncertainWrite(VariableAccess va) {
42+
private predicate isRefTarget(VariableAccess va, Variable v) {
43+
va = v.getAnAccess() and
1644
exists(RefExpr re, Expr arg |
1745
va = re.getExpr() and // todo: restrict to `mut`
1846
arg = withParens(re)
@@ -24,8 +52,6 @@ private predicate isUncertainWrite(VariableAccess va) {
2452
arg = mce.getReceiver()
2553
)
2654
)
27-
or
28-
va = any(CompoundAssignmentExpr cae).getLhs()
2955
}
3056

3157
module SsaInput implements SsaImplCommon::InputSig<Location> {
@@ -45,9 +71,9 @@ module SsaInput implements SsaImplCommon::InputSig<Location> {
4571
forall(VariableAccess va | va = this.getAnAccess() |
4672
va instanceof VariableReadAccess
4773
or
48-
va instanceof VariableWriteAccess
74+
variableWrite(va, this)
4975
or
50-
isUncertainWrite(va)
76+
isRefTarget(va, this)
5177
)
5278
}
5379
}
@@ -61,16 +87,30 @@ module SsaInput implements SsaImplCommon::InputSig<Location> {
6187
certain = true
6288
or
6389
exists(VariableAccess va |
64-
isUncertainWrite(va) and
90+
isRefTarget(va, v) and
6591
va = bb.getNode(i).getAstNode() and
66-
v = va.getVariable() and
6792
certain = false
6893
)
6994
}
7095

7196
predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) {
72-
bb.getNode(i).getAstNode() = v.getAnAccess().(VariableReadAccess) and
97+
exists(VariableAccess va |
98+
bb.getNode(i).getAstNode() = va and
99+
va = v.getAnAccess()
100+
|
101+
va instanceof VariableReadAccess
102+
or
103+
// although compound assignments, like `x += y`, may in fact not update `x`,
104+
// it makes sense to treat them as such
105+
va = any(CompoundAssignmentExpr cae).getLhs()
106+
) and
73107
certain = true
108+
or
109+
exists(VariableAccess va |
110+
isRefTarget(va, v) and
111+
va = bb.getNode(i).getAstNode() and
112+
certain = false
113+
)
74114
}
75115
}
76116

@@ -185,11 +225,7 @@ private module Cached {
185225
cached
186226
predicate variableWriteActual(BasicBlock bb, int i, Variable v, CfgNode write) {
187227
bb.getNode(i) = write and
188-
(
189-
write.getAstNode() = v.getPat()
190-
or
191-
write.getAstNode().(VariableWriteAccess).getVariable() = v
192-
)
228+
variableWrite(write.getAstNode(), v)
193229
}
194230

195231
cached

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

+67-51
Original file line numberDiff line numberDiff line change
@@ -113,16 +113,17 @@ module Impl {
113113
*/
114114
IdentPat getPat() { variableDecl(definingNode, result, name) }
115115

116+
/** Gets the `let` statement that introduces this variable, if any. */
117+
LetStmt getLetStmt() { this.getPat() = result.getPat() }
118+
116119
/** Gets the initial value of this variable, if any. */
117-
Expr getInitializer() {
118-
exists(LetStmt let |
119-
this.getPat() = let.getPat() and
120-
result = let.getInitializer()
121-
)
122-
}
120+
Expr getInitializer() { result = this.getLetStmt().getInitializer() }
123121

124122
/** Holds if this variable is captured. */
125123
predicate isCaptured() { this.getAnAccess().isCapture() }
124+
125+
/** Gets the parameter that introduces this variable, if any. */
126+
Param getParameter() { parameterDeclInScope(result, this, _) }
126127
}
127128

128129
/** A path expression that may access a local variable. */
@@ -175,6 +176,27 @@ module Impl {
175176
)
176177
}
177178

179+
/**
180+
* Holds if parameter `p` introduces the variable `v` inside variable scope
181+
* `scope`.
182+
*/
183+
private predicate parameterDeclInScope(Param p, Variable v, VariableScope scope) {
184+
exists(Pat pat |
185+
pat = getAVariablePatAncestor(v) and
186+
p.getPat() = pat
187+
|
188+
exists(Function f |
189+
f.getParamList().getAParam() = p and
190+
scope = f.getBody()
191+
)
192+
or
193+
exists(ClosureExpr ce |
194+
ce.getParamList().getAParam() = p and
195+
scope = ce.getBody()
196+
)
197+
)
198+
}
199+
178200
/**
179201
* Holds if `v` is named `name` and is declared inside variable scope
180202
* `scope`, and `v` is bound starting from `(line, column)`.
@@ -183,51 +205,44 @@ module Impl {
183205
Variable v, VariableScope scope, string name, int line, int column
184206
) {
185207
name = v.getName() and
186-
exists(Pat pat | pat = getAVariablePatAncestor(v) |
187-
scope =
188-
any(MatchArmScope arm |
189-
arm.getPat() = pat and
190-
arm.getLocation().hasLocationInfo(_, line, column, _, _)
191-
)
192-
or
193-
exists(Function f |
194-
f.getParamList().getAParam().getPat() = pat and
195-
scope = f.getBody() and
196-
scope.getLocation().hasLocationInfo(_, line, column, _, _)
197-
)
198-
or
199-
exists(LetStmt let |
200-
let.getPat() = pat and
201-
scope = getEnclosingScope(let) and
202-
// for `let` statements, variables are bound _after_ the statement, i.e.
203-
// not in the RHS
204-
let.getLocation().hasLocationInfo(_, _, _, line, column)
205-
)
206-
or
207-
exists(IfExpr ie, LetExpr let |
208-
let.getPat() = pat and
209-
ie.getCondition() = let and
210-
scope = ie.getThen() and
211-
scope.getLocation().hasLocationInfo(_, line, column, _, _)
212-
)
213-
or
214-
exists(ForExpr fe |
215-
fe.getPat() = pat and
216-
scope = fe.getLoopBody() and
217-
scope.getLocation().hasLocationInfo(_, line, column, _, _)
218-
)
219-
or
220-
exists(ClosureExpr ce |
221-
ce.getParamList().getAParam().getPat() = pat and
222-
scope = ce.getBody() and
223-
scope.getLocation().hasLocationInfo(_, line, column, _, _)
224-
)
208+
(
209+
parameterDeclInScope(_, v, scope) and
210+
scope.getLocation().hasLocationInfo(_, line, column, _, _)
225211
or
226-
exists(WhileExpr we, LetExpr let |
227-
let.getPat() = pat and
228-
we.getCondition() = let and
229-
scope = we.getLoopBody() and
230-
scope.getLocation().hasLocationInfo(_, line, column, _, _)
212+
exists(Pat pat | pat = getAVariablePatAncestor(v) |
213+
scope =
214+
any(MatchArmScope arm |
215+
arm.getPat() = pat and
216+
arm.getLocation().hasLocationInfo(_, line, column, _, _)
217+
)
218+
or
219+
exists(LetStmt let |
220+
let.getPat() = pat and
221+
scope = getEnclosingScope(let) and
222+
// for `let` statements, variables are bound _after_ the statement, i.e.
223+
// not in the RHS
224+
let.getLocation().hasLocationInfo(_, _, _, line, column)
225+
)
226+
or
227+
exists(IfExpr ie, LetExpr let |
228+
let.getPat() = pat and
229+
ie.getCondition() = let and
230+
scope = ie.getThen() and
231+
scope.getLocation().hasLocationInfo(_, line, column, _, _)
232+
)
233+
or
234+
exists(ForExpr fe |
235+
fe.getPat() = pat and
236+
scope = fe.getLoopBody() and
237+
scope.getLocation().hasLocationInfo(_, line, column, _, _)
238+
)
239+
or
240+
exists(WhileExpr we, LetExpr let |
241+
let.getPat() = pat and
242+
we.getCondition() = let and
243+
scope = we.getLoopBody() and
244+
scope.getLocation().hasLocationInfo(_, line, column, _, _)
245+
)
231246
)
232247
)
233248
}
@@ -422,7 +437,8 @@ module Impl {
422437
exists(Expr mid |
423438
assignmentExprDescendant(mid) and
424439
getImmediateParent(e) = mid and
425-
not mid.(PrefixExpr).getOperatorName() = "*"
440+
not mid.(PrefixExpr).getOperatorName() = "*" and
441+
not mid instanceof FieldExpr
426442
)
427443
}
428444

‎rust/ql/src/queries/unusedentities/UnusedValue.ql

+13-3
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,17 @@
99
*/
1010

1111
import rust
12+
import codeql.rust.controlflow.ControlFlowGraph
13+
import codeql.rust.dataflow.Ssa
14+
import codeql.rust.dataflow.internal.SsaImpl
15+
import UnusedVariable
1216

13-
from Locatable e
14-
where none() // TODO: implement query
15-
select e, "Variable is assigned a value that is never used."
17+
from AstNode write, Ssa::Variable v
18+
where
19+
variableWrite(write, v) and
20+
// SSA definitions are only created for live writes
21+
not write = any(Ssa::WriteDefinition def).getWriteAccess().getAstNode() and
22+
// avoid overlap with the unused variable query
23+
not isUnused(v) and
24+
not v instanceof DiscardVariable
25+
select write, "Variable is assigned a value that is never used."

‎rust/ql/src/queries/unusedentities/UnusedVariable.ql

+2-5
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,8 @@
99
*/
1010

1111
import rust
12+
import UnusedVariable
1213

1314
from Variable v
14-
where
15-
not exists(v.getAnAccess()) and
16-
not exists(v.getInitializer()) and
17-
not v.getName().charAt(0) = "_" and
18-
exists(File f | f.getBaseName() = "main.rs" | v.getLocation().getFile() = f) // temporarily severely limit results
15+
where isUnused(v)
1916
select v, "Variable is not used."

‎rust/ql/src/queries/unusedentities/UnusedVariable.qll

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import rust
2+
3+
/** A deliberately unused variable. */
4+
class DiscardVariable extends Variable {
5+
DiscardVariable() { this.getName().charAt(0) = "_" }
6+
}
7+
8+
/** Holds if variable `v` is unused. */
9+
predicate isUnused(Variable v) {
10+
not exists(v.getAnAccess()) and
11+
not exists(v.getInitializer()) and
12+
not v instanceof DiscardVariable and
13+
exists(File f | f.getBaseName() = "main.rs" | v.getLocation().getFile() = f) // temporarily severely limit results
14+
}

0 commit comments

Comments
 (0)