Skip to content

Commit 75746cb

Browse files
hvitvedhvitved
authored
Merge pull request github#12549 from hvitved/ruby/ssa-write-access
Ruby: `Ssa::WriteDefinition::getWriteAccess` should return a CFG node
2 parents ee01e9a + 1d0b3d4 commit 75746cb

File tree

11 files changed

+214
-156
lines changed

11 files changed

+214
-156
lines changed

‎ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

Lines changed: 58 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -756,16 +756,30 @@ module ExprNodes {
756756

757757
override VariableAccess e;
758758

759-
final override VariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
759+
override VariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
760+
761+
/** Gets the variable that is being accessed. */
762+
Variable getVariable() { result = this.getExpr().getVariable() }
760763
}
761764

762765
/** A control-flow node that wraps a `VariableReadAccess` AST expression. */
763-
class VariableReadAccessCfgNode extends ExprCfgNode {
766+
class VariableReadAccessCfgNode extends VariableAccessCfgNode {
764767
override string getAPrimaryQlClass() { result = "VariableReadAccessCfgNode" }
765768

766769
override VariableReadAccess e;
767770

768-
final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
771+
override VariableReadAccess getExpr() { result = VariableAccessCfgNode.super.getExpr() }
772+
}
773+
774+
/** A control-flow node that wraps a `LocalVariableReadAccess` AST expression. */
775+
class LocalVariableReadAccessCfgNode extends VariableReadAccessCfgNode {
776+
override string getAPrimaryQlClass() { result = "LocalVariableReadAccessCfgNode" }
777+
778+
override LocalVariableReadAccess e;
779+
780+
final override LocalVariableReadAccess getExpr() { result = super.getExpr() }
781+
782+
final override LocalVariable getVariable() { result = super.getVariable() }
769783
}
770784

771785
private class InstanceVariableAccessMapping extends ExprChildMapping, InstanceVariableAccess {
@@ -791,21 +805,57 @@ module ExprNodes {
791805
}
792806

793807
/** A control-flow node that wraps a `SelfVariableAccess` AST expression. */
794-
class SelfVariableAccessCfgNode extends ExprCfgNode {
808+
class SelfVariableAccessCfgNode extends VariableAccessCfgNode {
795809
final override string getAPrimaryQlClass() { result = "SelfVariableAccessCfgNode" }
796810

797811
override SelfVariableAccessMapping e;
798812

799-
override SelfVariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
813+
override SelfVariableAccess getExpr() { result = VariableAccessCfgNode.super.getExpr() }
814+
}
815+
816+
private class VariableWriteAccessChildMapping extends ExprChildMapping, VariableWriteAccess {
817+
override predicate relevantChild(AstNode n) { this.isExplicitWrite(n) }
800818
}
801819

802820
/** A control-flow node that wraps a `VariableWriteAccess` AST expression. */
803-
class VariableWriteAccessCfgNode extends ExprCfgNode {
821+
class VariableWriteAccessCfgNode extends VariableAccessCfgNode {
822+
/**
823+
* Holds if this access is a write access belonging to the explicit
824+
* assignment `assignment`. For example, in
825+
*
826+
* ```rb
827+
* a = foo
828+
* ```
829+
*
830+
* both `a` is write accesses belonging to the assignment `a = foo`.
831+
*/
832+
predicate isExplicitWrite(AssignExprCfgNode assignment) { this = assignment.getLhs() }
833+
834+
/**
835+
* Holds if this access is a write access belonging to an implicit assignment.
836+
*/
837+
predicate isImplicitWrite() { e.isImplicitWrite() }
838+
804839
override string getAPrimaryQlClass() { result = "VariableWriteAccessCfgNode" }
805840

806-
override VariableWriteAccess e;
841+
override VariableWriteAccessChildMapping e;
842+
843+
override VariableWriteAccess getExpr() { result = VariableAccessCfgNode.super.getExpr() }
844+
}
845+
846+
private class LocalVariableWriteAccessChildMapping extends VariableWriteAccessChildMapping,
847+
LocalVariableWriteAccess
848+
{ }
849+
850+
/** A control-flow node that wraps a `LocalVariableWriteAccess` AST expression. */
851+
class LocalVariableWriteAccessCfgNode extends VariableWriteAccessCfgNode {
852+
override string getAPrimaryQlClass() { result = "LocalVariableWriteAccessCfgNode" }
853+
854+
override LocalVariableWriteAccessChildMapping e;
855+
856+
final override LocalVariableWriteAccess getExpr() { result = super.getExpr() }
807857

808-
final override VariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
858+
final override LocalVariable getVariable() { result = super.getVariable() }
809859
}
810860

811861
/** A control-flow node that wraps a `ConstantReadAccess` AST expression. */

‎ruby/ql/lib/codeql/ruby/dataflow/SSA.qll

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,9 @@ module Ssa {
178178

179179
/** Gets the location of this SSA definition. */
180180
Location getLocation() { result = this.getControlFlowNode().getLocation() }
181+
182+
/** Gets the scope of this SSA definition. */
183+
CfgScope getScope() { result = this.getBasicBlock().getScope() }
181184
}
182185

183186
/**
@@ -189,7 +192,7 @@ module Ssa {
189192
* ```
190193
*/
191194
class WriteDefinition extends Definition, SsaImpl::WriteDefinition {
192-
private VariableWriteAccess write;
195+
private VariableWriteAccessCfgNode write;
193196

194197
WriteDefinition() {
195198
exists(BasicBlock bb, int i, Variable v |
@@ -199,7 +202,7 @@ module Ssa {
199202
}
200203

201204
/** Gets the underlying write access. */
202-
final VariableWriteAccess getWriteAccess() { result = write }
205+
final VariableWriteAccessCfgNode getWriteAccess() { result = write }
203206

204207
/**
205208
* Holds if this SSA definition represents a direct assignment of `value`
@@ -289,7 +292,7 @@ module Ssa {
289292
)
290293
}
291294

292-
final override string toString() { result = "<captured> " + this.getSourceVariable() }
295+
final override string toString() { result = "<captured entry> " + this.getSourceVariable() }
293296

294297
override Location getLocation() { result = this.getBasicBlock().getLocation() }
295298
}
@@ -342,7 +345,7 @@ module Ssa {
342345
*/
343346
final Definition getPriorDefinition() { result = SsaImpl::uncertainWriteDefinitionInput(this) }
344347

345-
override string toString() { result = this.getControlFlowNode().toString() }
348+
override string toString() { result = "<captured exit> " + this.getSourceVariable() }
346349
}
347350

348351
/**

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,8 @@ private predicate selfInToplevel(SelfVariable self, Module m) {
250250
private predicate asModulePattern(SsaDefinitionExtNode def, Module m) {
251251
exists(AsPattern ap |
252252
m = resolveConstantReadAccess(ap.getPattern()) and
253-
def.getDefinitionExt().(Ssa::WriteDefinition).getWriteAccess() = ap.getVariableAccess()
253+
def.getDefinitionExt().(Ssa::WriteDefinition).getWriteAccess().getNode() =
254+
ap.getVariableAccess()
254255
)
255256
}
256257

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

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -89,20 +89,20 @@ predicate uninitializedWrite(Cfg::EntryBasicBlock bb, int i, LocalVariable v) {
8989
/** Holds if `bb` contains a captured read of variable `v`. */
9090
pragma[noinline]
9191
private predicate hasCapturedVariableRead(Cfg::BasicBlock bb, LocalVariable v) {
92-
exists(LocalVariableReadAccess read |
93-
read = bb.getANode().getNode() and
94-
read.isCapturedAccess() and
92+
exists(LocalVariableReadAccessCfgNode read |
93+
read = bb.getANode() and
94+
read.getExpr().isCapturedAccess() and
9595
read.getVariable() = v
9696
)
9797
}
9898

9999
/** Holds if `bb` contains a captured write to variable `v`. */
100100
pragma[noinline]
101101
private predicate writesCapturedVariable(Cfg::BasicBlock bb, LocalVariable v) {
102-
exists(LocalVariableWriteAccess write |
103-
write = bb.getANode().getNode() and
104-
write.isCapturedAccess() and
105-
write.getVariable() = v
102+
exists(LocalVariableWriteAccessCfgNode write |
103+
write = bb.getANode() and
104+
write.getVariable() = v and
105+
v.isCaptured()
106106
)
107107
}
108108

@@ -137,9 +137,9 @@ private predicate namespaceSelfExitRead(Cfg::AnnotatedExitBasicBlock bb, int i,
137137
*/
138138
pragma[noinline]
139139
private predicate hasCapturedRead(Variable v, Cfg::CfgScope scope) {
140-
any(LocalVariableReadAccess read |
141-
read.getVariable() = v and scope = read.getCfgScope().getOuterCfgScope*()
142-
).isCapturedAccess()
140+
any(LocalVariableReadAccessCfgNode read |
141+
read.getVariable() = v and scope = read.getScope().getOuterCfgScope*()
142+
).getExpr().isCapturedAccess()
143143
}
144144

145145
/**
@@ -153,11 +153,13 @@ private predicate variableWriteInOuterScope(Cfg::BasicBlock bb, LocalVariable v,
153153
}
154154

155155
pragma[noinline]
156-
private predicate hasVariableWriteWithCapturedRead(
156+
private predicate proceedsVariableWriteWithCapturedRead(
157157
Cfg::BasicBlock bb, LocalVariable v, Cfg::CfgScope scope
158158
) {
159159
hasCapturedRead(v, scope) and
160160
variableWriteInOuterScope(bb, v, scope)
161+
or
162+
proceedsVariableWriteWithCapturedRead(bb.getAPredecessor(), v, scope)
161163
}
162164

163165
/**
@@ -166,7 +168,7 @@ private predicate hasVariableWriteWithCapturedRead(
166168
*/
167169
private predicate capturedCallRead(CallCfgNode call, Cfg::BasicBlock bb, int i, LocalVariable v) {
168170
exists(Cfg::CfgScope scope |
169-
hasVariableWriteWithCapturedRead(bb.getAPredecessor*(), v, scope) and
171+
proceedsVariableWriteWithCapturedRead(bb, v, scope) and
170172
call = bb.getNode(i)
171173
|
172174
// If the read happens inside a block, we restrict to the call that
@@ -191,9 +193,9 @@ private predicate variableReadActual(Cfg::BasicBlock bb, int i, LocalVariable v)
191193
*/
192194
pragma[noinline]
193195
private predicate hasCapturedWrite(Variable v, Cfg::CfgScope scope) {
194-
any(LocalVariableWriteAccess write |
195-
write.getVariable() = v and scope = write.getCfgScope().getOuterCfgScope*()
196-
).isCapturedAccess()
196+
any(LocalVariableWriteAccessCfgNode write |
197+
write.getVariable() = v and scope = write.getScope().getOuterCfgScope*()
198+
).getExpr().isCapturedAccess()
197199
}
198200

199201
/**
@@ -339,11 +341,11 @@ private module Cached {
339341
*/
340342
cached
341343
predicate variableWriteActual(
342-
Cfg::BasicBlock bb, int i, LocalVariable v, VariableWriteAccess write
344+
Cfg::BasicBlock bb, int i, LocalVariable v, VariableWriteAccessCfgNode write
343345
) {
344-
exists(AstNode n |
346+
exists(Cfg::CfgNode n |
345347
write.getVariable() = v and
346-
n = bb.getNode(i).getNode()
348+
n = bb.getNode(i)
347349
|
348350
write.isExplicitWrite(n)
349351
or

‎ruby/ql/src/experimental/performance/UseDetect.ql

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
// This is an implementation of the Rubocop rule
1212
// https://github.com/rubocop/rubocop-performance/blob/master/lib/rubocop/cop/performance/detect.rb
1313
import codeql.ruby.AST
14+
import codeql.ruby.CFG
1415
import codeql.ruby.dataflow.SSA
1516

1617
/** A call that extracts the first or last element of a list. */
@@ -41,12 +42,12 @@ class EndCall extends MethodCall {
4142
}
4243

4344
Expr getUniqueRead(Expr e) {
44-
exists(AssignExpr ae |
45-
e = ae.getRightOperand() and
46-
forex(Ssa::WriteDefinition def | def.getWriteAccess() = ae.getLeftOperand() |
45+
forex(CfgNode eNode | eNode.getNode() = e |
46+
exists(Ssa::WriteDefinition def |
47+
def.assigns(eNode) and
4748
strictcount(def.getARead()) = 1 and
4849
not def = any(Ssa::PhiNode phi).getAnInput() and
49-
def.getARead() = result.getAControlFlowNode()
50+
def.getARead().getNode() = result
5051
)
5152
)
5253
}

‎ruby/ql/src/queries/analysis/Definitions.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ newtype DefLoc =
4848
/** A local variable. */
4949
LocalVariableLoc(VariableReadAccess read, VariableWriteAccess write) {
5050
exists(Ssa::WriteDefinition w |
51-
write = w.getWriteAccess() and
51+
write = w.getWriteAccess().getNode() and
5252
read = w.getARead().getExpr() and
5353
not read.isSynthesized()
5454
)

‎ruby/ql/src/queries/variables/DeadStoreOfLocal.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,5 +24,5 @@ from RelevantLocalVariableWriteAccess write, LocalVariable v
2424
where
2525
v = write.getVariable() and
2626
exists(write.getAControlFlowNode()) and
27-
not exists(Ssa::WriteDefinition def | def.getWriteAccess() = write)
27+
not exists(Ssa::WriteDefinition def | def.getWriteAccess().getNode() = write)
2828
select write, "This assignment to $@ is useless, since its value is never read.", v, v.getName()

‎ruby/ql/src/queries/variables/UnusedParameter.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,5 @@ class RelevantParameterVariable extends LocalVariable {
2323
}
2424

2525
from RelevantParameterVariable v
26-
where not exists(Ssa::WriteDefinition def | def.getWriteAccess() = v.getDefiningAccess())
26+
where not exists(Ssa::WriteDefinition def | def.getWriteAccess().getNode() = v.getDefiningAccess())
2727
select v, "The parameter '" + v.getName() + "' is never used."

‎ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2425,7 +2425,7 @@
24252425
| local_dataflow.rb:9:9:9:15 | call to [] | local_dataflow.rb:9:1:9:15 | ... = ... |
24262426
| local_dataflow.rb:9:9:9:15 | call to [] | local_dataflow.rb:9:1:9:15 | ... = ... |
24272427
| local_dataflow.rb:10:5:13:3 | ... = ... | local_dataflow.rb:12:5:12:5 | x |
2428-
| local_dataflow.rb:10:5:13:3 | <captured> self | local_dataflow.rb:11:1:11:2 | self |
2428+
| local_dataflow.rb:10:5:13:3 | <captured entry> self | local_dataflow.rb:11:1:11:2 | self |
24292429
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | ... = ... |
24302430
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | ... = ... |
24312431
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | __synth__0__1 |
@@ -2464,7 +2464,7 @@
24642464
| local_dataflow.rb:45:10:45:10 | 6 | local_dataflow.rb:45:3:45:10 | return |
24652465
| local_dataflow.rb:49:1:53:3 | [post] self | local_dataflow.rb:55:1:55:14 | self |
24662466
| local_dataflow.rb:49:1:53:3 | self | local_dataflow.rb:55:1:55:14 | self |
2467-
| local_dataflow.rb:49:3:53:3 | <captured> x | local_dataflow.rb:50:18:50:18 | x |
2467+
| local_dataflow.rb:49:3:53:3 | <captured entry> x | local_dataflow.rb:50:18:50:18 | x |
24682468
| local_dataflow.rb:50:8:50:13 | "next" | local_dataflow.rb:50:3:50:13 | next |
24692469
| local_dataflow.rb:50:18:50:18 | [post] x | local_dataflow.rb:51:20:51:20 | x |
24702470
| local_dataflow.rb:50:18:50:18 | x | local_dataflow.rb:51:20:51:20 | x |
@@ -2631,7 +2631,7 @@
26312631
| local_dataflow.rb:118:3:118:11 | [post] self | local_dataflow.rb:119:3:119:31 | self |
26322632
| local_dataflow.rb:118:3:118:11 | call to source | local_dataflow.rb:118:3:118:31 | call to tap |
26332633
| local_dataflow.rb:118:3:118:11 | self | local_dataflow.rb:119:3:119:31 | self |
2634-
| local_dataflow.rb:118:17:118:31 | <captured> self | local_dataflow.rb:118:23:118:29 | self |
2634+
| local_dataflow.rb:118:17:118:31 | <captured entry> self | local_dataflow.rb:118:23:118:29 | self |
26352635
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:20:118:20 | x |
26362636
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:28:118:28 | x |
26372637
| local_dataflow.rb:119:3:119:31 | [post] self | local_dataflow.rb:119:8:119:16 | self |
@@ -2645,7 +2645,7 @@
26452645
| local_dataflow.rb:123:8:123:16 | call to source | local_dataflow.rb:123:8:123:20 | call to dup |
26462646
| local_dataflow.rb:123:8:123:20 | call to dup | local_dataflow.rb:123:8:123:45 | call to tap |
26472647
| local_dataflow.rb:123:8:123:45 | call to tap | local_dataflow.rb:123:8:123:49 | call to dup |
2648-
| local_dataflow.rb:123:26:123:45 | <captured> self | local_dataflow.rb:123:32:123:43 | self |
2648+
| local_dataflow.rb:123:26:123:45 | <captured entry> self | local_dataflow.rb:123:32:123:43 | self |
26492649
| local_dataflow.rb:126:1:128:3 | self (use) | local_dataflow.rb:127:3:127:8 | self |
26502650
| local_dataflow.rb:126:1:128:3 | self in use | local_dataflow.rb:126:1:128:3 | self (use) |
26512651
| local_dataflow.rb:130:1:150:3 | self (use_use_madness) | local_dataflow.rb:132:6:132:11 | self |

‎ruby/ql/test/library-tests/dataflow/local/TaintStep.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2858,7 +2858,7 @@
28582858
| local_dataflow.rb:9:9:9:15 | call to [] | local_dataflow.rb:9:1:9:15 | ... = ... |
28592859
| local_dataflow.rb:9:9:9:15 | call to [] | local_dataflow.rb:9:1:9:15 | ... = ... |
28602860
| local_dataflow.rb:10:5:13:3 | ... = ... | local_dataflow.rb:12:5:12:5 | x |
2861-
| local_dataflow.rb:10:5:13:3 | <captured> self | local_dataflow.rb:11:1:11:2 | self |
2861+
| local_dataflow.rb:10:5:13:3 | <captured entry> self | local_dataflow.rb:11:1:11:2 | self |
28622862
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | ... = ... |
28632863
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | ... = ... |
28642864
| local_dataflow.rb:10:5:13:3 | __synth__0__1 | local_dataflow.rb:10:5:13:3 | __synth__0__1 |
@@ -2903,7 +2903,7 @@
29032903
| local_dataflow.rb:45:10:45:10 | 6 | local_dataflow.rb:45:3:45:10 | return |
29042904
| local_dataflow.rb:49:1:53:3 | [post] self | local_dataflow.rb:55:1:55:14 | self |
29052905
| local_dataflow.rb:49:1:53:3 | self | local_dataflow.rb:55:1:55:14 | self |
2906-
| local_dataflow.rb:49:3:53:3 | <captured> x | local_dataflow.rb:50:18:50:18 | x |
2906+
| local_dataflow.rb:49:3:53:3 | <captured entry> x | local_dataflow.rb:50:18:50:18 | x |
29072907
| local_dataflow.rb:50:8:50:13 | "next" | local_dataflow.rb:50:3:50:13 | next |
29082908
| local_dataflow.rb:50:18:50:18 | [post] x | local_dataflow.rb:51:20:51:20 | x |
29092909
| local_dataflow.rb:50:18:50:18 | x | local_dataflow.rb:50:18:50:22 | ... < ... |
@@ -3086,7 +3086,7 @@
30863086
| local_dataflow.rb:118:3:118:11 | [post] self | local_dataflow.rb:119:3:119:31 | self |
30873087
| local_dataflow.rb:118:3:118:11 | call to source | local_dataflow.rb:118:3:118:31 | call to tap |
30883088
| local_dataflow.rb:118:3:118:11 | self | local_dataflow.rb:119:3:119:31 | self |
3089-
| local_dataflow.rb:118:17:118:31 | <captured> self | local_dataflow.rb:118:23:118:29 | self |
3089+
| local_dataflow.rb:118:17:118:31 | <captured entry> self | local_dataflow.rb:118:23:118:29 | self |
30903090
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:20:118:20 | x |
30913091
| local_dataflow.rb:118:20:118:20 | x | local_dataflow.rb:118:28:118:28 | x |
30923092
| local_dataflow.rb:119:3:119:31 | [post] self | local_dataflow.rb:119:8:119:16 | self |
@@ -3100,7 +3100,7 @@
31003100
| local_dataflow.rb:123:8:123:16 | call to source | local_dataflow.rb:123:8:123:20 | call to dup |
31013101
| local_dataflow.rb:123:8:123:20 | call to dup | local_dataflow.rb:123:8:123:45 | call to tap |
31023102
| local_dataflow.rb:123:8:123:45 | call to tap | local_dataflow.rb:123:8:123:49 | call to dup |
3103-
| local_dataflow.rb:123:26:123:45 | <captured> self | local_dataflow.rb:123:32:123:43 | self |
3103+
| local_dataflow.rb:123:26:123:45 | <captured entry> self | local_dataflow.rb:123:32:123:43 | self |
31043104
| local_dataflow.rb:126:1:128:3 | self (use) | local_dataflow.rb:127:3:127:8 | self |
31053105
| local_dataflow.rb:126:1:128:3 | self in use | local_dataflow.rb:126:1:128:3 | self (use) |
31063106
| local_dataflow.rb:130:1:150:3 | self (use_use_madness) | local_dataflow.rb:132:6:132:11 | self |

0 commit comments

Comments
 (0)