Skip to content

Commit 6873eba

Browse files
dilanbhalladilanbhalla
authored
Merge pull request #185 from microsoft/fix-ssa-for-powershell-2
PS: Fixup SSA after GitHub's 2.21.0 changes
2 parents 7c59a74 + 2f215c1 commit 6873eba

File tree

4 files changed

+56
-73
lines changed

4 files changed

+56
-73
lines changed

‎powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll

Lines changed: 32 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -97,15 +97,22 @@ module SsaFlow {
9797
or
9898
result.(Impl::ExprPostUpdateNode).getExpr() = n.(PostUpdateNode).getPreUpdateNode().asExpr()
9999
or
100-
n = toParameterNode(result.(Impl::ParameterNode).getParameter())
100+
exists(SsaImpl::ParameterExt p |
101+
n = toParameterNode(p) and
102+
p.isInitializedBy(result.(Impl::WriteDefSourceNode).getDefinition())
103+
)
104+
or
105+
result.(Impl::WriteDefSourceNode).getDefinition().(Ssa::WriteDefinition).assigns(n.asExpr())
101106
}
102107

103-
predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
104-
Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
108+
predicate localFlowStep(
109+
SsaImpl::SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep
110+
) {
111+
Impl::localFlowStep(v, asNode(nodeFrom), asNode(nodeTo), isUseStep)
105112
}
106113

107-
predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
108-
Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo))
114+
predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
115+
Impl::localMustFlowStep(_, asNode(nodeFrom), asNode(nodeTo))
109116
}
110117
}
111118

@@ -179,7 +186,7 @@ module LocalFlow {
179186
}
180187

