Ruby: Fix spurious flow through reverse stores

Author: hvitved

ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll

@@ -45,6 +45,17 @@ module SummaryComponent {
     result = SC::content(TSingletonContent(DataFlow::Content::getElementContent(cv)))
   }
 
+  /**
+   * Gets a summary component that represents an element in a collection at a specific
+   * known index `cv`, or an uknown index.
+   */
+  SummaryComponent elementKnownOrUnknown(ConstantValue cv) {
+    result = SC::content(TKnownOrUnknownElementContent(TKnownElementContent(cv)))
+    or
+    not exists(TKnownElementContent(cv)) and
+    result = elementUnknown()
+  }
+
   /**
    * Gets a summary component that represents an element in a collection at either an unknown
    * index or known index. This has the same semantics as
@@ -62,7 +73,15 @@ module SummaryComponent {
    * integer index `lower` or above.
    */
   SummaryComponent elementLowerBound(int lower) {
-    result = SC::content(TElementLowerBoundContent(lower))
+    result = SC::content(TElementLowerBoundContent(lower, false))
+  }
+
+  /**
+   * Gets a summary component that represents an element in a collection at known
+   * integer index `lower` or above, or possibly at an unknown index.
+   */
+  SummaryComponent elementLowerBoundOrUnknown(int lower) {
+    result = SC::content(TElementLowerBoundContent(lower, true))
   }
 
   /** Gets a summary component that represents a value in a pair at an unknown key. */

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

