Merge pull request github#13938 from hmac/splat-flow-2

Ruby: More precise flow into splat parameters

Author: hmac

ruby/ql/lib/change-notes/2023-08-14-splat-parameters.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Flow between positional arguments and splat parameters (`*args`) is now tracked more precisely.

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

@@ -440,6 +440,7 @@ private module Cached {
     THashSplatArgumentPosition() or
     TSplatAllArgumentPosition() or
     TSplatArgumentPosition(int pos) { exists(Call c | c.getArgument(pos) instanceof SplatExpr) } or
+    TSynthSplatArgumentPosition() or
     TAnyArgumentPosition() or
     TAnyKeywordArgumentPosition()
 
@@ -473,6 +474,7 @@ private module Cached {
       exists(Parameter p | p.getPosition() = pos and p instanceof SplatParameter)
     } or
     TSynthSplatParameterPosition() or
+    TSynthArgSplatParameterPosition() or
     TAnyParameterPosition() or
     TAnyKeywordParameterPosition()
 }
@@ -1295,6 +1297,9 @@ class ParameterPosition extends TParameterPosition {
 
   predicate isSynthSplat() { this = TSynthSplatParameterPosition() }
 
+  // A fake position to indicate that this parameter node holds content from a synth arg splat node
+  predicate isSynthArgSplat() { this = TSynthArgSplatParameterPosition() }
+
   predicate isSplatAll() { this = TSplatAllParameterPosition() }
 
   predicate isSplat(int n) { this = TSplatParameterPosition(n) }
@@ -1332,6 +1337,8 @@ class ParameterPosition extends TParameterPosition {
     or
     this.isSynthSplat() and result = "synthetic *"
     or
+    this.isSynthArgSplat() and result = "synthetic * (from *args)"
+    or
     exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
   }
 }
