Ruby: move special treatment of Hash.[] into Hash.qll

Author: asgerf

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

@@ -1,11 +1,13 @@
 /** Provides flow summaries for the `Hash` class. */
 
 private import codeql.ruby.AST
+private import codeql.ruby.CFG as Cfg
 private import codeql.ruby.ApiGraphs
 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.
@@ -68,6 +70,50 @@ 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() = Hash::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

@@ -10,7 +10,6 @@ private import codeql.ruby.dataflow.internal.SsaImpl as SsaImpl
 private import codeql.ruby.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 private import codeql.ruby.dataflow.internal.FlowSummaryImplSpecific as FlowSummaryImplSpecific
 private import codeql.ruby.dataflow.internal.AccessPathSyntax
-private import codeql.ruby.frameworks.core.Hash
 
 class Node = DataFlowPublic::Node;
 
@@ -61,6 +60,15 @@ class ContentFilter extends TContentFilter {
   }
 }
 
+/** Module for getting `ContentFilter` values. */
+module ContentFilter {
+  /** Gets the filter that only allow element contents. */
+  ContentFilter hasElements() { result = MkElementFilter() }
+
+  /** Gets the filter that only allow pair-value contents. */
+  ContentFilter hasPairValue() { result = MkPairValueFilter() }
+}
+
 /**
  * Holds if a value stored with `storeContents` can be read back with `loadContents`.
  */
@@ -226,28 +234,9 @@ predicate basicStoreStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet conten
     nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
   )
   or
-  // Hash literals
-  exists(Cfg::CfgNodes::ExprNodes::PairCfgNode pair |
-    hashLiteralStore(nodeTo, any(DataFlow::Node n | n.asExpr() = pair)) and
-    nodeFrom.asExpr() = pair.getValue()
-  |
-    exists(ConstantValue constant |
-      constant = pair.getKey().getConstantValue() and
-      contents.isSingleton(DataFlow::Content::getElementContent(constant))
-    )
-    or
-    not exists(pair.getKey().getConstantValue()) and
-    contents.isAnyElement()
-  )
-  or
   TypeTrackingStep::storeStep(nodeFrom, nodeTo, contents)
 }
 
-private predicate hashLiteralStore(DataFlow::CallNode hashCreation, DataFlow::Node argument) {
-  hashCreation.getExprNode().getExpr() = Hash::getAStaticHashCall("[]") and
-  argument = hashCreation.getArgument(_)
-}
-
 /**
  * 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.
@@ -343,14 +332,6 @@ predicate basicWithContentStep(Node nodeFrom, Node nodeTo, ContentFilter filter)
     nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
   )
   or
-  // Hash-splat in a hash literal
-  exists(DataFlow::Node node |
-    hashLiteralStore(nodeTo, node) and
-    node.asExpr().getExpr() instanceof HashSplatExpr and
-    nodeFrom.asExpr() = node.asExpr().(Cfg::CfgNodes::ExprNodes::UnaryOperationCfgNode).getOperand() and
-    filter = MkElementFilter()
-  )
-  or
   TypeTrackingStep::withContentStep(nodeFrom, nodeTo, filter)
 }
 
@@ -368,6 +349,7 @@ private predicate hasStoreSummary(
   SummarizedCallable callable, DataFlow::ContentSet contents, SummaryComponentStack input,
   SummaryComponentStack output
 ) {
+  not TypeTrackingStep::suppressSummary(callable) and
   callable.propagatesFlow(input, push(SummaryComponent::content(contents), output), true) and
   not isNonLocal(input.head()) and
   not isNonLocal(output.head())
@@ -378,6 +360,7 @@ private predicate hasLoadSummary(
   SummarizedCallable callable, DataFlow::ContentSet contents, SummaryComponentStack input,
   SummaryComponentStack output
 ) {
+  not TypeTrackingStep::suppressSummary(callable) and
   callable.propagatesFlow(push(SummaryComponent::content(contents), input), output, true) and
   not isNonLocal(input.head()) and
   not isNonLocal(output.head())
@@ -388,12 +371,12 @@ private predicate hasLoadStoreSummary(
   SummarizedCallable callable, DataFlow::ContentSet loadContents,
   DataFlow::ContentSet storeContents, SummaryComponentStack input, SummaryComponentStack output
 ) {
+  not TypeTrackingStep::suppressSummary(callable) and
   callable
       .propagatesFlow(push(SummaryComponent::content(loadContents), input),
         push(SummaryComponent::content(storeContents), output), true) and
   not isNonLocal(input.head()) and
-  not isNonLocal(output.head()) and
-  callable != "Hash.[]" // Special-cased due to having a huge number of summaries
+  not isNonLocal(output.head())
 }
 
 /**
@@ -426,6 +409,7 @@ private predicate hasWithoutContentSummary(
   SummaryComponentStack output
 ) {
   exists(DataFlow::ContentSet content |
+    not TypeTrackingStep::suppressSummary(callable) and
     callable.propagatesFlow(push(SummaryComponent::withoutContent(content), input), output, true) and
     filter = getFilterFromWithoutContentStep(content) and
     not isNonLocal(input.head()) and
@@ -464,6 +448,7 @@ private predicate hasWithContentSummary(
   SummaryComponentStack output
 ) {
   exists(DataFlow::ContentSet content |
+    not TypeTrackingStep::suppressSummary(callable) and
     callable.propagatesFlow(push(SummaryComponent::withContent(content), input), output, true) and
     filter = getFilterFromWithContentStep(content) and
     not isNonLocal(input.head()) and
@@ -508,6 +493,7 @@ private predicate dependsOnSummaryComponentStack(
   SummarizedCallable callable, SummaryComponentStack stack
 ) {
   exists(callable.getACallSimple()) and
+  not TypeTrackingStep::suppressSummary(callable) and
   (
     callable.propagatesFlow(stack, _, true)
     or
@@ -592,6 +578,13 @@ class TypeTrackingStep extends TUnit {
   /** Gets the string `"unit"`. */
   string toString() { result = "unit" }
 
+  /**
+   * Holds if type-tracking should not attempt to derive steps from (simple) calls to `callable`.
+   *
+   * This can be done to manually control how steps are generated from such calls.
+   */
+  predicate suppressSummary(SummarizedCallable callable) { none() }
+
   /**
    * Holds if type-tracking should step from `pred` to `succ`.
    */
@@ -630,6 +623,15 @@ class TypeTrackingStep extends TUnit {
 
 /** Provides access to the steps contributed by subclasses of `SharedTypeTrackingStep`. */
 module TypeTrackingStep {
+  /**
+   * Holds if type-tracking should not attempt to derive steps from (simple) calls to `callable`.
+   *
+   * This can be done to manually control how steps are generated from such calls.
+   */
+  predicate suppressSummary(SummarizedCallable callable) {
+    any(TypeTrackingStep st).suppressSummary(callable)
+  }
+
   /**
    * Holds if type-tracking should step from `pred` to `succ`.
    */