@@ -380,8 +380,10 @@ private module Cached {
   newtype TContentSet =
     TSingletonContent(Content c) or
     TAnyElementContent() or
-    TElementLowerBoundContent(int lower) {
-      FlowSummaryImplSpecific::ParsePositions::isParsedElementLowerBoundPosition(_, lower)
+    TKnownOrUnknownElementContent(Content::KnownElementContent c) or
+    TElementLowerBoundContent(int lower, boolean includeUnknown) {
+      FlowSummaryImplSpecific::ParsePositions::isParsedElementLowerBoundPosition(_, includeUnknown,
+        lower)
     }
 
   cached

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

@@ -329,11 +329,28 @@ class ContentSet extends TContentSet {
   /** Holds if this content set represents all `ElementContent`s. */
   predicate isAnyElement() { this = TAnyElementContent() }
 
+  /**
+   * Holds if this content set represents a specific known element index, or an
+   * unknown element index.
+   */
+  predicate isKnownOrUnknownElement(Content::KnownElementContent c) {
+    this = TKnownOrUnknownElementContent(c)
+  }
+
   /**
    * Holds if this content set represents all `KnownElementContent`s where
    * the index is an integer greater than or equal to `lower`.
    */
-  predicate isElementLowerBound(int lower) { this = TElementLowerBoundContent(lower) }
+  predicate isElementLowerBound(int lower) { this = TElementLowerBoundContent(lower, false) }
+
+  /**
+   * Holds if this content set represents `UnknownElementContent` unioned with
+   * all `KnownElementContent`s where the index is an integer greater than or
+   * equal to `lower`.
+   */
+  predicate isElementLowerBoundOrUnknown(int lower) {
+    this = TElementLowerBoundContent(lower, true)
+  }
 
   /** Gets a textual representation of this content set. */
   string toString() {
@@ -345,8 +362,18 @@ class ContentSet extends TContentSet {
     this.isAnyElement() and
     result = "any element"
     or
-    exists(int lower |
-      this.isElementLowerBound(lower) and
+    exists(Content::KnownElementContent c |
+      this.isKnownOrUnknownElement(c) and
+      result = c + " or unknown"
+    )
+    or
+    exists(int lower, boolean includeUnknown |
+      this = TElementLowerBoundContent(lower, includeUnknown)
+    |
+      includeUnknown = false and
+      result = lower + "..!"
+      or
+      includeUnknown = true and
       result = lower + ".."
     )
   }
@@ -355,9 +382,18 @@ class ContentSet extends TContentSet {
   Content getAStoreContent() {
     this.isSingleton(result)
     or
+    // For reverse stores, `a[unknown][0] = x`, it is important that the read-step
+    // from `a` to `a[unknown]` (which can read any element), gets translated into
+    // a reverse store step that store only into `?`
     this.isAnyElement() and
     result = TUnknownElementContent()
     or
+    // For reverse stores, `a[1][0] = x`, it is important that the read-step
+    // from `a` to `a[1]` (which can read both elements stored at exactly index `1`
+    // and elements stored at unknown index), gets translated into a reverse store
+    // step that store only into `1`
+    this.isKnownOrUnknownElement(result)
+    or
     this.isElementLowerBound(_) and
     result = TUnknownElementContent()
   }
@@ -369,10 +405,21 @@ class ContentSet extends TContentSet {
     this.isAnyElement() and
     result instanceof Content::ElementContent
     or
-    exists(int lower, int i |
-      this.isElementLowerBound(lower) and
-      result.(Content::KnownElementContent).getIndex().isInt(i) and
-      i >= lower
+    exists(Content::KnownElementContent c | this.isKnownOrUnknownElement(c) |
+      result = c or
+      result = TUnknownElementContent()
+    )
+    or
+    exists(int lower, boolean includeUnknown |
+      this = TElementLowerBoundContent(lower, includeUnknown)
+    |
+      exists(int i |
+        result.(Content::KnownElementContent).getIndex().isInt(i) and
+        i >= lower
+      )
+      or
+      includeUnknown = true and
+      result = TUnknownElementContent()
     )
   }
 }

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

@@ -74,16 +74,30 @@ private SummaryComponent interpretElementArg(string arg) {
   arg = "any" and
   result = FlowSummary::SummaryComponent::elementAny()
   or
-  exists(int lower |
-    ParsePositions::isParsedElementLowerBoundPosition(arg, lower) and
+  exists(int lower, boolean includeUnknown |
+    ParsePositions::isParsedElementLowerBoundPosition(arg, includeUnknown, lower)
+  |
+    includeUnknown = false and
     result = FlowSummary::SummaryComponent::elementLowerBound(lower)
+    or
+    includeUnknown = true and
+    result = FlowSummary::SummaryComponent::elementLowerBoundOrUnknown(lower)
   )
   or
-  exists(ConstantValue cv | result = FlowSummary::SummaryComponent::elementKnown(cv) |
-    cv.isInt(AccessPath::parseInt(arg))
+  exists(ConstantValue cv, string argAdjusted, boolean includeUnknown |
+    argAdjusted = ParsePositions::adjustElementArgument(arg, includeUnknown) and
+    (
+      includeUnknown = false and
+      result = FlowSummary::SummaryComponent::elementKnown(cv)
+      or
+      includeUnknown = true and
+      result = FlowSummary::SummaryComponent::elementKnownOrUnknown(cv)
+    )
+  |
+    cv.isInt(AccessPath::parseInt(argAdjusted))
     or
-    not exists(AccessPath::parseInt(arg)) and
-    cv.serialize() = arg
+    not exists(AccessPath::parseInt(argAdjusted)) and
+    cv.serialize() = argAdjusted
   )
 }
 
@@ -277,9 +291,19 @@ module ParsePositions {
     c = paramName + ":"
   }
 
-  predicate isParsedElementLowerBoundPosition(string c, int lower) {
+  bindingset[arg]
+  string adjustElementArgument(string arg, boolean includeUnknown) {
+    result = arg.regexpCapture("(.*)!", 1) and
+    includeUnknown = false
+    or
+    result = arg and
+    not arg.matches("%!") and
+    includeUnknown = true
+  }
+
+  predicate isParsedElementLowerBoundPosition(string c, boolean includeUnknown, int lower) {
     isElementBody(c) and
-    lower = AccessPath::parseLowerBound(c)
+    lower = AccessPath::parseLowerBound(adjustElementArgument(c, includeUnknown))
   }
 }