PS: Add some implicit reads at process blocks, and fix CFG for process blocks so that these reads appear in the CFG.

Author: MathiasVP

powershell/ql/lib/semmle/code/powershell/ast/internal/ChildIndex.qll

@@ -33,11 +33,14 @@ newtype ChildIndex =
   } or
   ThisVar() or
   PipelineParamVar() or
-  // PipelineByPropertNameVar(Raw::PipelineByPropertyNameParameter p) or
+  PipelineByPropertyNameVar(Raw::PipelineByPropertyNameParameter p) or
   PipelineIteratorVar() or
   PipelineByPropertyNameIteratorVar(Raw::PipelineByPropertyNameParameter p) or
   RealVar(string name) { name = variableNameInScope(_, _) } or
-  ProcessBlockPipelineVarReadAccess()
+  ProcessBlockPipelineVarReadAccess() or
+  ProcessBlockPipelineByPropertyNameVarReadAccess(string name) {
+    name = any(Raw::PipelineByPropertyNameParameter p).getName()
+  }
 
 int synthPipelineParameterChildIndex(Raw::ScriptBlock sb) {
   // If there is a parameter block, but no pipeline parameter
@@ -340,3 +343,7 @@ ChildIndex whileStmtCond() { result = RawChildIndex(Raw::WhileStmtCond()) }
 ChildIndex whileStmtBody() { result = RawChildIndex(Raw::WhileStmtBody()) }
 
 ChildIndex processBlockPipelineVarReadAccess() { result = ProcessBlockPipelineVarReadAccess() }
+
+ChildIndex processBlockPipelineByPropertyNameVarReadAccess(string name) {
+  result = ProcessBlockPipelineByPropertyNameVarReadAccess(name)
+}

powershell/ql/lib/semmle/code/powershell/ast/internal/NamedBlock.qll

@@ -58,7 +58,20 @@ class ProcessBlock extends NamedBlock {
     synthChild(getRawAst(this), processBlockPipelineVarReadAccess(), result)
   }
 
+  PipelineByPropertyNameParameter getPipelineByPropertyNameParameter(string name) {
+    result = scriptBlock.getAParameter() and
+    result.getPropertyName() = name
+  }
+
   PipelineByPropertyNameParameter getAPipelineByPropertyNameParameter() {
-    result = scriptBlock.getEnclosingFunction().getAParameter()
+    result = this.getPipelineByPropertyNameParameter(_)
+  }
+
+  VarReadAccess getPipelineByPropertyNameParameterAccess(string name) {
+    synthChild(getRawAst(this), processBlockPipelineByPropertyNameVarReadAccess(name), result)
+  }
+
+  VarReadAccess getAPipelineByPropertyNameParameterAccess() {
+    result = this.getPipelineByPropertyNameParameterAccess(_)
   }
 }

powershell/ql/lib/semmle/code/powershell/ast/internal/Synthesis.qll

@@ -615,7 +615,7 @@ private module LiteralSynth {
 
   /**
    * Holds if `va` is an access to the automatic variable named `name`.
-   * 
+   *
    * Unlike `Raw::isAutomaticVariableAccess`, this predicate also checks for
    * shadowing.
    */
@@ -770,6 +770,13 @@ private module IteratorAccessSynth {
         or
         va.getUserPath().toLowerCase() =
           pb.getScriptBlock().getParamBlock().getPipelineParameter().getName().toLowerCase()
+        or
+        va.getUserPath().toLowerCase() =
+          pb.getScriptBlock()
+              .getParamBlock()
+              .getAPipelineByPropertyNameParameter()
+              .getName()
+              .toLowerCase()
       )
     }
 
@@ -867,12 +874,19 @@ private module PipelineAccess {
           pipelineVar = TVariableSynth(pb.getScriptBlock(), PipelineParamVar()) and
           child = SynthChild(VarAccessSynthKind(pipelineVar))
         )
+        or
+        exists(PipelineByPropertyNameVariable pipelineVar, Raw::PipelineByPropertyNameParameter p |
+          i = processBlockPipelineByPropertyNameVarReadAccess(p.getName()) and
+          getResultAst(p) = pipelineVar and
+          child = SynthChild(VarAccessSynthKind(pipelineVar))
+        )
       )
     }
 
     final override Location getLocation(Ast n) {
       exists(ProcessBlock pb |
-        pb.getPipelineParameterAccess() = n and
+        pb.getPipelineParameterAccess() = n or pb.getAPipelineByPropertyNameParameterAccess() = n
+      |
         result = pb.getLocation()
       )
     }
