Ruby: ensure Hash flow works again

Author: asgerf

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

@@ -381,7 +381,7 @@ private module Cached {
     n instanceof SynthReturnNode
     or
     // Needed for stores in type tracking
-    TypeTrackerSpecific::postUpdateStoreStep(_, n, _)
+    TypeTrackerSpecific::storeStepIntoSourceNode(_, n, _)
   }
 
   cached
@@ -1010,6 +1010,31 @@ private ContentSet getKeywordContent(string name) {
   )
 }
 
+/**
+ * Subset of `storeStep` that should be shared with type-tracking.
+ */
+predicate storeStepCommon(Node node1, ContentSet c, Node node2) {
+  // Wrap all key-value arguments in a synthesized hash-splat argument node
+  exists(CfgNodes::ExprNodes::CallCfgNode call | node2 = TSynthHashSplatArgumentNode(call) |
+    // symbol key
+    exists(ArgumentPosition keywordPos, string name |
+      node1.asExpr().(Argument).isArgumentOf(call, keywordPos) and
+      keywordPos.isKeyword(name) and
+      c = getKeywordContent(name)
+    )
+    or
+    // non-symbol key
+    exists(CfgNodes::ExprNodes::PairCfgNode pair, CfgNodes::ExprCfgNode key, ConstantValue cv |
+      node1.asExpr() = pair.getValue() and
+      pair = call.getAnArgument() and
+      key = pair.getKey() and
+      cv = key.getConstantValue() and
+      not cv.isSymbol(_) and
+      c.isSingleton(TKnownElementContent(cv))
+    )
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via an assignment to
  * content `c`.
@@ -1040,25 +1065,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
   or
   FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2)
   or
-  // Wrap all key-value arguments in a synthesized hash-splat argument node
-  exists(CfgNodes::ExprNodes::CallCfgNode call | node2 = TSynthHashSplatArgumentNode(call) |
-    // symbol key
-    exists(ArgumentPosition keywordPos, string name |
-      node1.asExpr().(Argument).isArgumentOf(call, keywordPos) and
-      keywordPos.isKeyword(name) and
-      c = getKeywordContent(name)
-    )
-    or
-    // non-symbol key
-    exists(CfgNodes::ExprNodes::PairCfgNode pair, CfgNodes::ExprCfgNode key, ConstantValue cv |
-      node1.asExpr() = pair.getValue() and
-      pair = call.getAnArgument() and
-      key = pair.getKey() and
-      cv = key.getConstantValue() and
-      not cv.isSymbol(_) and
-      c.isSingleton(TKnownElementContent(cv))
-    )
-  )
+  storeStepCommon(node1, c, node2)
 }
 
 /**

ruby/ql/lib/codeql/ruby/frameworks/Core.qll

@@ -77,7 +77,7 @@ private class SplatSummary extends SummarizedCallable {
 private class HashSplatSummary extends SummarizedCallable {
   HashSplatSummary() { this = "**(hash-splat)" }
 
-  override HashSplatExpr getACall() { any() }
+  override HashSplatExpr getACallSimple() { any() }
 
   override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
     input = "Argument[self].WithElement[any]" and

ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll

@@ -7,7 +7,6 @@ private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.FlowSummary
 private import codeql.ruby.dataflow.internal.DataFlowDispatch
 private import codeql.ruby.ast.internal.Module
-private import codeql.ruby.typetracking.TypeTrackerSpecific
 
 /**
  * Provides flow summaries for the `Hash` class.
@@ -42,50 +41,6 @@ module Hash {
     }
   }
 
-  /** Holds if `literal` is a call to `Hash.[]` and `argument` is one of its arguments. */
-  private predicate hashLiteralStore(DataFlow::CallNode literal, DataFlow::Node argument) {
-    literal.getExprNode().getExpr() = getAStaticHashCall("[]") and
-    argument = literal.getArgument(_)
-  }
-
-  /**
-   * A set of type-tracking steps to replace the `Hash.[]` summary.
-   *
-   * The `Hash.[]` method tends to have a large number of summaries, which would result
-   * in too many unnecessary type-tracking edges, so we specialize it here.
-   */
-  private class HashLiteralTypeTracker extends TypeTrackingStep {
-    override predicate suppressSummary(SummarizedCallable callable) {
-      callable instanceof HashLiteralSummary
-    }
-
-    override predicate storeStep(Node pred, TypeTrackingNode succ, TypeTrackerContent content) {
-      // Store edge: `value -> { key: value }` with content derived from `key`
-      exists(Cfg::CfgNodes::ExprNodes::PairCfgNode pair |
-        hashLiteralStore(succ, any(DataFlow::Node n | n.asExpr() = pair)) and
-        pred.asExpr() = pair.getValue()
-      |
-        exists(ConstantValue constant |
-          constant = pair.getKey().getConstantValue() and
-          content.isSingleton(DataFlow::Content::getElementContent(constant))
-        )
-        or
-        not exists(pair.getKey().getConstantValue()) and
-        content.isAnyElement()
-      )
-    }
-
-    override predicate withContentStep(Node pred, Node succ, ContentFilter filter) {
-      // `WithContent[element]` edge: `args --> { **args }`.
-      exists(DataFlow::Node node |
-        hashLiteralStore(succ, node) and
-        node.asExpr().getExpr() instanceof HashSplatExpr and
-        pred.asExpr() = node.asExpr().(Cfg::CfgNodes::ExprNodes::UnaryOperationCfgNode).getOperand() and
-        filter = ContentFilter::hasElements()
-      )
-    }
-  }
-
   /**
    * `Hash[]` called on an existing hash, e.g.
    *

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

@@ -209,7 +209,7 @@ predicate returnStep(Node nodeFrom, Node nodeTo) {
  * called.
  */
 predicate basicStoreStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
