Merge pull request #185 from microsoft/fix-ssa-for-powershell-2

PS: Fixup SSA after GitHub's 2.21.0 changes

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

@@ -97,15 +97,22 @@ module SsaFlow {
     or
     result.(Impl::ExprPostUpdateNode).getExpr() = n.(PostUpdateNode).getPreUpdateNode().asExpr()
     or
-    n = toParameterNode(result.(Impl::ParameterNode).getParameter())
+    exists(SsaImpl::ParameterExt p |
+      n = toParameterNode(p) and
+      p.isInitializedBy(result.(Impl::WriteDefSourceNode).getDefinition())
+    )
+    or
+    result.(Impl::WriteDefSourceNode).getDefinition().(Ssa::WriteDefinition).assigns(n.asExpr())
   }
 
-  predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
-    Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
+  predicate localFlowStep(
+    SsaImpl::SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep
+  ) {
+    Impl::localFlowStep(v, asNode(nodeFrom), asNode(nodeTo), isUseStep)
   }
 
-  predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
-    Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo))
+  predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
+    Impl::localMustFlowStep(_, asNode(nodeFrom), asNode(nodeTo))
   }
 }
 
@@ -179,7 +186,7 @@ module LocalFlow {
   }
 
   predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
-    SsaFlow::localMustFlowStep(_, nodeFrom, nodeTo)
+    SsaFlow::localMustFlowStep(nodeFrom, nodeTo)
     or
     nodeFrom =
       unique(FlowSummaryNode n1 |
@@ -258,9 +265,7 @@ private module Cached {
     (
       LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
       or
-      exists(SsaImpl::DefinitionExt def, boolean isUseStep |
-        SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
-      |
+      exists(boolean isUseStep | SsaFlow::localFlowStep(_, nodeFrom, nodeTo, isUseStep) |
         isUseStep = false
         or
         isUseStep = true and
@@ -293,8 +298,8 @@ private module Cached {
   }
 
   /** Holds if `n` wraps an SSA definition without ingoing flow. */
-  private predicate entrySsaDefinition(SsaDefinitionExtNode n) {
-    n.getDefinitionExt() =
+  private predicate entrySsaDefinition(SsaDefinitionNodeImpl n) {
+    n.getDefinition() =
       any(SsaImpl::WriteDefinition def | not def.(Ssa::WriteDefinition).assigns(_))
   }
 
@@ -334,7 +339,7 @@ private module Cached {
     // to parameters (which are themselves local sources)
     entrySsaDefinition(n) and
     not exists(SsaImpl::ParameterExt p |
-      p.isInitializedBy(n.(SsaDefinitionExtNode).getDefinitionExt())
+      p.isInitializedBy(n.(SsaDefinitionNodeImpl).getDefinition())
     )
     or
     isStoreTargetNode(n)
@@ -419,57 +424,36 @@ predicate nodeIsHidden(Node n) { n.(NodeImpl).nodeIsHidden() }
 predicate neverSkipInPathGraph(Node n) { isReturned(n.(AstNode).getCfgNode()) }
 
 /** An SSA node. */
-abstract class SsaNode extends NodeImpl, TSsaNode {
+class SsaNode extends NodeImpl, TSsaNode {
   SsaImpl::DataFlowIntegration::SsaNode node;
-  SsaImpl::DefinitionExt def;
 
-  SsaNode() {
-    this = TSsaNode(node) and
-    def = node.getDefinitionExt()
-  }
+  SsaNode() { this = TSsaNode(node) }
 
-  SsaImpl::DefinitionExt getDefinitionExt() { result = def }
+  /** Gets the underlying variable. */
+  Variable getVariable() { result = node.getSourceVariable() }
 
   /** Holds if this node should be hidden from path explanations. */
-  abstract predicate isHidden();
+  predicate isHidden() { any() }
+
+  override CfgScope getCfgScope() { result = node.getBasicBlock().getScope() }
 
   override Location getLocationImpl() { result = node.getLocation() }
 
   override string toStringImpl() { result = node.toString() }
 }
 
-/** An (extended) SSA definition, viewed as a node in a data flow graph. */
-class SsaDefinitionExtNode extends SsaNode {
-  override SsaImpl::DataFlowIntegration::SsaDefinitionExtNode node;
+class SsaDefinitionNodeImpl extends SsaNode {
+  override SsaImpl::DataFlowIntegration::SsaDefinitionNode node;
 
-  /** Gets the underlying variable. */
-  Variable getVariable() { result = def.getSourceVariable() }
+  Ssa::Definition getDefinition() { result = node.getDefinition() }
 
   override predicate isHidden() {
-    not def instanceof Ssa::WriteDefinition
-    or
-    def = getParameterDef(_)
+    exists(SsaImpl::Definition def | def = this.getDefinition() |
+      not def instanceof Ssa::WriteDefinition
+      or
+      def = getParameterDef(_)
+    )
   }
-
-  override CfgScope getCfgScope() { result = def.getBasicBlock().getScope() }
-}
-
-class SsaDefinitionNodeImpl extends SsaDefinitionExtNode {
-  Ssa::Definition ssaDef;
-
-  SsaDefinitionNodeImpl() { ssaDef = def }
-
-  override Location getLocationImpl() { result = ssaDef.getLocation() }
-
-  override string toStringImpl() { result = ssaDef.toString() }
-}
-
-class SsaInputNode extends SsaNode {
-  override SsaImpl::DataFlowIntegration::SsaInputNode node;
-
-  override predicate isHidden() { any() }
-
-  override CfgScope getCfgScope() { result = node.getDefinitionExt().getBasicBlock().getScope() }
 }
 
 private string getANamedArgument(CfgNodes::ExprNodes::CallExprCfgNode c) {

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

@@ -207,13 +207,15 @@ private module Cached {
     import DataFlowIntegrationImpl
 
     cached
-    predicate localFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
-      DataFlowIntegrationImpl::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
+    predicate localFlowStep(
+      SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep
+    ) {
+      DataFlowIntegrationImpl::localFlowStep(v, nodeFrom, nodeTo, isUseStep)
     }
 
     cached
-    predicate localMustFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) {
-      DataFlowIntegrationImpl::localMustFlowStep(def, nodeFrom, nodeTo)
+    predicate localMustFlowStep(SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo) {
+      DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo)
     }
 
     signature predicate guardChecksSig(Cfg::CfgNodes::AstCfgNode g, Cfg::CfgNode e, boolean branch);
@@ -346,33 +348,34 @@ class ParameterExt extends TParameterExt {
 }
 
 private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
-  class Parameter = ParameterExt;
-
   class Expr extends Cfg::CfgNodes::ExprCfgNode {
     predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
   }
 
   Expr getARead(Definition def) { result = Cached::getARead(def) }
 
-  predicate ssaDefAssigns(WriteDefinition def, Expr value) {
-    def.(Ssa::WriteDefinition).assigns(value)
+  predicate ssaDefHasSource(WriteDefinition def) {
+    any(ParameterExt p).isInitializedBy(def) or def.(Ssa::WriteDefinition).assigns(_)
   }
 
-  predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) }
-
   class Guard extends Cfg::CfgNodes::AstCfgNode {
-    predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
+    /**
+     * Holds if the control flow branching from `bb1` is dependent on this guard,
+     * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
+     * guard to `branch`.
+     */
+    predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) {
+      exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
+        this.getBasicBlock() = bb1 and
+        bb2 = bb1.getASuccessor(s) and
+        s.getValue() = branch
+      )
+    }
   }
 
   /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
-  predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) { none() }
-
-  /** Gets an immediate conditional successor of basic block `bb`, if any. */
-  SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) {
-    exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
-      result = bb.getASuccessor(s) and
-      s.getValue() = branch
-    )
+  predicate guardDirectlyControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) {
+    none()
   }
 }
 

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

@@ -6,11 +6,9 @@
 | test.ps1:2:1:2:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
 | test.ps1:4:1:4:2 | b | test.ps1:5:4:5:5 | b |
 | test.ps1:4:6:4:12 | Call to GetBool | test.ps1:4:1:4:2 | b |
-| test.ps1:5:1:7:1 | phi (a2) | test.ps1:8:6:8:8 | a2 |
 | test.ps1:5:4:5:5 | b | test.ps1:10:14:10:15 | b |
-| test.ps1:6:5:6:7 | a2 | test.ps1:6:11:6:16 | [input] phi (a2) |
+| test.ps1:6:5:6:7 | a2 | test.ps1:8:6:8:8 | a2 |
 | test.ps1:6:11:6:16 | Call to Source | test.ps1:6:5:6:7 | a2 |
-| test.ps1:6:11:6:16 | [input] phi (a2) | test.ps1:5:1:7:1 | phi (a2) |
 | test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
 | test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
 | test.ps1:10:1:10:2 | c | test.ps1:11:6:11:7 | c |

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

@@ -7,11 +7,9 @@
 | test.ps1:2:1:2:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
 | test.ps1:4:1:4:2 | b | test.ps1:5:4:5:5 | b |
 | test.ps1:4:6:4:12 | Call to GetBool | test.ps1:4:1:4:2 | b |
-| test.ps1:5:1:7:1 | phi (a2) | test.ps1:8:6:8:8 | a2 |
 | test.ps1:5:4:5:5 | b | test.ps1:10:14:10:15 | b |
-| test.ps1:6:5:6:7 | a2 | test.ps1:6:11:6:16 | [input] phi (a2) |
+| test.ps1:6:5:6:7 | a2 | test.ps1:8:6:8:8 | a2 |
 | test.ps1:6:11:6:16 | Call to Source | test.ps1:6:5:6:7 | a2 |
-| test.ps1:6:11:6:16 | [input] phi (a2) | test.ps1:5:1:7:1 | phi (a2) |
 | test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
 | test.ps1:8:1:8:8 | Call to Sink | test.ps1:1:1:24:22 | pre-return value for {...} |
 | test.ps1:10:1:10:2 | c | test.ps1:11:6:11:7 | c |