@@ -881,6 +895,11 @@ private module PipelineAccess {
       exists(ProcessBlock pb |
         pb.getPipelineParameterAccess() = va and
         v = pb.getPipelineParameter()
+        or
+        exists(string name |
+          pb.getPipelineByPropertyNameParameterAccess(name) = va and
+          v = pb.getPipelineByPropertyNameParameter(name)
+        )
       )
     }
   }

powershell/ql/lib/semmle/code/powershell/ast/internal/Variable.qll

@@ -45,7 +45,9 @@ module Private {
 
   class ParameterImpl extends VariableSynth {
     ParameterImpl() {
-      i instanceof FunParam or i instanceof PipelineParamVar or i instanceof ThisVar
+      i instanceof FunParam or
+      i instanceof PipelineParamVar or
+      i instanceof ThisVar
     }
   }
 
@@ -59,6 +61,14 @@ module Private {
     ScriptBlock getScriptBlock() { this = TVariableSynth(getRawAst(result), _) }
   }
 
+  class PipelineByPropertyNameVariableImpl extends ParameterImpl {
+    PipelineByPropertyNameVariableImpl() {
+      getRawAst(this) instanceof Raw::PipelineByPropertyNameParameter
+    }
+
+    ScriptBlock getScriptBlock() { this = TVariableSynth(getRawAst(result), _) }
+  }
+
   class PipelineIteratorVariableImpl extends VariableSynth {
     override PipelineIteratorVar i;
 
@@ -171,6 +181,11 @@ module Public {
     ScriptBlock getScriptBlock() { result = super.getScriptBlock() }
   }
 
+  class PipelineByPropertyNameVariable extends Variable instanceof PipelineByPropertyNameVariableImpl
+  {
+    ScriptBlock getScriptBlock() { result = super.getScriptBlock() }
+  }
+
   class PipelineIteratorVariable extends Variable instanceof PipelineIteratorVariableImpl {
     ProcessBlock getProcessBlock() { result = super.getProcessBlock() }
   }

powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll

@@ -277,6 +277,8 @@ private class ProcessBlockChildMapping extends NamedBlockChildMapping, ProcessBl
     super.relevantChild(child)
     or
     child = super.getPipelineParameterAccess()
+    or
+    child = super.getAPipelineByPropertyNameParameterAccess()
   }
 }
 
