Skip to content

Commit ee9163a

Browse files
hvitvedhvitved
committed
Ruby: Fix flow steps into phi nodes
- Add missing flow from post-update nodes into phi nodes. - Prevent flow from reads into phi nodes when use-use flow is prohibited.
1 parent a191edf commit ee9163a

File tree

2 files changed

+58
-30
lines changed

2 files changed

+58
-30
lines changed

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

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,19 +76,29 @@ module LocalFlow {
7676
private import codeql.ruby.dataflow.internal.SsaImpl
7777

7878
/**
79-
* Holds if `nodeFrom` is a last node referencing SSA definition `def`, which
80-
* can reach `next`.
79+
* Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`.
8180
*/
82-
private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) {
83-
exists(BasicBlock bb, int i | lastRefBeforeRedef(def, bb, i, next) |
84-
def = nodeFrom.(SsaDefinitionNode).getDefinition() and
81+
private predicate localFlowSsaInputFromDef(
82+
SsaDefinitionNode nodeFrom, Ssa::Definition def, Ssa::Definition next
83+
) {
84+
exists(BasicBlock bb, int i |
85+
lastRefBeforeRedef(def, bb, i, next) and
86+
def = nodeFrom.getDefinition() and
8587
def.definesAt(_, bb, i)
86-
or
87-
exists(CfgNodes::ExprCfgNode e |
88-
e = nodeFrom.asExpr() and
89-
e = bb.getNode(i) and
90-
e.getExpr() instanceof VariableReadAccess
91-
)
88+
)
89+
}
90+
91+
/**
92+
* Holds if `exprFrom` is a last read of SSA definition `def`, which
93+
* can reach `next`.
94+
*/
95+
predicate localFlowSsaInputFromExpr(
96+
CfgNodes::ExprCfgNode exprFrom, Ssa::Definition def, Ssa::Definition next
97+
) {
98+
exists(BasicBlock bb, int i |
99+
lastRefBeforeRedef(def, bb, i, next) and
100+
exprFrom = bb.getNode(i) and
101+
exprFrom.getExpr() instanceof VariableReadAccess
92102
)
93103
}
94104

@@ -139,9 +149,9 @@ module LocalFlow {
139149
// Flow from read to next read
140150
localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
141151
or
142-
// Flow into phi node
152+
// Flow into phi node from definition
143153
exists(Ssa::PhiNode phi |
144-
localFlowSsaInput(nodeFrom, def, phi) and
154+
localFlowSsaInputFromDef(nodeFrom, def, phi) and
145155
phi = nodeTo.(SsaDefinitionNode).getDefinition() and
146156
def = phi.getAnInput()
147157
)
@@ -308,6 +318,18 @@ private module Cached {
308318
LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) and
309319
not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
310320
or
321+
// Flow into phi node from read
322+
exists(Ssa::Definition def, Ssa::PhiNode phi, CfgNodes::ExprCfgNode exprFrom |
323+
LocalFlow::localFlowSsaInputFromExpr(exprFrom, def, phi) and
324+
phi = nodeTo.(SsaDefinitionNode).getDefinition() and
325+
def = phi.getAnInput()
326+
|
327+
exprFrom = nodeFrom.asExpr() and
328+
not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
329+
or
330+
exprFrom = nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()
331+
)
332+
or
311333
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true)
312334
}
313335

@@ -338,6 +360,14 @@ private module Cached {
338360
)
339361
or
340362
LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
363+
or
364+
// Flow into phi node from read
365+
exists(Ssa::Definition def, Ssa::PhiNode phi, CfgNodes::ExprCfgNode exprFrom |
366+
LocalFlow::localFlowSsaInputFromExpr(exprFrom, def, phi) and
367+
phi = nodeTo.(SsaDefinitionNode).getDefinition() and
368+
def = phi.getAnInput() and
369+
exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()]
370+
)
341371
}
342372