@@ -1369,6 +1376,8 @@ class ArgumentPosition extends TArgumentPosition {
 
   predicate isSplat(int n) { this = TSplatArgumentPosition(n) }
 
+  predicate isSynthSplat() { this = TSynthSplatArgumentPosition() }
+
   /** Gets a textual representation of this position. */
   string toString() {
     this.isSelf() and result = "self"
@@ -1387,6 +1396,8 @@ class ArgumentPosition extends TArgumentPosition {
     or
     this.isSplatAll() and result = "*"
     or
+    this.isSynthSplat() and result = "synthetic *"
+    or
     exists(int pos | this.isSplat(pos) and result = "* (position " + pos + ")")
   }
 }
@@ -1418,8 +1429,12 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   or
   ppos.isSplatAll() and apos.isSplatAll()
   or
+  ppos.isSplatAll() and apos.isSynthSplat()
+  or
   ppos.isSynthSplat() and apos.isSplatAll()
   or
+  apos.isSynthSplat() and ppos.isSynthArgSplat()
+  or
   // Exact splat match
   exists(int n | apos.isSplat(n) and ppos.isSplat(n))
   or

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

@@ -344,6 +344,15 @@ private module Cached {
       exists(c.asCallable()) and // exclude library callables
       isParameterNode(_, c, any(ParameterPosition p | p.isPositional(_)))
     } or
+    TSynthSplatArgParameterNode(DataFlowCallable c) {
+      exists(c.asCallable()) and // exclude library callables
+      isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_)))
+    } or
+    TSynthSplatParameterElementNode(DataFlowCallable c, int n) {
+      exists(c.asCallable()) and // exclude library callables
+      isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_))) and
+      n in [0 .. 10]
+    } or
     TExprPostUpdateNode(CfgNodes::ExprCfgNode n) {
       // filter out nodes that clearly don't need post-update nodes
       isNonConstantExpr(n) and
@@ -358,11 +367,18 @@ private module Cached {
       exists(Argument arg | arg.isArgumentOf(c, any(ArgumentPosition pos | pos.isKeyword(_))))
       or
       c.getAnArgument() instanceof CfgNodes::ExprNodes::PairCfgNode
+    } or
+    TSynthSplatArgumentNode(CfgNodes::ExprNodes::CallCfgNode c) {
+      exists(Argument arg, ArgumentPosition pos | pos.isPositional(_) | arg.isArgumentOf(c, pos)) and
+      not exists(Argument arg, ArgumentPosition pos | pos.isSplat(_) or pos.isSplatAll() |
+        arg.isArgumentOf(c, pos)
+      )
     }
 
   class TSourceParameterNode =
     TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or
-        TSynthHashSplatParameterNode or TSynthSplatParameterNode;
+        TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode or
+        TSynthSplatParameterElementNode;
 
   cached
   Location getLocation(NodeImpl n) { result = n.getLocationImpl() }
@@ -562,6 +578,12 @@ predicate nodeIsHidden(Node n) {
   n instanceof SynthHashSplatArgumentNode
   or
   n instanceof SynthSplatParameterNode
+  or
+  n instanceof SynthSplatArgumentNode
+  or
+  n instanceof SynthSplatArgParameterNode
+  or
+  n instanceof SynthSplatParameterElementNode
 }
 
 /** An SSA definition, viewed as a node in a data flow graph. */
@@ -885,6 +907,69 @@ private module ParameterNodes {
     final override string toStringImpl() { result = "synthetic *args" }
   }
 
+  /**
+   * A node that holds all positional arguments passed in a call to `c`.
+   * This is a mirror of the `SynthSplatArgumentNode` on the callable side.
+   * See `SynthSplatArgumentNode` for more information.
+   */
+  class SynthSplatArgParameterNode extends ParameterNodeImpl, TSynthSplatArgParameterNode {
+    private DataFlowCallable callable;
+
+    SynthSplatArgParameterNode() { this = TSynthSplatArgParameterNode(callable) }
+
+    final override Parameter getParameter() { none() }
+
+    final override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
+      c = callable and pos.isSynthArgSplat()
+    }
+
+    final override CfgScope getCfgScope() { result = callable.asCallable() }
+
+    final override DataFlowCallable getEnclosingCallable() { result = callable }
+
+    final override Location getLocationImpl() { result = callable.getLocation() }
+
+    final override string toStringImpl() { result = "synthetic *args" }
+  }
+
+  /**
+   * A node that holds the content of a specific positional argument.
+   * See `SynthSplatArgumentNode` for more information.
+   */
+  class SynthSplatParameterElementNode extends ParameterNodeImpl, TSynthSplatParameterElementNode {
+    private DataFlowCallable callable;
+    private int pos;
+
+    SynthSplatParameterElementNode() { this = TSynthSplatParameterElementNode(callable, pos) }
+
+    pragma[nomagic]
+    NormalParameterNode getSplatParameterNode(int splatPos) {
+      result
+          .isParameterOf(this.getEnclosingCallable(), any(ParameterPosition p | p.isSplat(splatPos)))
+    }
+
+    int getStorePosition() { result = pos }
+
+    int getReadPosition() {
+      exists(int splatPos |
+        exists(this.getSplatParameterNode(splatPos)) and
+        result = pos + splatPos
+      )
+    }
+
+    final override Parameter getParameter() { none() }
+
+    final override predicate isParameterOf(DataFlowCallable c, ParameterPosition p) { none() }
+
+    final override CfgScope getCfgScope() { result = callable.asCallable() }
+
+    final override DataFlowCallable getEnclosingCallable() { result = callable }
+
+    final override Location getLocationImpl() { result = callable.getLocation() }
+
+    final override string toStringImpl() { result = "synthetic *args[" + pos + "]" }
+  }
+
   /** A parameter for a library callable with a flow summary. */
   class SummaryParameterNode extends ParameterNodeImpl, FlowSummaryNode {
     private ParameterPosition pos_;
@@ -1030,6 +1115,51 @@ private module ArgumentNodes {
 
     override string toStringImpl() { result = "**" }
   }
+
+  /**
+   * A data-flow node that represents all arguments passed to the call.
+   * We use this to model data flow via splat parameters.
+   * Consider this example:
+   *
+   * ```rb
+   * def foo(x, y, *z)
+   * end
+   *
+   * foo(1, 2, 3, 4)
+   * ```
+   *
+   * 1. We want `3` to flow to `z[0]` and `4` to flow to `z[1]`. We model this by first storing all arguments
+   *   in a synthetic argument node `SynthSplatArgumentNode` (see `storeStepCommon`).
+   * 2. We match this to an analogous parameter node `SynthSplatArgParameterNode` on the callee side
+   *   (see `parameterMatch`).
+   * 3. For each content element stored in the `SynthSplatArgParameterNode`, we add a read step to a separate
+   *   `SynthSplatParameterElementNode`, which is parameterized by the element index (see `readStep`).
+   * 4. Finally, we add store steps from these `SynthSplatParameterElementNode`s to the real splat parameter node
+   *   (see `storeStep`).
+   *   We only add store steps for elements that will not flow to the earlier positional parameters.
+   *   In practice that means we ignore elements at index `<= N`, where `N` is the index of the splat parameter.
+   *   For the remaining elements we subtract `N` from their index and store them in the splat parameter.
+   */
+  class SynthSplatArgumentNode extends ArgumentNode, NodeImpl, TSynthSplatArgumentNode {
+    CfgNodes::ExprNodes::CallCfgNode c;
+
+    SynthSplatArgumentNode() { this = TSynthSplatArgumentNode(c) }
+
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
+      this.sourceArgumentOf(call.asCall(), pos)
+    }
+
+    override predicate sourceArgumentOf(CfgNodes::ExprNodes::CallCfgNode call, ArgumentPosition pos) {
+      call = c and
+      pos.isSynthSplat()
+    }
+
+    override CfgScope getCfgScope() { result = c.getExpr().getCfgScope() }
+
+    override Location getLocationImpl() { result = c.getLocation() }
+
+    override string toStringImpl() { result = "*" }
+  }
 }
 
 import ArgumentNodes
@@ -1269,6 +1399,19 @@ predicate storeStepCommon(Node node1, ContentSet c, Node node2) {
       c.isSingleton(TKnownElementContent(cv))
     )
   )
+  or
+  exists(CfgNodes::ExprNodes::CallCfgNode call, ArgumentPosition pos |
+    node2 = TSynthSplatArgumentNode(call) and
+    node1.asExpr().(Argument).isArgumentOf(call, pos)
+  |
+    exists(int n | pos.isPositional(n) and c = getPositionalContent(n))
+  )
+  or
+  node1 =
+    any(SynthSplatParameterElementNode elemNode |
+      node2 = elemNode.getSplatParameterNode(_) and
+      c = getPositionalContent(elemNode.getStorePosition())
+    )
 }
 
 /**
@@ -1334,10 +1477,17 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
   or
   node2 = node1.(SynthHashSplatParameterNode).getAKeywordParameter(c)
   or
-  node2 = node1.(SynthSplatParameterNode).getAParameter(c)
-  or
   FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), c,
     node2.(FlowSummaryNode).getSummaryNode())
+  or
+  node2 = node1.(SynthSplatParameterNode).getAParameter(c)
+  or
+  // Read from SynthSplatArgParameterNode into SynthSplatParameterElementNode
+  node2 =
+    any(SynthSplatParameterElementNode e |
+      node1.(SynthSplatArgParameterNode).isParameterOf(e.getEnclosingCallable(), _) and
+      c = getPositionalContent(e.getReadPosition())
+    )
 }
 
 /**