@@ -300,6 +302,23 @@ class ProcessBlockCfgNode extends NamedBlockCfgNode {
   PipelineIteratorVariable getPipelineIteratorVariable() {
     result.getProcessBlock().getScriptBlock() = this.getScriptBlock().getAstNode()
   }
+
+  PipelineByPropertyNameIteratorVariable getPipelineBypropertyNameIteratorVariable(string name) {
+    result.getPropertyName() = name and
+    result.getProcessBlock().getScriptBlock() = this.getScriptBlock().getAstNode()
+  }
+
+  PipelineByPropertyNameIteratorVariable getAPipelineBypropertyNameIteratorVariable() {
+    result = this.getPipelineBypropertyNameIteratorVariable(_)
+  }
+
+  ExprNodes::VarReadAccessCfgNode getPipelineByPropertyNameParameterAccess(string name) {
+    block.hasCfgChild(block.getPipelineByPropertyNameParameterAccess(name), this, result)
+  }
+
+  ExprNodes::VarReadAccessCfgNode getAPipelineByPropertyNameParameterAccess() {
+    result = this.getPipelineByPropertyNameParameterAccess(_)
+  }
 }
 
 private class CatchClauseChildMapping extends NonExprChildMapping, CatchClause {

powershell/ql/lib/semmle/code/powershell/controlflow/internal/Completion.qll

@@ -188,7 +188,11 @@ private predicate inMatchingContext(Ast n) {
  * Holds if a normal completion of `cfe` must be an emptiness completion. Thats is,
  * whether `cfe` determines whether to execute the body of a `foreach` statement.
  */
-private predicate mustHaveEmptinessCompletion(Ast n) { n instanceof ForEachStmt }
+private predicate mustHaveEmptinessCompletion(Ast n) {
+  n instanceof ForEachStmt
+  or
+  any(CfgImpl::Trees::ProcessBlockTree pbtree).lastEmptinessCheck(n)
+}
 
 /**
  * A completion that represents normal evaluation of a statement or an

powershell/ql/lib/semmle/code/powershell/controlflow/internal/ControlFlowGraphImpl.qll

@@ -221,13 +221,7 @@ module Trees {
       or
       last(super.getProcessBlock(), pred, c) and
       completionIsNormal(c) and
-      (
-        // If we process multiple items we will loop back to the process block
-        first(super.getProcessBlock(), succ)
-        or
-        // Once we're done process all items we will go to the end block
-        first(super.getEndBlock(), succ)
-      )
+      first(super.getEndBlock(), succ)
     }
 
     final override predicate propagatesAbnormal(AstNode child) {
@@ -292,25 +286,17 @@ module Trees {
     final override predicate succEntry(Ast n, Completion c) { n = this and completionIsSimple(c) }
   }
 
-  abstract class NamedBlockTreeBase extends ControlFlowTree instanceof NamedBlock {
-    final override predicate last(Ast last, Completion c) {
+  abstract class NamedBlockTreeBase extends PreOrderTree instanceof NamedBlock {
+    override predicate last(Ast last, Completion c) {
       exists(int i | last(super.getStmt(i), last, c) |
         completionIsNormal(c) and
         not exists(super.getStmt(i + 1))
         or
         not completionIsNormal(c)
       )
-      or
-      not exists(super.getAStmt()) and
-      completionIsSimple(c) and
-      last = this
     }
 
     override predicate succ(Ast pred, Ast succ, Completion c) {
-      pred = this and
-      completionIsSimple(c) and
-      first(super.getStmt(0), succ)
-      or
       exists(int i |
         last(super.getStmt(i), pred, c) and
         completionIsNormal(c) and
@@ -324,20 +310,85 @@ module Trees {
   class NamedBlockTree extends NamedBlockTreeBase instanceof NamedBlock {
     NamedBlockTree() { not this instanceof ProcessBlock }
 
-    final override predicate first(Ast first) { first = this }
+    final override predicate last(Ast last, Completion c) {
+      super.last(last, c)
+      or
+      not exists(super.getAStmt()) and
+      completionIsSimple(c) and
+      last = this
+    }
+
+    final override predicate succ(Ast pred, Ast succ, Completion c) {
+      pred = this and
+      completionIsSimple(c) and
+      first(super.getStmt(0), succ)
+      or
+      super.succ(pred, succ, c)
+    }
 
     final override predicate propagatesAbnormal(Ast child) { super.propagatesAbnormal(child) }
   }
 
+  private VarAccess getRankedPipelineByPropertyNameVariable(ProcessBlock pb, int i) {
+    result =
+      rank[i + 1](string name | | pb.getPipelineByPropertyNameParameterAccess(name) order by name)
+  }
+
   class ProcessBlockTree extends NamedBlockTreeBase instanceof ProcessBlock {
-    final override predicate first(Ast first) { first = super.getPipelineParameterAccess() }
+    predicate lastEmptinessCheck(AstNode last) {
+      last = super.getPipelineParameterAccess() and
+      not exists(super.getAPipelineByPropertyNameParameterAccess())
+      or
+      exists(int i |
+        last = getRankedPipelineByPropertyNameVariable(this, i) and
+        not exists(getRankedPipelineByPropertyNameVariable(this, i + 1))
+      )
+    }
+
+    private predicate succEmptinessCheck(AstNode pred, AstNode succ, Completion c) {
+      last(super.getPipelineParameterAccess(), pred, c) and
+      first(getRankedPipelineByPropertyNameVariable(this, 0), succ)
+      or
+      exists(int i |
+        last(getRankedPipelineByPropertyNameVariable(this, i), pred, c) and
+        first(getRankedPipelineByPropertyNameVariable(this, i + 1), succ)
+      )
+    }
+
+    private predicate firstEmptinessCheck(AstNode first) {
+      first(super.getPipelineParameterAccess(), first)
+    }
+
+    final override predicate last(AstNode last, Completion c) {
+      // Emptiness test exits with no more elements
+      this.lastEmptinessCheck(last) and
+      c.(EmptinessCompletion).isEmpty()
+      or
+      super.last(last, c)
+    }
 
     final override predicate succ(Ast pred, Ast succ, Completion c) {
-      this.first(pred) and
-      completionIsSimple(c) and
-      succ = this
+      // Evaluate the pipeline access
+      pred = this and
+      this.firstEmptinessCheck(succ) and
+      completionIsSimple(c)
+      or
+      this.succEmptinessCheck(pred, succ, c)
+      or
+      this.lastEmptinessCheck(pred) and
+      c = any(EmptinessCompletion ec | not ec.isEmpty()) and
+      first(super.getStmt(0), succ)
       or
       super.succ(pred, succ, c)
+      or
+      // Body to emptiness test
+      exists(Ast last0 |
+        super.last(last0, _) and
+        last(last0, pred, c) and
+        // TODO: I don't think this correctly models the semantics inside process blocks
+        c.continuesLoop()
+      ) and
+      this.firstEmptinessCheck(succ)
     }
 
     final override predicate propagatesAbnormal(Ast child) { super.propagatesAbnormal(child) }