Skip to content

Commit 6781a76

Browse files
aibaarsaibaars
authored
Merge pull request github#9206 from aibaars/instance-variable-flow
Ruby: flow through instance variables
2 parents 730f54a + cf2eb0d commit 6781a76

File tree

17 files changed

+388
-15
lines changed

17 files changed

+388
-15
lines changed

‎ruby/ql/lib/change-notes/2022-05-23-flow-through-instance-variables.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
Support for data flow through instance variables has been added.

‎ruby/ql/lib/codeql/ruby/ast/Variable.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,17 @@ class GlobalVariableReadAccess extends GlobalVariableAccess, VariableReadAccess
181181
/** An access to an instance variable. */
182182
class InstanceVariableAccess extends VariableAccess instanceof InstanceVariableAccessImpl {
183183
final override string getAPrimaryQlClass() { result = "InstanceVariableAccess" }
184+
185+
/**
186+
* Gets the synthetic receiver (`self`) of this instance variable access.
187+
*/
188+
final SelfVariableAccess getReceiver() { synthChild(this, 0, result) }
189+
190+
final override AstNode getAChild(string pred) {
191+
result = VariableAccess.super.getAChild(pred)
192+
or
193+
pred = "getReceiver" and result = this.getReceiver()
194+
}
184195
}
185196

186197
/** An access to an instance variable where the value is updated. */

‎ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,38 @@ private module ImplicitSelfSynthesis {
210210
regularMethodCallSelfSynthesis(parent, i, child)
211211
}
212212
}
213+
214+
pragma[nomagic]
215+
private AstNode instanceVarAccessSynthParentStar(InstanceVariableAccess var) {
216+
result = var
217+
or
218+
instanceVarAccessSynthParentStar(var) = getSynthChild(result, _)
219+
}
220+
221+
/**
222+
* Gets the `SelfKind` for instance variable access `var`. This is based on the
223+
* "owner" of `var`; for real nodes this is the node itself, for synthetic nodes
224+
* this is the closest parent that is a real node.
225+
*/
226+
pragma[nomagic]
227+
private SelfKind getSelfKind(InstanceVariableAccess var) {
228+
exists(Ruby::AstNode owner |
229+
owner = toGenerated(instanceVarAccessSynthParentStar(var)) and
230+
result = SelfKind(TSelfVariable(scopeOf(owner).getEnclosingSelfScope()))
231+
)
232+
}
233+
234+
pragma[nomagic]
235+
private predicate instanceVariableSelfSynthesis(InstanceVariableAccess var, int i, Child child) {
236+
child = SynthChild(getSelfKind(var)) and
237+
i = 0
238+
}
239+
240+
private class InstanceVariableSelfSynthesis extends Synthesis {
241+
final override predicate child(AstNode parent, int i, Child child) {
242+
instanceVariableSelfSynthesis(parent, i, child)
243+
}
244+
}
213245
}
214246