343373
private predicate entrySsaDefinition(SsaDefinitionNode n) {

‎ruby/ql/test/library-tests/dataflow/ssa-flow/ssa-flow.expected

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,20 @@
11
failures
2-
| ssa_flow.rb:16:16:16:33 | # $ hasValueFlow=1 | Missing result:hasValueFlow=1 |
3-
| ssa_flow.rb:29:10:29:13 | ...[...] | Unexpected result: hasValueFlow=2 |
42
edges
5-
| ssa_flow.rb:24:9:24:9 | [post] a [element 0] : | ssa_flow.rb:29:10:29:10 | a [element 0] : |
6-
| ssa_flow.rb:24:9:24:9 | [post] a [element 0] : | ssa_flow.rb:29:10:29:10 | a [element 0] : |
7-
| ssa_flow.rb:24:16:24:23 | call to taint : | ssa_flow.rb:24:9:24:9 | [post] a [element 0] : |
8-
| ssa_flow.rb:24:16:24:23 | call to taint : | ssa_flow.rb:24:9:24:9 | [post] a [element 0] : |
9-
| ssa_flow.rb:29:10:29:10 | a [element 0] : | ssa_flow.rb:29:10:29:13 | ...[...] |
10-
| ssa_flow.rb:29:10:29:10 | a [element 0] : | ssa_flow.rb:29:10:29:13 | ...[...] |
3+
| ssa_flow.rb:12:9:12:9 | [post] a [element 0] : | ssa_flow.rb:16:10:16:10 | a [element 0] : |
4+
| ssa_flow.rb:12:9:12:9 | [post] a [element 0] : | ssa_flow.rb:16:10:16:10 | a [element 0] : |
5+
| ssa_flow.rb:12:16:12:23 | call to taint : | ssa_flow.rb:12:9:12:9 | [post] a [element 0] : |
6+
| ssa_flow.rb:12:16:12:23 | call to taint : | ssa_flow.rb:12:9:12:9 | [post] a [element 0] : |
7+
| ssa_flow.rb:16:10:16:10 | a [element 0] : | ssa_flow.rb:16:10:16:13 | ...[...] |
8+
| ssa_flow.rb:16:10:16:10 | a [element 0] : | ssa_flow.rb:16:10:16:13 | ...[...] |
119
nodes
12-
| ssa_flow.rb:24:9:24:9 | [post] a [element 0] : | semmle.label | [post] a [element 0] : |
13-
| ssa_flow.rb:24:9:24:9 | [post] a [element 0] : | semmle.label | [post] a [element 0] : |
14-
| ssa_flow.rb:24:16:24:23 | call to taint : | semmle.label | call to taint : |
15-
| ssa_flow.rb:24:16:24:23 | call to taint : | semmle.label | call to taint : |
16-
| ssa_flow.rb:29:10:29:10 | a [element 0] : | semmle.label | a [element 0] : |
17-
| ssa_flow.rb:29:10:29:10 | a [element 0] : | semmle.label | a [element 0] : |
18-
| ssa_flow.rb:29:10:29:13 | ...[...] | semmle.label | ...[...] |
19-
| ssa_flow.rb:29:10:29:13 | ...[...] | semmle.label | ...[...] |
10+
| ssa_flow.rb:12:9:12:9 | [post] a [element 0] : | semmle.label | [post] a [element 0] : |
11+
| ssa_flow.rb:12:9:12:9 | [post] a [element 0] : | semmle.label | [post] a [element 0] : |
12+
| ssa_flow.rb:12:16:12:23 | call to taint : | semmle.label | call to taint : |
13+
| ssa_flow.rb:12:16:12:23 | call to taint : | semmle.label | call to taint : |
14+
| ssa_flow.rb:16:10:16:10 | a [element 0] : | semmle.label | a [element 0] : |
15+
| ssa_flow.rb:16:10:16:10 | a [element 0] : | semmle.label | a [element 0] : |
16+
| ssa_flow.rb:16:10:16:13 | ...[...] | semmle.label | ...[...] |
17+
| ssa_flow.rb:16:10:16:13 | ...[...] | semmle.label | ...[...] |
2018
subpaths
2119
#select
22-
| ssa_flow.rb:29:10:29:13 | ...[...] | ssa_flow.rb:24:16:24:23 | call to taint : | ssa_flow.rb:29:10:29:13 | ...[...] | $@ | ssa_flow.rb:24:16:24:23 | call to taint : | call to taint : |
20+
| ssa_flow.rb:16:10:16:13 | ...[...] | ssa_flow.rb:12:16:12:23 | call to taint : | ssa_flow.rb:16:10:16:13 | ...[...] | $@ | ssa_flow.rb:12:16:12:23 | call to taint : | call to taint : |

0 commit comments

Comments
 (0)