Ruby: Improvments to splat flow

- Only step through a `SynthSplatParameterElementNode` when there is a splat parameter at index > 0. - Model read+stores via `SynthSplatArgumentElementNode` as a single read-store step in type tracking.
github · Sep 13, 2023 · 7cedb54 · 7cedb54
1 parent e7c27a8
commit 7cedb54
Show file tree

Hide file tree

Showing 3 changed files with 419 additions and 263 deletions.
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
@@ -341,7 +341,7 @@ private module Cached {
     } or
     TSynthSplatParameterElementNode(DataFlowCallable c, int n) {
       exists(c.asCallable()) and // exclude library callables
-      isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_))) and
+      isParameterNode(_, c, any(ParameterPosition p | p.isSplat(any(int i | i > 0)))) and
       n in [0 .. 10]
     } or
     TExprPostUpdateNode(CfgNodes::ExprCfgNode n) {
@@ -1047,7 +1047,7 @@ private module ArgumentNodes {
    * part of the method signature, such that those cannot end up in the hash-splat
    * parameter.
    */
-  class SynthHashSplatArgumentNode extends ArgumentNode, TSynthHashSplatArgumentNode {
+  class SynthHashSplatArgumentNode extends ArgumentNode, NodeImpl, TSynthHashSplatArgumentNode {
     CfgNodes::ExprNodes::CallCfgNode c;
 
     SynthHashSplatArgumentNode() { this = TSynthHashSplatArgumentNode(c) }
@@ -1060,12 +1060,6 @@ private module ArgumentNodes {
       call = c and
       pos.isHashSplat()
     }
-  }
-
-  private class SynthHashSplatArgumentNodeImpl extends NodeImpl, TSynthHashSplatArgumentNode {
-    CfgNodes::ExprNodes::CallCfgNode c;
-
-    SynthHashSplatArgumentNodeImpl() { this = TSynthHashSplatArgumentNode(c) }
 
     override CfgScope getCfgScope() { result = c.getExpr().getCfgScope() }
 
@@ -1384,6 +1378,23 @@ predicate storeStepCommon(Node node1, ContentSet c, Node node2) {
   )
 }
 
+// Store from TSynthSplatArgumentElementNode(n)
+// into TSynthSplatArgumentNode[n]
+predicate synthSplatArgumentElementStoreStep(
+  SynthSplatArgumentElementNode node1, ContentSet c, SynthSplatArgumentNode node2
+) {
+  exists(CfgNodes::ExprNodes::CallCfgNode call, int n |
+    node2 = TSynthSplatArgumentNode(call) and
+    node1 = TSynthSplatArgumentElementNode(call, n) and
+    (
+      c = getPositionalContent(n)
+      or
+      n = -1 and
+      c.isSingleton(TUnknownElementContent())
+    )
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via an assignment to
  * content `c`.
@@ -1421,18 +1432,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
     c = getPositionalContent(elemNode.getStorePosition() - splatPos)
   )
   or
-  // Store from TSynthSplatArgumentElementNode(n)
-  // into TSynthSplatArgumentNode[n]
-  exists(CfgNodes::ExprNodes::CallCfgNode call, int n |
-    node2 = TSynthSplatArgumentNode(call) and
-    node1 = TSynthSplatArgumentElementNode(call, n) and
-    (
-      c = getPositionalContent(n)
-      or
-      n = -1 and
-      c.isSingleton(TUnknownElementContent())
-    )
-  )
+  synthSplatArgumentElementStoreStep(node1, c, node2)
   or
   storeStepCommon(node1, c, node2)
 }
@@ -1444,19 +1444,6 @@ predicate readStepCommon(Node node1, ContentSet c, Node node2) {
   node2 = node1.(SynthHashSplatParameterNode).getAKeywordParameter(c)
   or
   node2 = node1.(SynthSplatParameterNode).getAParameter(c)
-  or
-  // TODO: convert into the above form
-  synthSplatArgumentElementReadStep(node1, c, node2)
-  or
-  // read from SynthSplatParameterNode[n] to nth positional parameter
-  exists(SynthSplatParameterNode paramNode, NormalParameterNode posNode, int n |
-    paramNode = node1 and
-    posNode = node2 and
-    posNode
-        .isParameterOf(paramNode.getEnclosingCallable(),
-          any(ParameterPosition p | p.isPositional(n))) and
-    c = getPositionalContent(n)
-  )
 }
 
 // read from splat arg to synth splat arg element
@@ -1516,6 +1503,9 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
       c = getPositionalContent(e.getReadPosition())
     )
   or
+  // TODO: convert into the above form
+  synthSplatArgumentElementReadStep(node1, c, node2)
+  or
   readStepCommon(node1, c, node2)
 }
 

diff --git a/ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll b/ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll
@@ -339,6 +339,11 @@ predicate readStoreStepIntoSourceNode(
     nodeTo = mid.getSplatParameterNode(_) and
     storeContent = DataFlowPrivate::getPositionalContent(mid.getStorePosition())
   )
+  or
+  exists(DataFlowPrivate::SynthSplatArgumentElementNode mid |
+    DataFlowPrivate::synthSplatArgumentElementReadStep(nodeFrom, loadContent, mid) and
+    DataFlowPrivate::synthSplatArgumentElementStoreStep(mid, storeContent, nodeTo)
+  )
 }
 
 /**