Merge pull request github#11517 from aibaars/phi-reads-in-data-flow-graph

Ruby: Include SSA "phi reads" in DataFlow::Node

Author: hvitved

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

@@ -237,10 +237,10 @@ private predicate selfInToplevel(SelfVariable self, Module m) {
  *
  * the SSA definition for `c` is introduced by matching on `C`.
  */
-private predicate asModulePattern(SsaDefinitionNode def, Module m) {
+private predicate asModulePattern(SsaDefinitionExtNode def, Module m) {
   exists(AsPattern ap |
     m = resolveConstantReadAccess(ap.getPattern()) and
-    def.getDefinition().(Ssa::WriteDefinition).getWriteAccess() = ap.getVariableAccess()
+    def.getDefinitionExt().(Ssa::WriteDefinition).getWriteAccess() = ap.getVariableAccess()
   )
 }
 
@@ -982,15 +982,15 @@ private DataFlow::Node trackSingletonMethodOnInstance(MethodBase method, string
  */
 pragma[nomagic]
 private predicate argMustFlowToReceiver(
-  RelevantCall ctx, DataFlow::LocalSourceNode source, ArgumentNode arg, SsaDefinitionNode paramDef,
-  RelevantCall call, Callable encl, string name
+  RelevantCall ctx, DataFlow::LocalSourceNode source, ArgumentNode arg,
+  SsaDefinitionExtNode paramDef, RelevantCall call, Callable encl, string name
 ) {
   exists(ParameterNodeImpl p, ParameterPosition ppos, ArgumentPosition apos |
     // the receiver of `call` references `p`
     exists(DataFlow::Node receiver |
       LocalFlow::localFlowSsaParamInput(p, paramDef) and
       methodCall(pragma[only_bind_into](call), receiver, pragma[only_bind_into](name)) and
-      receiver.asExpr() = paramDef.getDefinition().getARead()
+      receiver.asExpr() = paramDef.getDefinitionExt().(Ssa::Definition).getARead()
     ) and
     // `p` is a parameter of `encl`,
     encl = call.getScope() and

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

@@ -75,44 +75,54 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {
 module LocalFlow {
   private import codeql.ruby.dataflow.internal.SsaImpl
 
+  /** An SSA definition into which another SSA definition may flow. */
+  private class SsaInputDefinitionExtNode extends SsaDefinitionExtNode {
+    SsaInputDefinitionExtNode() {
+      def instanceof Ssa::PhiNode
+      or
+      def instanceof SsaImpl::PhiReadNode
+      //TODO: or def instanceof LocalFlow::UncertainExplicitSsaDefinition
+    }
+  }
+
   /**
    * Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`.
    */
   private predicate localFlowSsaInputFromDef(
-    SsaDefinitionNode nodeFrom, Ssa::Definition def, Ssa::Definition next
+    SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputDefinitionExtNode next
   ) {
     exists(BasicBlock bb, int i |
-      lastRefBeforeRedef(def, bb, i, next) and
-      def = nodeFrom.getDefinition() and
-      def.definesAt(_, bb, i)
+      lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
+      def = nodeFrom.getDefinitionExt() and
+      def.definesAt(_, bb, i, _)
     )
   }
 
   /**
    * Holds if `exprFrom` is a last read of SSA definition `def`, which
    * can reach `next`.
    */
-  predicate localFlowSsaInputFromExpr(
-    CfgNodes::ExprCfgNode exprFrom, Ssa::Definition def, Ssa::Definition next
+  predicate localFlowSsaInputFromRead(
+    CfgNodes::ExprCfgNode exprFrom, SsaImpl::DefinitionExt def, SsaInputDefinitionExtNode next
   ) {
     exists(BasicBlock bb, int i |
-      lastRefBeforeRedef(def, bb, i, next) and
+      SsaImpl::lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
       exprFrom = bb.getNode(i) and
       exprFrom.getExpr() instanceof VariableReadAccess
     )
   }
 
   /** Gets the SSA definition node corresponding to parameter `p`. */
-  SsaDefinitionNode getParameterDefNode(NamedParameter p) {
+  SsaDefinitionExtNode getParameterDefNode(NamedParameter p) {
     exists(BasicBlock bb, int i |
       bb.getNode(i).getNode() = p.getDefiningAccess() and
-      result.getDefinition().definesAt(_, bb, i)
+      result.getDefinitionExt().definesAt(_, bb, i, _)
     )
   }
 
   /** Gets the SSA definition node corresponding to the implicit `self` parameter for `m`. */
-  private SsaDefinitionNode getSelfParameterDefNode(MethodBase m) {
-    result.getDefinition().(Ssa::SelfDefinition).getSourceVariable().getDeclaringScope() = m
+  private SsaDefinitionExtNode getSelfParameterDefNode(MethodBase m) {
+    result.getDefinitionExt().(Ssa::SelfDefinition).getSourceVariable().getDeclaringScope() = m
   }
 
   /**
@@ -136,41 +146,37 @@ module LocalFlow {
       or
       nodeFrom.(SelfParameterNode).getSelfVariable() = def.getSourceVariable()
     |
-      nodeTo.(SsaDefinitionNode).getDefinition() = def
+      nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def
     )
   }
 
   /**
    * Holds if there is a local use-use flow step from `nodeFrom` to `nodeTo`
    * involving SSA definition `def`.
    */
-  predicate localSsaFlowStepUseUse(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
-    def.hasAdjacentReads(nodeFrom.asExpr(), nodeTo.asExpr())
+  predicate localSsaFlowStepUseUse(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
+    SsaImpl::adjacentReadPairExt(def, nodeFrom.asExpr(), nodeTo.asExpr())
   }
 
   /**
    * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving
    * SSA definition `def`.
    */
   private predicate localSsaFlowStep(Node nodeFrom, Node nodeTo) {
-    exists(Ssa::Definition def |
+    exists(SsaImpl::DefinitionExt def |
       // Flow from assignment into SSA definition
       def.(Ssa::WriteDefinition).assigns(nodeFrom.asExpr()) and
-      nodeTo.(SsaDefinitionNode).getDefinition() = def
+      nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def
       or
       // Flow from SSA definition to first read
-      def = nodeFrom.(SsaDefinitionNode).getDefinition() and
-      nodeTo.asExpr() = def.getAFirstRead()
+      def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and
+      firstReadExt(def, nodeTo.asExpr())
       or
       // Flow from read to next read
       localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
       or
-      // Flow into phi node from definition
-      exists(Ssa::PhiNode phi |
-        localFlowSsaInputFromDef(nodeFrom, def, phi) and
-        phi = nodeTo.(SsaDefinitionNode).getDefinition() and
-        def = phi.getAnInput()
-      )
+      // Flow into phi (read) SSA definition node from def
+      localFlowSsaInputFromDef(nodeFrom, def, nodeTo)
     )
     // TODO
     // or
@@ -287,7 +293,7 @@ private module Cached {
         ret.getKind() = kind
       )
     } or
-    TSsaDefinitionNode(Ssa::Definition def) or
+    TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) or
     TNormalParameterNode(Parameter p) {
       p instanceof SimpleParameter or
       p instanceof OptionalParameter or
@@ -352,10 +358,8 @@ private module Cached {
     not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
     or
     // Flow into phi node from read
-    exists(Ssa::Definition def, Ssa::PhiNode phi, CfgNodes::ExprCfgNode exprFrom |
-      LocalFlow::localFlowSsaInputFromExpr(exprFrom, def, phi) and
-      phi = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = phi.getAnInput()
+    exists(SsaImpl::DefinitionExt def, CfgNodes::ExprCfgNode exprFrom |
+      LocalFlow::localFlowSsaInputFromRead(exprFrom, def, nodeTo)
     |
       exprFrom = nodeFrom.asExpr() and
       not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
@@ -400,18 +404,16 @@ private module Cached {
     LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
     or
     // Flow into phi node from read
-    exists(Ssa::Definition def, Ssa::PhiNode phi, CfgNodes::ExprCfgNode exprFrom |
-      LocalFlow::localFlowSsaInputFromExpr(exprFrom, def, phi) and
-      phi = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = phi.getAnInput() and
+    exists(SsaImpl::DefinitionExt def, CfgNodes::ExprCfgNode exprFrom |
+      LocalFlow::localFlowSsaInputFromRead(exprFrom, def, nodeTo) and
       exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()]
     )
   }
 
-  private predicate entrySsaDefinition(SsaDefinitionNode n) {
+  private predicate entrySsaDefinition(SsaDefinitionExtNode n) {
     n = LocalFlow::getParameterDefNode(_)
     or
-    exists(Ssa::Definition def | def = n.getDefinition() |
+    exists(SsaImpl::DefinitionExt def | def = n.getDefinitionExt() |
       def instanceof Ssa::SelfDefinition
       or
       def instanceof Ssa::CapturedEntryDefinition
@@ -520,8 +522,9 @@ import Cached
 
 /** Holds if `n` should be hidden from path explanations. */
 predicate nodeIsHidden(Node n) {
-  exists(Ssa::Definition def | def = n.(SsaDefinitionNode).getDefinition() |
+  exists(SsaImpl::DefinitionExt def | def = n.(SsaDefinitionExtNode).getDefinitionExt() |
     def instanceof Ssa::PhiNode or
+    def instanceof SsaImpl::PhiReadNode or
     def instanceof Ssa::CapturedEntryDefinition or
     def instanceof Ssa::CapturedCallDefinition
   )
@@ -542,13 +545,13 @@ predicate nodeIsHidden(Node n) {
 }
 
 /** An SSA definition, viewed as a node in a data flow graph. */
-class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode {
-  Ssa::Definition def;
+class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode {
+  SsaImpl::DefinitionExt def;
 
-  SsaDefinitionNode() { this = TSsaDefinitionNode(def) }
+  SsaDefinitionExtNode() { this = TSsaDefinitionExtNode(def) }
 
   /** Gets the underlying SSA definition. */
-  Ssa::Definition getDefinition() { result = def }
+  SsaImpl::DefinitionExt getDefinitionExt() { result = def }
 
   /** Gets the underlying variable. */
   Variable getVariable() { result = def.getSourceVariable() }
@@ -561,7 +564,7 @@ class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode {
 }
 
 /** An SSA definition for a `self` variable. */
-class SsaSelfDefinitionNode extends LocalSourceNode, SsaDefinitionNode {
+class SsaSelfDefinitionNode extends LocalSourceNode, SsaDefinitionExtNode {
   private SelfVariable self;
 
   SsaSelfDefinitionNode() { self = def.getSourceVariable() }
@@ -1078,11 +1081,11 @@ private module OutNodes {
 import OutNodes
 
 predicate jumpStep(Node pred, Node succ) {
-  SsaImpl::captureFlowIn(_, pred.(SsaDefinitionNode).getDefinition(),
-    succ.(SsaDefinitionNode).getDefinition())
+  SsaImpl::captureFlowIn(_, pred.(SsaDefinitionExtNode).getDefinitionExt(),
+    succ.(SsaDefinitionExtNode).getDefinitionExt())
   or
-  SsaImpl::captureFlowOut(_, pred.(SsaDefinitionNode).getDefinition(),
-    succ.(SsaDefinitionNode).getDefinition())
+  SsaImpl::captureFlowOut(_, pred.(SsaDefinitionExtNode).getDefinitionExt(),
+    succ.(SsaDefinitionExtNode).getDefinitionExt())
   or
   succ.asExpr().getExpr().(ConstantReadAccess).getValue() = pred.asExpr().getExpr()
   or

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

@@ -373,7 +373,7 @@ private module Cached {
   LocalSourceNode getConstantAccessNode(ConstantAccess access) {
     // Namespaces don't evaluate to the constant being accessed, they return the value of their last statement.
     // Use the definition of 'self' in the namespace as the representative in this case.
-    result.(SsaDefinitionNode).getDefinition().(Ssa::SelfDefinition).getSourceVariable() =
+    result.(SsaDefinitionExtNode).getDefinitionExt().(Ssa::SelfDefinition).getSourceVariable() =
       access.(Namespace).getModuleSelfVariable()
     or
     not access instanceof Namespace and
@@ -819,7 +819,7 @@ class ModuleNode instanceof Module {
    * This only gets `self` at the module level, not inside any (singleton) method.
    */
   LocalSourceNode getModuleLevelSelf() {
-    result.(SsaDefinitionNode).getVariable() = super.getADeclaration().getModuleSelfVariable()
+    result.(SsaDefinitionExtNode).getVariable() = super.getADeclaration().getModuleSelfVariable()
   }
 
   /**