Ruby: Revert changes to isLocalSourceNode and localFlowStepTypeTracker

Instead, use small-step type tracking, as suggested by @RasmusWL offline.

Author: hvitved

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

@@ -186,9 +186,9 @@ private predicate superCall(CfgNodes::ExprNodes::CallCfgNode call, Module superC
 
 pragma[nomagic]
 private predicate instanceMethodCall(CfgNodes::ExprNodes::CallCfgNode call, Module tp, string method) {
-  exists(DataFlow::LocalSourceNode sourceNode, Module m, boolean exact |
-    flowsToMethodCall(call, sourceNode, method) and
-    sourceNode = trackInstance(m, exact)
+  exists(DataFlow::Node receiver, Module m, boolean exact |
+    methodCall(call, receiver, method) and
+    receiver = trackInstance(m, exact)
   |
     tp = m
     or
@@ -239,7 +239,7 @@ private predicate selfInToplevel(SelfVariable self, Module m) {
  *
  * the SSA definition for `c` is introduced by matching on `C`.
  */
-predicate asModulePattern(SsaDefinitionNode def, Module m) {
+private predicate asModulePattern(SsaDefinitionNode def, Module m) {
   exists(AsPattern ap |
     m = resolveConstantReadAccess(ap.getPattern()) and
     def.getDefinition().(Ssa::WriteDefinition).getWriteAccess() = ap.getVariableAccess()
@@ -258,7 +258,7 @@ predicate asModulePattern(SsaDefinitionNode def, Module m) {
  *
  * the two reads of `object` are adjacent, and the second is checked to have type `C`.
  */
-predicate hasAdjacentTypeCheckedReads(
+private predicate hasAdjacentTypeCheckedReads(
   Ssa::Definition def, CfgNodes::ExprCfgNode read1, CfgNodes::ExprCfgNode read2, Module m
 ) {
   exists(
@@ -322,11 +322,9 @@ private module Cached {
         // def c.singleton; end # <- result
         // c.singleton          # <- call
         // ```
-        exists(DataFlow::Node sourceNode |
-          methodCall(call, sourceNode, method) or
-          flowsToMethodCall(call, sourceNode, method)
-        |
-          sourceNode = trackSingletonMethodOnInstance(result, method)
+        exists(DataFlow::Node receiver |
+          methodCall(call, receiver, method) and
+          receiver = trackSingletonMethodOnInstance(result, method)
         )
         or
         // singleton method defined on a module
@@ -445,7 +443,7 @@ private DataFlow::LocalSourceNode trackModuleAccess(Module m) {
 }
 
 pragma[nomagic]
-private DataFlow::LocalSourceNode trackInstance(Module tp, boolean exact, TypeTracker t) {
+private DataFlow::Node trackInstance(Module tp, boolean exact, TypeTracker t) {
   t.start() and
   (
     result.asExpr().getExpr() instanceof NilLiteral and
@@ -554,23 +552,30 @@ private DataFlow::LocalSourceNode trackInstance(Module tp, boolean exact, TypeTr
   )
 }
 
+private predicate localFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {
+  localFlowStepTypeTracker(nodeFrom, nodeTo) and
+  summary.toString() = "level"
+}
+
 /**
  * We exclude steps into `self` parameters and type checked variables. For those,
  * we instead rely on the type of the enclosing module resp. the type being checked
  * against, and apply an open-world assumption when determining possible dispatch
  * targets.
  */
 pragma[nomagic]
-private DataFlow::LocalSourceNode trackInstanceRec(
-  Module tp, TypeTracker t, boolean exact, StepSummary summary
-) {
-  StepSummary::step(trackInstance(tp, exact, t), result, summary) and
+private DataFlow::Node trackInstanceRec(Module tp, TypeTracker t, boolean exact, StepSummary summary) {
+  exists(DataFlow::Node mid | mid = trackInstance(tp, exact, t) |
+    StepSummary::smallstep(mid, result, summary)
+    or
+    localFlowStep(mid, result, summary)
+  ) and
   not result instanceof SelfParameterNode and
   not hasAdjacentTypeCheckedReads(_, _, result.asExpr(), _)
 }
 
 pragma[nomagic]
-private DataFlow::LocalSourceNode trackInstance(Module tp, boolean exact) {
+private DataFlow::Node trackInstance(Module tp, boolean exact) {
   result = trackInstance(tp, exact, TypeTracker::end())
 }
 
@@ -683,16 +688,6 @@ predicate singletonMethodOnInstance(MethodBase method, string name, Expr object)
   not exists(resolveConstantReadAccess(object))
 }
 
-/**
- * Same as `singletonMethodOnInstance`, but where `n` is the post-update node
- * of the object that is the target of the singleton method `method`.
- */
-predicate singletonMethodOnInstancePostUpdate(
-  MethodBase method, string name, DataFlow::PostUpdateNode n
-) {
-  singletonMethodOnInstance(method, name, n.getPreUpdateNode().asExpr().getExpr())
-}
-
 /**
  * Holds if there is reverse flow from `nodeFrom` to `nodeTo` via a parameter.
  *
@@ -723,44 +718,46 @@ predicate singletonMethodOnInstancePostUpdate(
  * ```
  */
 pragma[nomagic]
-private predicate paramReturnFlow(DataFlow::PostUpdateNode nodeFrom, DataFlow::PostUpdateNode nodeTo) {
+private predicate paramReturnFlow(
+  DataFlow::Node nodeFrom, DataFlow::PostUpdateNode nodeTo, StepSummary summary
+) {
   exists(
     CfgNodes::ExprNodes::CallCfgNode call, DataFlow::Node arg, DataFlow::ParameterNode p,
     Expr nodeFromPreExpr
   |
     TypeTrackerSpecific::callStep(call, arg, p) and
     nodeTo.getPreUpdateNode() = arg and
-    nodeFromPreExpr = nodeFrom.getPreUpdateNode().asExpr().getExpr()
+    summary.toString() = "return" and
+    (
+      nodeFromPreExpr = nodeFrom.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr()
+      or
+      nodeFromPreExpr = nodeFrom.asExpr().getExpr() and
+      singletonMethodOnInstance(_, _, nodeFromPreExpr)
+    )
   |
     nodeFromPreExpr = p.getParameter().(NamedParameter).getVariable().getAnAccess()
     or
     nodeFromPreExpr = p.(SelfParameterNode).getSelfVariable().getAnAccess()
   )
 }
 
-// Since post-update nodes are not (and should not be) `LocalSourceNode`s in general,
-// we need to do local flow manually
-pragma[nomagic]
-private predicate argPostUpdateFlowsTo(DataFlow::PostUpdateNode arg, DataFlow::Node n) {
-  paramReturnFlow(_, arg) and
-  n = arg
-  or
-  exists(DataFlow::Node mid |
-    argPostUpdateFlowsTo(arg, mid) and
-    localFlowStepTypeTracker(mid, n)
-  )
-}
-
 pragma[nomagic]
 private DataFlow::Node trackSingletonMethodOnInstance(MethodBase method, string name, TypeTracker t) {
   t.start() and
-  singletonMethodOnInstancePostUpdate(method, name, result)
+  singletonMethodOnInstance(method, name, result.asExpr().getExpr())
   or
   exists(TypeTracker t2, StepSummary summary |
     result = trackSingletonMethodOnInstanceRec(method, name, t2, summary) and
     t = t2.append(summary) and
-    // do not step over redefinitions
-    not singletonMethodOnInstancePostUpdate(_, name, result)
+    // Stop flow at redefinitions.
+    //
+    // Example:
+    // ```rb
+    // def x.foo; end
+    // def x.foo; end
+    // x.foo # <- we want to resolve this call to the second definition only
+    // ```
+    not singletonMethodOnInstance(_, name, result.asExpr().getExpr())
   )
 }
 
@@ -769,15 +766,11 @@ private DataFlow::Node trackSingletonMethodOnInstanceRec(
   MethodBase method, string name, TypeTracker t, StepSummary summary
 ) {
   exists(DataFlow::Node mid | mid = trackSingletonMethodOnInstance(method, name, t) |
-    StepSummary::step(mid, result, summary)
+    StepSummary::smallstep(mid, result, summary)
     or
-    // include flow out through parameters
-    paramReturnFlow(mid, result) and
-    summary.toString() = "return"
+    paramReturnFlow(mid, result, summary)
     or
-    // include flow starting from an output argument
-    argPostUpdateFlowsTo(mid, result) and
-    summary.toString() = "level"
+    localFlowStep(mid, result, summary)
   )
 }
 

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

@@ -242,8 +242,7 @@ private module Cached {
       isNonConstantExpr(n) and
       (
         n instanceof Argument or
-        n = any(CfgNodes::ExprNodes::InstanceVariableAccessCfgNode v).getReceiver() or
-        singletonMethodOnInstance(_, _, n.getExpr())
+        n = any(CfgNodes::ExprNodes::InstanceVariableAccessCfgNode v).getReceiver()
       )
     } or
     TSummaryNode(
@@ -315,40 +314,7 @@ private module Cached {
       nodeTo = LocalFlow::getParameterDefNode(p)
     )
     or
-    exists(Ssa::Definition def |
-      LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
-      // For nodes that are the target of a singleton method definition, such
-      // as `x` in `def x.foo; end`, we disallow use-use flow out of `x`, and
-      // instead add a type-tracker level-step from `x` to the post-update node
-      // of `x` (which does allow further use-use flow).
-      //
-      // This enables us to stregthen call resolution for singleton methods, since
-      // we can stop flow at redefinitions, which would otherwise not be possible,
-      // as type-tracking would step over such redefinitions.
-      //
-      // Example:
-      // ```rb
-      // def x.foo; end
-      // def x.foo; end
-      // x.foo # <- we want to resolve this call to the second definition only
-      // ```
-      not singletonMethodOnInstance(_, _, nodeFrom.asExpr().getExpr()) and
-      // We disallow adjacent use-use steps, where the target is type checked, and
-      // instead add a type-tracker level-step.
-      //
-      // This enables us to strengthen call resolution for instance methods, since
-      // we can use the extra type information on the target node.
-      //
-      // Example:
-      // ```rb
-      // case object
-      //   when C then object.foo # <- we want to resolve this call as if it was a call inside `C`
-      // end
-      // ```
-      //
-      // The second access to `object` is known to have type `C` (or a sub-type thereof).
-      not hasAdjacentTypeCheckedReads(def, nodeFrom.asExpr(), nodeTo.asExpr(), _)
-    )
+    LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
   }
 
   private predicate entrySsaDefinition(SsaDefinitionNode n) {
@@ -393,15 +359,6 @@ private module Cached {
     or
     // Needed for stores in type tracking
     TypeTrackerSpecific::basicStoreStep(_, n, _)
-    or
-    // Needed to be able to track singleton methods defined on instances
-    singletonMethodOnInstancePostUpdate(_, _, n)
-    or
-    // Needed to be able to track instance methods on variables introduced via pattern matching
-    asModulePattern(n, _)
-    or
-    // Needed to be able to (better) track instance methods on variables that are type checked
-    hasAdjacentTypeCheckedReads(_, _, n.asExpr(), _)
   }
 
   cached

ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll

@@ -35,16 +35,7 @@ private predicate summarizedLocalStep(Node nodeFrom, Node nodeTo) {
 }
 
 /** Holds if there is a level step from `nodeFrom` to `nodeTo`. */
-predicate levelStep(Node nodeFrom, Node nodeTo) {
-  summarizedLocalStep(nodeFrom, nodeTo)
-  or
-  // See comment in `localFlowStepTypeTracker/2`
-  nodeTo.(DataFlowPublic::PostUpdateNode).getPreUpdateNode() = nodeFrom and
-  DataFlowDispatch::singletonMethodOnInstance(_, _, nodeFrom.asExpr().getExpr())
-  or
-  // See comment in `localFlowStepTypeTracker/2`
-  DataFlowDispatch::hasAdjacentTypeCheckedReads(_, nodeFrom.asExpr(), nodeTo.asExpr(), _)
-}
+predicate levelStep(Node nodeFrom, Node nodeTo) { summarizedLocalStep(nodeFrom, nodeTo) }
 
 /**
  * Gets the name of a possible piece of content. This will usually include things like