Merge pull request #13997 from hvitved/ruby/type-tracking-splats

Ruby: Include more (hash) splat flow in type tracking

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

@@ -377,8 +377,7 @@ private module Cached {
 
   class TSourceParameterNode =
     TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or
-        TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode or
-        TSynthSplatParameterElementNode;
+        TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode;
 
   cached
   Location getLocation(NodeImpl n) { result = n.getLocationImpl() }
@@ -480,8 +479,11 @@ private module Cached {
     entrySsaDefinition(n) and
     not LocalFlow::localFlowSsaParamInput(_, n)
     or
-    // Needed for stores in type tracking
     TypeTrackerSpecific::storeStepIntoSourceNode(_, n, _)
+    or
+    TypeTrackerSpecific::readStepIntoSourceNode(_, n, _)
+    or
+    TypeTrackerSpecific::readStoreStepIntoSourceNode(_, n, _, _)
   }
 
   cached
@@ -936,7 +938,7 @@ private module ParameterNodes {
    * A node that holds the content of a specific positional argument.
    * See `SynthSplatArgumentNode` for more information.
    */
-  class SynthSplatParameterElementNode extends ParameterNodeImpl, TSynthSplatParameterElementNode {
+  class SynthSplatParameterElementNode extends NodeImpl, TSynthSplatParameterElementNode {
     private DataFlowCallable callable;
     private int pos;
 
@@ -957,10 +959,6 @@ private module ParameterNodes {
       )
     }
 
-    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 }
@@ -1369,7 +1367,7 @@ private ContentSet getKeywordContent(string name) {
   )
 }
 
-private ContentSet getPositionalContent(int n) {
+ContentSet getPositionalContent(int n) {
   exists(ConstantValue::ConstantIntegerValue i |
     result.isSingleton(TKnownElementContent(i)) and
     i.isInt(n)
@@ -1400,18 +1398,13 @@ predicate storeStepCommon(Node node1, ContentSet c, Node node2) {
     )
   )
   or
+  // Wrap all positional arguments in a synthesized splat argument node
   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())
-    )
 }
 
 /**
@@ -1445,9 +1438,24 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
   FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), c,
     node2.(FlowSummaryNode).getSummaryNode())
   or
+  node1 =
+    any(SynthSplatParameterElementNode elemNode |
+      node2 = elemNode.getSplatParameterNode(_) and
+      c = getPositionalContent(elemNode.getStorePosition())
+    )
+  or
   storeStepCommon(node1, c, node2)
 }
 
+/**
+ * Subset of `readStep` that should be shared with type-tracking.
+ */
+predicate readStepCommon(Node node1, ContentSet c, Node node2) {
+  node2 = node1.(SynthHashSplatParameterNode).getAKeywordParameter(c)
+  or
+  node2 = node1.(SynthSplatParameterNode).getAParameter(c)
+}
+
 /**
  * Holds if there is a read step of content `c` from `node1` to `node2`.
  */
@@ -1475,19 +1483,17 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
         ))
     )
   or
-  node2 = node1.(SynthHashSplatParameterNode).getAKeywordParameter(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())
     )
+  or
+  readStepCommon(node1, c, node2)
 }
 
 /**

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

@@ -295,6 +295,8 @@ predicate storeStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentS
  * Holds if `nodeTo` is the result of accessing the `content` content of `nodeFrom`.
  */
 predicate basicLoadStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
+  readStepIntoSourceNode(nodeFrom, nodeTo, contents)
+  or
   exists(ExprNodes::MethodCallCfgNode call |
     call.getExpr().getNumberOfArguments() = 0 and
     contents.isSingleton(DataFlowPublic::Content::getAttributeName(call.getExpr().getMethodName())) and
@@ -305,15 +307,42 @@ predicate basicLoadStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet content
   TypeTrackerSummaryFlow::basicLoadStep(nodeFrom, nodeTo, contents)
 }
 
+/**
+ * Holds if a read step `nodeFrom -> nodeTo` with `contents` exists, where the destination node
+ * should be treated as a local source node.
+ */
+predicate readStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
+  DataFlowPrivate::readStepCommon(nodeFrom, contents, nodeTo)
+}
+
 /**
  * Holds if the `loadContent` of `nodeFrom` is stored in the `storeContent` of `nodeTo`.
  */
 predicate basicLoadStoreStep(
   Node nodeFrom, Node nodeTo, DataFlow::ContentSet loadContent, DataFlow::ContentSet storeContent
 ) {
+  readStoreStepIntoSourceNode(nodeFrom, nodeTo, loadContent, storeContent)
+  or
   TypeTrackerSummaryFlow::basicLoadStoreStep(nodeFrom, nodeTo, loadContent, storeContent)
 }
 
+/**
+ * Holds if a read+store step `nodeFrom -> nodeTo` exists, where the destination node
+ * should be treated as a local source node.
+ */
+predicate readStoreStepIntoSourceNode(
+  Node nodeFrom, Node nodeTo, DataFlow::ContentSet loadContent, DataFlow::ContentSet storeContent
+) {
+  exists(DataFlowPrivate::SynthSplatParameterElementNode mid |
+    nodeFrom
+        .(DataFlowPrivate::SynthSplatArgParameterNode)
+        .isParameterOf(mid.getEnclosingCallable(), _) and
+    loadContent = DataFlowPrivate::getPositionalContent(mid.getReadPosition()) and
+    nodeTo = mid.getSplatParameterNode(_) and
+    storeContent = DataFlowPrivate::getPositionalContent(mid.getStorePosition())
+  )
+}
+
 /**
  * Holds if type-tracking should step from `nodeFrom` to `nodeTo` but block flow of contents matched by `filter` through here.
  */