181188
predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
182-
SsaFlow::localMustFlowStep(_, nodeFrom, nodeTo)
189+
SsaFlow::localMustFlowStep(nodeFrom, nodeTo)
183190
or
184191
nodeFrom =
185192
unique(FlowSummaryNode n1 |
@@ -258,9 +265,7 @@ private module Cached {
258265
(
259266
LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
260267
or
261-
exists(SsaImpl::DefinitionExt def, boolean isUseStep |
262-
SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
263-
|
268+
exists(boolean isUseStep | SsaFlow::localFlowStep(_, nodeFrom, nodeTo, isUseStep) |
264269
isUseStep = false
265270
or
266271
isUseStep = true and
@@ -293,8 +298,8 @@ private module Cached {
293298
}
294299

295300
/** Holds if `n` wraps an SSA definition without ingoing flow. */
296-
private predicate entrySsaDefinition(SsaDefinitionExtNode n) {
297-
n.getDefinitionExt() =
301+
private predicate entrySsaDefinition(SsaDefinitionNodeImpl n) {
302+
n.getDefinition() =
298303
any(SsaImpl::WriteDefinition def | not def.(Ssa::WriteDefinition).assigns(_))
299304
}
300305

@@ -334,7 +339,7 @@ private module Cached {
334339
// to parameters (which are themselves local sources)
335340
entrySsaDefinition(n) and
336341
not exists(SsaImpl::ParameterExt p |
337-
p.isInitializedBy(n.(SsaDefinitionExtNode).getDefinitionExt())
342+
p.isInitializedBy(n.(SsaDefinitionNodeImpl).getDefinition())
338343
)
339344
or
340345
isStoreTargetNode(n)
@@ -419,57 +424,36 @@ predicate nodeIsHidden(Node n) { n.(NodeImpl).nodeIsHidden() }
419424
predicate neverSkipInPathGraph(Node n) { isReturned(n.(AstNode).getCfgNode()) }
420425

421426
/** An SSA node. */
422-
abstract class SsaNode extends NodeImpl, TSsaNode {
427+
class SsaNode extends NodeImpl, TSsaNode {
423428
SsaImpl::DataFlowIntegration::SsaNode node;
424-
SsaImpl::DefinitionExt def;
425429

426-
SsaNode() {
427-
this = TSsaNode(node) and
428-
def = node.getDefinitionExt()
429-
}
430+
SsaNode() { this = TSsaNode(node) }
430431

431-
SsaImpl::DefinitionExt getDefinitionExt() { result = def }
432+
/** Gets the underlying variable. */
433+
Variable getVariable() { result = node.getSourceVariable() }
432434

433435
/** Holds if this node should be hidden from path explanations. */
434-
abstract predicate isHidden();
436+
predicate isHidden() { any() }
437+
438+
override CfgScope getCfgScope() { result = node.getBasicBlock().getScope() }
435439

436440
override Location getLocationImpl() { result = node.getLocation() }
437441

438442
override string toStringImpl() { result = node.toString() }
439443
}
440444

441-
/** An (extended) SSA definition, viewed as a node in a data flow graph. */
442-
class SsaDefinitionExtNode extends SsaNode {
443-
override SsaImpl::DataFlowIntegration::SsaDefinitionExtNode node;
445+
class SsaDefinitionNodeImpl extends SsaNode {
446+
override SsaImpl::DataFlowIntegration::SsaDefinitionNode node;
444447

445-
/** Gets the underlying variable. */
446-
Variable getVariable() { result = def.getSourceVariable() }
448+
Ssa::Definition getDefinition() { result = node.getDefinition() }
447449

448450
override predicate isHidden() {
449-
not def instanceof Ssa::WriteDefinition
450-
or
451-
def = getParameterDef(_)
451+
exists(SsaImpl::Definition def | def = this.getDefinition() |
452+
not def instanceof Ssa::WriteDefinition
453+
or
454+
def = getParameterDef(_)
455+
)
452456
}
453-
454-
override CfgScope getCfgScope() { result = def.getBasicBlock().getScope() }
455-
}
456-
457-
class SsaDefinitionNodeImpl extends SsaDefinitionExtNode {
458-
Ssa::Definition ssaDef;
459-
460-
SsaDefinitionNodeImpl() { ssaDef = def }
461-
462-
override Location getLocationImpl() { result = ssaDef.getLocation() }
463-
464-
override string toStringImpl() { result = ssaDef.toString() }
465-
}
466-
467-
class SsaInputNode extends SsaNode {
468-
override SsaImpl::DataFlowIntegration::SsaInputNode node;
469-
470-
override predicate isHidden() { any() }
471-
472-
override CfgScope getCfgScope() { result = node.getDefinitionExt().getBasicBlock().getScope() }
473457
}
474458

475459
private string getANamedArgument(CfgNodes::ExprNodes::CallExprCfgNode c) {

‎powershell/ql/lib/semmle/code/powershell/dataflow/internal/SsaImpl.qll

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -207,13 +207,15 @@ private module Cached {
207207
import DataFlowIntegrationImpl
208208

209209
cached
210-
predicate localFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
211-
DataFlowIntegrationImpl::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
210+
predicate localFlowStep(
211+
SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep
212+
) {
213+
DataFlowIntegrationImpl::localFlowStep(v, nodeFrom, nodeTo, isUseStep)
212214
}
213215

214216
cached
215-
predicate localMustFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) {
216-
DataFlowIntegrationImpl::localMustFlowStep(def, nodeFrom, nodeTo)
217+
predicate localMustFlowStep(SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo) {
218+
DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo)
217219
}
218220

219221
signature predicate guardChecksSig(Cfg::CfgNodes::AstCfgNode g, Cfg::CfgNode e, boolean branch);
@@ -346,33 +348,34 @@ class ParameterExt extends TParameterExt {
346348
}
347349

348350
private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
349-
class Parameter = ParameterExt;
350-
351351
class Expr extends Cfg::CfgNodes::ExprCfgNode {
352352
predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
353353
}
354354

355355
Expr getARead(Definition def) { result = Cached::getARead(def) }
356356

357-
predicate ssaDefAssigns(WriteDefinition def, Expr value) {
358-
def.(Ssa::WriteDefinition).assigns(value)
357+
predicate ssaDefHasSource(WriteDefinition def) {
358+
any(ParameterExt p).isInitializedBy(def) or def.(Ssa::WriteDefinition).assigns(_)
359359
}
360360

361-
predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) }
362-
363361
class Guard extends Cfg::CfgNodes::AstCfgNode {
364-
predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
362+
/**
363+
* Holds if the control flow branching from `bb1` is dependent on this guard,
364+
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
365+
* guard to `branch`.
366+
*/
367+
predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) {
368+
exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
369+
this.getBasicBlock() = bb1 and
370+
bb2 = bb1.getASuccessor(s) and
371+
s.getValue() = branch
372+
)
373+
}
365374
}
366375

367376
/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
368-
predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) { none() }
369-
370-
/** Gets an immediate conditional successor of basic block `bb`, if any. */
371-
SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) {
372-
exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
373-
result = bb.getASuccessor(s) and
374-
s.getValue() = branch
375-
)
377+
predicate guardDirectlyControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) {
378+
none()
376379
}
377380
}
378381

‎powershell/ql/test/library-tests/dataflow/local/flow.expected

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,9 @@
66
| test.ps1:2:1:2:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
77
| test.ps1:4:1:4:2 | b | test.ps1:5:4:5:5 | b |
88
| test.ps1:4:6:4:12 | Call to GetBool | test.ps1:4:1:4:2 | b |
9-
| test.ps1:5:1:7:1 | phi (a2) | test.ps1:8:6:8:8 | a2 |
109
| test.ps1:5:4:5:5 | b | test.ps1:10:14:10:15 | b |
11-
| test.ps1:6:5:6:7 | a2 | test.ps1:6:11:6:16 | [input] phi (a2) |
10+
| test.ps1:6:5:6:7 | a2 | test.ps1:8:6:8:8 | a2 |
1211
| test.ps1:6:11:6:16 | Call to Source | test.ps1:6:5:6:7 | a2 |
13-
| test.ps1:6:11:6:16 | [input] phi (a2) | test.ps1:5:1:7:1 | phi (a2) |
1412
| test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
1513
| test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
1614
| test.ps1:10:1:10:2 | c | test.ps1:11:6:11:7 | c |

‎powershell/ql/test/library-tests/dataflow/local/taint.expected

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,9 @@
77
| test.ps1:2:1:2:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
88
| test.ps1:4:1:4:2 | b | test.ps1:5:4:5:5 | b |
99
| test.ps1:4:6:4:12 | Call to GetBool | test.ps1:4:1:4:2 | b |
10-
| test.ps1:5:1:7:1 | phi (a2) | test.ps1:8:6:8:8 | a2 |
1110
| test.ps1:5:4:5:5 | b | test.ps1:10:14:10:15 | b |
12-
| test.ps1:6:5:6:7 | a2 | test.ps1:6:11:6:16 | [input] phi (a2) |
11+
| test.ps1:6:5:6:7 | a2 | test.ps1:8:6:8:8 | a2 |
1312
| test.ps1:6:11:6:16 | Call to Source | test.ps1:6:5:6:7 | a2 |
14-
| test.ps1:6:11:6:16 | [input] phi (a2) | test.ps1:5:1:7:1 | phi (a2) |
1513
| test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
1614
| test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
1715
| test.ps1:10:1:10:2 | c | test.ps1:11:6:11:7 | c |

0 commit comments

Comments
 (0)