-  postUpdateStoreStep(nodeFrom, nodeTo, contents)
+  storeStepIntoSourceNode(nodeFrom, nodeTo, contents)
   or
   exists(
     SummarizedCallable callable, DataFlowPublic::CallNode call, SummaryComponentStack input,
@@ -229,7 +229,7 @@ predicate basicStoreStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet conten
  * Holds if a store step `nodeFrom -> nodeTo` with `contents` exists, where the destination node
  * is a post-update node that should be treated as a local source node.
  */
-predicate postUpdateStoreStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
+predicate storeStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
   // TODO: support SetterMethodCall inside TuplePattern
   exists(ExprNodes::MethodCallCfgNode call |
     contents
@@ -241,6 +241,8 @@ predicate postUpdateStoreStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet c
     call.getArgument(call.getNumberOfArguments() - 1) =
       nodeFrom.(DataFlowPublic::ExprNode).getExprNode()
   )
+  or
+  DataFlowPrivate::storeStepCommon(nodeFrom, contents, nodeTo)
 }
 
 /**

ruby/ql/test/library-tests/dataflow/type-tracker/TypeTracker.expected

@@ -114,39 +114,48 @@ track
 | type_tracker.rb:27:5:27:11 | call to puts | type tracker without call steps | type_tracker.rb:30:1:30:21 | call to keyword |
 | type_tracker.rb:27:5:27:11 | call to puts | type tracker without call steps | type_tracker.rb:31:1:31:21 | call to keyword |
 | type_tracker.rb:27:5:27:11 | call to puts | type tracker without call steps | type_tracker.rb:32:1:32:27 | call to keyword |
+| type_tracker.rb:30:1:30:21 | ** | type tracker without call steps | type_tracker.rb:30:1:30:21 | ** |
 | type_tracker.rb:30:1:30:21 | call to keyword | type tracker without call steps | type_tracker.rb:30:1:30:21 | call to keyword |
 | type_tracker.rb:30:9:30:10 | :p1 | type tracker without call steps | type_tracker.rb:30:9:30:10 | :p1 |
 | type_tracker.rb:30:9:30:13 | Pair | type tracker without call steps | type_tracker.rb:30:9:30:13 | Pair |
 | type_tracker.rb:30:13:30:13 | 3 | type tracker with call steps | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:30:13:30:13 | 3 | type tracker with call steps | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:30:13:30:13 | 3 | type tracker without call steps | type_tracker.rb:30:13:30:13 | 3 |
+| type_tracker.rb:30:13:30:13 | 3 | type tracker without call steps with content element :p1 | type_tracker.rb:30:1:30:21 | ** |
 | type_tracker.rb:30:16:30:17 | :p2 | type tracker without call steps | type_tracker.rb:30:16:30:17 | :p2 |
 | type_tracker.rb:30:16:30:20 | Pair | type tracker without call steps | type_tracker.rb:30:16:30:20 | Pair |
 | type_tracker.rb:30:20:30:20 | 4 | type tracker with call steps | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:30:20:30:20 | 4 | type tracker with call steps | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:30:20:30:20 | 4 | type tracker without call steps | type_tracker.rb:30:20:30:20 | 4 |
+| type_tracker.rb:30:20:30:20 | 4 | type tracker without call steps with content element :p2 | type_tracker.rb:30:1:30:21 | ** |
+| type_tracker.rb:31:1:31:21 | ** | type tracker without call steps | type_tracker.rb:31:1:31:21 | ** |
 | type_tracker.rb:31:1:31:21 | call to keyword | type tracker without call steps | type_tracker.rb:31:1:31:21 | call to keyword |
 | type_tracker.rb:31:9:31:10 | :p2 | type tracker without call steps | type_tracker.rb:31:9:31:10 | :p2 |
 | type_tracker.rb:31:9:31:13 | Pair | type tracker without call steps | type_tracker.rb:31:9:31:13 | Pair |
 | type_tracker.rb:31:13:31:13 | 5 | type tracker with call steps | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:31:13:31:13 | 5 | type tracker with call steps | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:31:13:31:13 | 5 | type tracker without call steps | type_tracker.rb:31:13:31:13 | 5 |
+| type_tracker.rb:31:13:31:13 | 5 | type tracker without call steps with content element :p2 | type_tracker.rb:31:1:31:21 | ** |
 | type_tracker.rb:31:16:31:17 | :p1 | type tracker without call steps | type_tracker.rb:31:16:31:17 | :p1 |
 | type_tracker.rb:31:16:31:20 | Pair | type tracker without call steps | type_tracker.rb:31:16:31:20 | Pair |
 | type_tracker.rb:31:20:31:20 | 6 | type tracker with call steps | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:31:20:31:20 | 6 | type tracker with call steps | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:31:20:31:20 | 6 | type tracker without call steps | type_tracker.rb:31:20:31:20 | 6 |
+| type_tracker.rb:31:20:31:20 | 6 | type tracker without call steps with content element :p1 | type_tracker.rb:31:1:31:21 | ** |
+| type_tracker.rb:32:1:32:27 | ** | type tracker without call steps | type_tracker.rb:32:1:32:27 | ** |
 | type_tracker.rb:32:1:32:27 | call to keyword | type tracker without call steps | type_tracker.rb:32:1:32:27 | call to keyword |
 | type_tracker.rb:32:9:32:11 | :p2 | type tracker without call steps | type_tracker.rb:32:9:32:11 | :p2 |
 | type_tracker.rb:32:9:32:16 | Pair | type tracker without call steps | type_tracker.rb:32:9:32:16 | Pair |
 | type_tracker.rb:32:16:32:16 | 7 | type tracker with call steps | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:32:16:32:16 | 7 | type tracker with call steps | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:32:16:32:16 | 7 | type tracker without call steps | type_tracker.rb:32:16:32:16 | 7 |
+| type_tracker.rb:32:16:32:16 | 7 | type tracker without call steps with content element :p2 | type_tracker.rb:32:1:32:27 | ** |
 | type_tracker.rb:32:19:32:21 | :p1 | type tracker without call steps | type_tracker.rb:32:19:32:21 | :p1 |
 | type_tracker.rb:32:19:32:26 | Pair | type tracker without call steps | type_tracker.rb:32:19:32:26 | Pair |
 | type_tracker.rb:32:26:32:26 | 8 | type tracker with call steps | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:32:26:32:26 | 8 | type tracker with call steps | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:32:26:32:26 | 8 | type tracker without call steps | type_tracker.rb:32:26:32:26 | 8 |
+| type_tracker.rb:32:26:32:26 | 8 | type tracker without call steps with content element :p1 | type_tracker.rb:32:1:32:27 | ** |
 | type_tracker.rb:34:1:53:3 | &block | type tracker without call steps | type_tracker.rb:34:1:53:3 | &block |
 | type_tracker.rb:34:1:53:3 | return return in throughArray | type tracker without call steps | type_tracker.rb:34:1:53:3 | return return in throughArray |
 | type_tracker.rb:34:1:53:3 | self in throughArray | type tracker without call steps | type_tracker.rb:34:1:53:3 | self in throughArray |
@@ -483,6 +492,7 @@ trackEnd
 | type_tracker.rb:27:5:27:11 | call to puts | type_tracker.rb:30:1:30:21 | call to keyword |
 | type_tracker.rb:27:5:27:11 | call to puts | type_tracker.rb:31:1:31:21 | call to keyword |
 | type_tracker.rb:27:5:27:11 | call to puts | type_tracker.rb:32:1:32:27 | call to keyword |
+| type_tracker.rb:30:1:30:21 | ** | type_tracker.rb:30:1:30:21 | ** |
 | type_tracker.rb:30:1:30:21 | call to keyword | type_tracker.rb:30:1:30:21 | call to keyword |
 | type_tracker.rb:30:9:30:10 | :p1 | type_tracker.rb:30:9:30:10 | :p1 |
 | type_tracker.rb:30:9:30:13 | Pair | type_tracker.rb:30:9:30:13 | Pair |
@@ -496,6 +506,7 @@ trackEnd
 | type_tracker.rb:30:20:30:20 | 4 | type_tracker.rb:25:18:25:19 | p2 |
 | type_tracker.rb:30:20:30:20 | 4 | type_tracker.rb:27:10:27:11 | p2 |
 | type_tracker.rb:30:20:30:20 | 4 | type_tracker.rb:30:20:30:20 | 4 |
+| type_tracker.rb:31:1:31:21 | ** | type_tracker.rb:31:1:31:21 | ** |
 | type_tracker.rb:31:1:31:21 | call to keyword | type_tracker.rb:31:1:31:21 | call to keyword |
 | type_tracker.rb:31:9:31:10 | :p2 | type_tracker.rb:31:9:31:10 | :p2 |
 | type_tracker.rb:31:9:31:13 | Pair | type_tracker.rb:31:9:31:13 | Pair |
@@ -509,6 +520,7 @@ trackEnd
 | type_tracker.rb:31:20:31:20 | 6 | type_tracker.rb:25:13:25:14 | p1 |
 | type_tracker.rb:31:20:31:20 | 6 | type_tracker.rb:26:10:26:11 | p1 |
 | type_tracker.rb:31:20:31:20 | 6 | type_tracker.rb:31:20:31:20 | 6 |
+| type_tracker.rb:32:1:32:27 | ** | type_tracker.rb:32:1:32:27 | ** |
 | type_tracker.rb:32:1:32:27 | call to keyword | type_tracker.rb:32:1:32:27 | call to keyword |
 | type_tracker.rb:32:9:32:11 | :p2 | type_tracker.rb:32:9:32:11 | :p2 |
 | type_tracker.rb:32:9:32:16 | Pair | type_tracker.rb:32:9:32:16 | Pair |