215247
private module SetterDesugar {

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

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,24 @@ module ExprNodes {
682682
final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
683683
}
684684

685+
private class InstanceVariableAccessMapping extends ExprChildMapping, InstanceVariableAccess {
686+
override predicate relevantChild(AstNode n) { n = this.getReceiver() }
687+
}
688+
689+
/** A control-flow node that wraps an `InstanceVariableAccess` AST expression. */
690+
class InstanceVariableAccessCfgNode extends ExprCfgNode {
691+
override string getAPrimaryQlClass() { result = "InstanceVariableAccessCfgNode" }
692+
693+
override InstanceVariableAccessMapping e;
694+
695+
override InstanceVariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
696+
697+
/**
698+
* Gets the synthetic receiver (`self`) of this instance variable access.
699+
*/
700+
final CfgNode getReceiver() { e.hasCfgChild(e.getReceiver(), this, result) }
701+
}
702+
685703
/** A control-flow node that wraps a `VariableWriteAccess` AST expression. */
686704
class VariableWriteAccessCfgNode extends ExprCfgNode {
687705
override string getAPrimaryQlClass() { result = "VariableWriteAccessCfgNode" }
@@ -709,13 +727,26 @@ module ExprNodes {
709727
final override ConstantWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
710728
}
711729

712-
/** A control-flow node that wraps a `InstanceVariableWriteAccess` AST expression. */
713-
class InstanceVariableWriteAccessCfgNode extends ExprCfgNode {
714-
override string getAPrimaryQlClass() { result = "InstanceVariableWriteAccessCfgNode" }
730+
/** A control-flow node that wraps an `InstanceVariableReadAccess` AST expression. */
731+
class InstanceVariableReadAccessCfgNode extends InstanceVariableAccessCfgNode {
732+
InstanceVariableReadAccessCfgNode() { this.getNode() instanceof InstanceVariableReadAccess }
733+
734+
override string getAPrimaryQlClass() { result = "InstanceVariableReadAccessCfgNode" }
735+
736+
final override InstanceVariableReadAccess getExpr() {
737+
result = InstanceVariableAccessCfgNode.super.getExpr()
738+
}
739+
}
740+
741+
/** A control-flow node that wraps an `InstanceVariableWriteAccess` AST expression. */
742+
class InstanceVariableWriteAccessCfgNode extends InstanceVariableAccessCfgNode {
743+
InstanceVariableWriteAccessCfgNode() { this.getNode() instanceof InstanceVariableWriteAccess }
715744

716-
override InstanceVariableWriteAccess e;
745+
override string getAPrimaryQlClass() { result = "InstanceVariableWriteAccessCfgNode" }
717746

718-
final override InstanceVariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
747+
final override InstanceVariableWriteAccess getExpr() {
748+
result = InstanceVariableAccessCfgNode.super.getExpr()
749+
}
719750
}
720751

721752
/** A control-flow node that wraps a `StringInterpolationComponent` AST expression. */

‎ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1006,7 +1006,9 @@ module Trees {
10061006
final override ControlFlowTree getChildElement(int i) { result = this.getComponent(i) }
10071007
}
10081008

1009-
private class InstanceVariableTree extends LeafTree, InstanceVariableAccess { }
1009+
private class InstanceVariableTree extends StandardPostOrderTree, InstanceVariableAccess {
1010+
final override ControlFlowTree getChildElement(int i) { result = this.getReceiver() and i = 0 }
1011+
}
10101012

10111013
private class KeywordParameterTree extends DefaultValueParameterTree, KeywordParameter {
10121014
final override Expr getDefaultValueExpr() { result = this.getDefaultValue() }

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

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,10 @@ private module Cached {
219219
} or
220220
TSelfParameterNode(MethodBase m) or
221221
TBlockParameterNode(MethodBase m) or
222-
TExprPostUpdateNode(CfgNodes::ExprCfgNode n) { n instanceof Argument } or
222+
TExprPostUpdateNode(CfgNodes::ExprCfgNode n) {
223+
n instanceof Argument or
224+
n = any(CfgNodes::ExprNodes::InstanceVariableAccessCfgNode v).getReceiver()
225+
} or
223226
TSummaryNode(
224227
FlowSummaryImpl::Public::SummarizedCallable c,
225228
FlowSummaryImpl::Private::SummaryNodeState state
@@ -339,6 +342,7 @@ private module Cached {
339342
not cv.isInt(_) or
340343
cv.getInt() in [0 .. 10]
341344
} or
345+
TFieldContent(string name) { name = any(InstanceVariable v).getName() } or
342346
TUnknownElementContent()
343347

344348
/**
@@ -795,11 +799,40 @@ predicate jumpStep(Node pred, Node succ) {
795799
succ.asExpr().getExpr().(ConstantReadAccess).getValue() = pred.asExpr().getExpr()
796800
}
797801

802+
/**
803+
* Holds if data can flow from `node1` to `node2` via an assignment to
804+
* content `c`.
805+
*/
798806
predicate storeStep(Node node1, ContentSet c, Node node2) {
807+
// Instance variable assignment, `@var = src`
808+
node2.(PostUpdateNode).getPreUpdateNode().asExpr() =
809+
any(CfgNodes::ExprNodes::InstanceVariableWriteAccessCfgNode var |
810+
exists(CfgNodes::ExprNodes::AssignExprCfgNode assign |
811+
var = assign.getLhs() and
812+
node1.asExpr() = assign.getRhs()
813+
|
814+
c.isSingleton(any(Content::FieldContent ct |
815+
ct.getName() = var.getExpr().getVariable().getName()
816+
))
817+
)
818+
).getReceiver()
819+
or
799820
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2)
800821
}
801822

823+
/**
824+
* Holds if there is a read step of content `c` from `node1` to `node2`.
825+
*/
802826
predicate readStep(Node node1, ContentSet c, Node node2) {
827+
// Instance variable read access, `@var`
828+
node2.asExpr() =
829+
any(CfgNodes::ExprNodes::InstanceVariableReadAccessCfgNode var |
830+
node1.asExpr() = var.getReceiver() and
831+
c.isSingleton(any(Content::FieldContent ct |
832+
ct.getName() = var.getExpr().getVariable().getName()
833+
))
834+
)
835+
or
803836
FlowSummaryImpl::Private::Steps::summaryReadStep(node1, c, node2)
804837
}
805838

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,18 @@ module Content {
223223
override string toString() { result = "element" }
224224
}
225225

226+
/** A field of an object, for example an instance variable. */
227+
class FieldContent extends Content, TFieldContent {
228+
private string name;
229+
230+
FieldContent() { this = TFieldContent(name) }
231+
232+
/** Gets the name of the field. */
233+
string getName() { result = name }
234+
235+
override string toString() { result = name }
236+
}
237+
226238
/** Gets the element content corresponding to constant value `cv`. */
227239
ElementContent getElementContent(ConstantValue cv) {
228240
result = TKnownElementContent(cv)

‎ruby/ql/test/library-tests/ast/Ast.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,6 +1234,7 @@ control/cases.rb:
12341234
# 150| getBranch: [InClause] in ... then ...
12351235
# 150| getPattern: [ReferencePattern] ^...
12361236
# 150| getExpr: [InstanceVariableAccess] @foo
1237+
# 150| getReceiver: [SelfVariableAccess] self
12371238
# 151| getBranch: [InClause] in ... then ...
12381239
# 151| getPattern: [ReferencePattern] ^...
12391240
# 151| getExpr: [ClassVariableAccess] @@foo
@@ -1246,6 +1247,7 @@ control/cases.rb:
12461247
# 156| getBranch: [InClause] in ... then ...
12471248
# 156| getPattern: [ReferencePattern] ^...
12481249
# 156| getExpr: [InstanceVariableAccess] @foo
1250+
# 156| getReceiver: [SelfVariableAccess] self
12491251
# 157| getBranch: [InClause] in ... then ...
12501252
# 157| getPattern: [ReferencePattern] ^...
12511253
# 157| getExpr: [AddExpr] ... + ...
@@ -2771,9 +2773,11 @@ operations/operations.rb:
27712773
# 87| getStmt: [ClassDeclaration] X
27722774
# 88| getStmt: [AssignExpr] ... = ...
27732775
# 88| getAnOperand/getLeftOperand: [InstanceVariableAccess] @x
2776+
# 88| getReceiver: [SelfVariableAccess] self
27742777
# 88| getAnOperand/getRightOperand: [IntegerLiteral] 1
27752778
# 89| getStmt: [AssignAddExpr] ... += ...
27762779
# 89| getAnOperand/getLeftOperand: [InstanceVariableAccess] @x
2780+
# 89| getReceiver: [SelfVariableAccess] self
27772781
# 89| getAnOperand/getRightOperand: [IntegerLiteral] 2
27782782
# 91| getStmt: [AssignExpr] ... = ...
27792783
# 91| getAnOperand/getLeftOperand: [ClassVariableAccess] @@y

‎ruby/ql/test/library-tests/ast/AstDesugar.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -836,8 +836,10 @@ operations/operations.rb:
836836
# 89| [AssignAddExpr] ... += ...
837837
# 89| getDesugared: [AssignExpr] ... = ...
838838
# 89| getAnOperand/getLeftOperand: [InstanceVariableAccess] @x
839+
# 89| getReceiver: [SelfVariableAccess] self
839840
# 89| getAnOperand/getRightOperand: [AddExpr] ... + ...
840841
# 89| getAnOperand/getLeftOperand/getReceiver: [InstanceVariableAccess] @x
842+
# 89| getReceiver: [SelfVariableAccess] self
841843
# 89| getAnOperand/getArgument/getRightOperand: [IntegerLiteral] 2
842844
# 92| [AssignDivExpr] ... /= ...
843845
# 92| getDesugared: [AssignExpr] ... = ...

‎ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1423,9 +1423,12 @@ cfg.html.erb:
14231423
# 5| @title
14241424
#-----| -> self
14251425

1426-
# 5| enter cfg.html.erb
1426+
# 5| self
14271427
#-----| -> @title
14281428

1429+
# 5| enter cfg.html.erb
1430+
#-----| -> self
1431+
14291432
# 5| exit cfg.html.erb
14301433

14311434
# 5| exit cfg.html.erb (normal)
@@ -2690,11 +2693,14 @@ cfg.rb:
26902693
#-----| -> ... > ...
26912694

26922695
# 115| C
2693-
#-----| -> @field
2696+
#-----| -> self
26942697

26952698
# 116| @field
26962699
#-----| -> 42
26972700

2701+
# 116| self
2702+
#-----| -> @field
2703+
26982704
# 116| ... = ...
26992705
#-----| -> @@static_field
27002706

@@ -4194,23 +4200,32 @@ desugar.rb:
41944200
#-----| -> call to []
41954201

41964202
# 29| X
4197-
#-----| -> @x
4203+
#-----| -> self
41984204

41994205
# 30| @x
42004206
#-----| -> 1
42014207

4202-
# 30| ... = ...
4208+
# 30| self
42034209
#-----| -> @x
42044210

4211+
# 30| ... = ...
4212+
#-----| -> self
4213+
42054214
# 30| 1
42064215
#-----| -> ... = ...
42074216

42084217
# 31| @x
4209-
#-----| -> @x
4218+
#-----| -> self
42104219

42114220
# 31| @x
42124221
#-----| -> 2
42134222

4223+
# 31| self
4224+
#-----| -> @x
4225+
4226+
# 31| self
4227+
#-----| -> @x
4228+
42144229
# 31| ... = ...
42154230
#-----| -> @@y
42164231

0 commit comments

Comments
 (0)