Merge pull request #22792 from asgerf/unified/bypass-optional

Unified: Bypass Optional<T> contents in data flow

Author: asgerf

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowGraph.qll

@@ -176,14 +176,26 @@ module DebugGraph<relevantNodeSig/1 relevantNode> {
     value = node.toString()
   }
 
+  private predicate isBypassedStoreReadStep(Step s) {
+    exists(ContentSet contents |
+      s.read(contents)
+      or
+      s.store(contents)
+    |
+      bypassContentSet(contents)
+    )
+  }
+
   query predicate edges(Node node1, Node node2, string key, string value) {
     key = "semmle.label" and
     relevantDataFlowNode(node1) and
     relevantDataFlowNode(node2) and
     (
       exists(Step step |
         step(node1, step, node2) and
-        value = step.toString()
+        if isBypassedStoreReadStep(step)
+        then value = "value [bypassed " + step.toString() + "]"
+        else value = step.toString()
       )
       or
       exists(boolean isUseStep |

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowInstantiation.qll

@@ -127,20 +127,33 @@ module DataFlowInput implements InputSig<Location> {
   //
   // Steps
   //
+  pragma[nomagic]
+  private Step valueStepEx() {
+    result.value()
+    or
+    exists(ContentSet contents | bypassContentSet(contents) |
+      result.read(contents)
+      or
+      result.store(contents)
+    )
+  }
+
   predicate simpleLocalFlowStep(Node node1, Node node2, string model) {
-    step(node1, any(Step s | s.value()), node2) and model = ""
+    step(node1, valueStepEx(), node2) and model = ""
     or
     localSsaStep(node1, node2, _) and model = ""
   }
 
   predicate jumpStep(Node node1, Node node2) { step(node1, any(Step s | s.jump()), node2) }
 
   predicate readStep(Node node1, ContentSet c, Node node2) {
-    step(node1, any(Step s | s.read(c)), node2)
+    step(node1, any(Step s | s.read(c)), node2) and
+    not bypassContentSet(c)
   }
 
   predicate storeStep(Node node1, ContentSet c, Node node2) {
-    step(node1, any(Step s | s.store(c)), node2)
+    step(node1, any(Step s | s.store(c)), node2) and
+    not bypassContentSet(c)
   }
 
   predicate clearsContent(Node n, ContentSet c) { none() } // TODO

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPlugin.qll

@@ -13,4 +13,16 @@ private module Plugins {
 class DataFlowPlugin extends Unit {
   /** Holds if there is a language-specific step from `node1 -> step -> node2`. */
   predicate step(Node node1, Step step, Node node2) { none() }
+
+  /**
+   * Holds if a read/store step with the given `contents` should be converted to a value step.
+   *
+   * This affects the steps generated by the language-agnostic code as well as steps from the plugin itself.
+   */
+  predicate bypassContentSet(ContentSet contents) { none() }
 }
+
+/**
+ * Holds if a read/store step with the given `contents` should be converted to a value step.
+ */
+predicate bypassContentSet(ContentSet contents) { any(DataFlowPlugin p).bypassContentSet(contents) }

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll

@@ -6,6 +6,10 @@ private import unified
 private import AllDataFlow
 
 private class SwiftDataFlowPlugin extends DataFlowPlugin {
+  override predicate bypassContentSet(ContentSet contents) {
+    contents.asSingleton().asNamedMember() = "some.0"
+  }
+
   // Note: For now we assume all code is Swift, but in the future we must restrict these rules to Swift-files
   override predicate step(Node node1, Step step, Node node2) {
     exists(BinaryExpr expr |
@@ -27,33 +31,31 @@ private class SwiftDataFlowPlugin extends DataFlowPlugin {
     )
     or
     exists(UnaryExpr expr |
-      expr.getOperator().(PostfixOperator).getValue() = "!" and
-      node1.isResultValue(expr.getOperand()) and
-      (step.readName("some.0") or step.taint()) and
-      node2.isResultValue(expr)
+      expr.getOperator().(PostfixOperator).getValue() = "!"
       or
-      expr.getOperator().(PrefixOperator).getValue() = ["try", "try!", "await"] and
+      expr.getOperator().(PrefixOperator).getValue() = ["try", "try!", "try?", "await"]
+    |
       node1.isResultValue(expr.getOperand()) and
       step.value() and
       node2.isResultValue(expr)
-      or
-      expr.getOperator().(PrefixOperator).getValue() = "try?" and
-      // TODO: preserve the value of some.0 if it is already stored in that
-      node1.isResultValue(expr.getOperand()) and
-      step.storeName("some.0") and
-      node2.isResultValue(expr)
     )
     or
     exists(TypeCastExpr expr |
-      // The `as?` type cast boxes the incoming value in Optional depending on whether the type cast succeeded
-      expr.getOperator().getValue() = "as?" and
+      // Type-cast operators:
+      // - "as": Safe upcast conversion
+      // - "as?": Wrap in Optional<T> if cast succeeds
+      // - "as!": Throw if cast fails
+      //
+      // Since we bypass Optional<T> content, we model all of these as value steps.
+      expr.getOperator().getValue() = ["as", "as?", "as!"] and
       node1.isResultValue(expr.getExpr()) and
-      step.storeName("some.0") and
+      step.value() and
       node2.isResultValue(expr)
-      or
-      // Safe upcast conversion ("as") and downcast-or-throw ("as!") propagate the value directly
-      expr.getOperator().getValue() = ["as", "as!"] and
-      node1.isResultValue(expr.getExpr()) and
+    )
+    or
+    exists(NullCoalescingExpr expr |
+      // We bypass Optional<T> content so we add value-flow out of both operands of "??"
+      node1.isResultValue(expr.getAnOperand()) and
       step.value() and
       node2.isResultValue(expr)
     )