Refactor using OptionalStep

Author: aibaars

rust/ql/integration-tests/hello-project/summary.expected

@@ -15,7 +15,7 @@
 | Macro calls - resolved | 2 |
 | Macro calls - total | 2 |
 | Macro calls - unresolved | 0 |
-| Taint edges - number of edges | 1675 |
+| Taint edges - number of edges | 1674 |
 | Taint reach - nodes tainted | 0 |
 | Taint reach - per million nodes | 0 |
 | Taint sinks - cryptographic operations | 0 |

rust/ql/integration-tests/hello-workspace/summary.cargo.expected

@@ -15,7 +15,7 @@
 | Macro calls - resolved | 2 |
 | Macro calls - total | 2 |
 | Macro calls - unresolved | 0 |
-| Taint edges - number of edges | 1675 |
+| Taint edges - number of edges | 1674 |
 | Taint reach - nodes tainted | 0 |
 | Taint reach - per million nodes | 0 |
 | Taint sinks - cryptographic operations | 0 |

rust/ql/integration-tests/hello-workspace/summary.rust-project.expected

@@ -15,7 +15,7 @@
 | Macro calls - resolved | 2 |
 | Macro calls - total | 2 |
 | Macro calls - unresolved | 0 |
-| Taint edges - number of edges | 1675 |
+| Taint edges - number of edges | 1674 |
 | Taint reach - nodes tainted | 0 |
 | Taint reach - per million nodes | 0 |
 | Taint sinks - cryptographic operations | 0 |

rust/ql/lib/codeql/rust/dataflow/internal/Content.qll

@@ -214,6 +214,41 @@ final class SingletonContentSet extends ContentSet, TSingletonContentSet {
   override Content getAReadContent() { result = c }
 }
 
+/**
+ * A step in a flow summary defined using `OptionalStep[name]`. An `OptionalStep` is "opt-in", which means
+ * that by default the step is not present in the flow summary and needs to be explicitly enabled by defining
+ * an additional flow step.
+ */
+final class OptionalStep extends ContentSet, TOptionalStep {
+  override string toString() {
+    exists(string name |
+      this = TOptionalStep(name) and
+      result = "OptionalStep[" + name + "]"
+    )
+  }
+
+  override Content getAStoreContent() { none() }
+
+  override Content getAReadContent() { none() }
+}
+
+/**
+ * A step in a flow summary defined using `OptionalBarrier[name]`. An `OptionalBarrier` is "opt-out", by default
+ * data can flow freely through the step. Flow through the step can be explicity blocked by defining its node as a barrier.
+ */
+final class OptionalBarrier extends ContentSet, TOptionalBarrier {
+  override string toString() {
+    exists(string name |
+      this = TOptionalBarrier(name) and
+      result = "OptionalBarrier[" + name + "]"
+    )
+  }
+
+  override Content getAStoreContent() { none() }
+
+  override Content getAReadContent() { none() }
+}
+
 private import codeql.rust.internal.CachedStages
 
 cached

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

@@ -581,6 +581,12 @@ module RustDataFlow implements InputSig<Location> {
     model = ""
     or
     LocalFlow::flowSummaryLocalStep(nodeFrom, nodeTo, model)
+    or
+    // Add flow through optional barriers. This step is then blocked by the barrier for queries that choose to use the barrier.
+    FlowSummaryImpl::Private::Steps::summaryReadStep(nodeFrom
+          .(Node::FlowSummaryNode)
+          .getSummaryNode(), TOptionalBarrier(_), nodeTo.(Node::FlowSummaryNode).getSummaryNode()) and
+    model = ""
   }
 
   /**
@@ -710,7 +716,8 @@ module RustDataFlow implements InputSig<Location> {
     )
     or
     FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), cs,
-      node2.(FlowSummaryNode).getSummaryNode())
+      node2.(FlowSummaryNode).getSummaryNode()) and
+    not isSpecialContentSet(cs)
   }
 
   pragma[nomagic]
@@ -807,7 +814,8 @@ module RustDataFlow implements InputSig<Location> {
     storeContentStep(node1, cs.(SingletonContentSet).getContent(), node2)
     or
     FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), cs,
-      node2.(FlowSummaryNode).getSummaryNode())
+      node2.(FlowSummaryNode).getSummaryNode()) and
+    not isSpecialContentSet(cs)
   }
 
   /**
@@ -1093,7 +1101,24 @@ private module Cached {
   newtype TReturnKind = TNormalReturnKind()
 
   cached
-  newtype TContentSet = TSingletonContentSet(Content c)
+  newtype TContentSet =
+    TSingletonContentSet(Content c) or
+    TOptionalStep(string name) {
+      name = any(FlowSummaryImpl::Private::AccessPathToken tok).getAnArgument("OptionalStep")
+    } or
+    TOptionalBarrier(string name) {
+      name = any(FlowSummaryImpl::Private::AccessPathToken tok).getAnArgument("OptionalBarrier")
+    }
+
+  /**
+   * Holds if `cs` is used to encode a special operation as a content component, but should not
+   * be treated as an ordinary content component.
+   */
+  cached
+  predicate isSpecialContentSet(ContentSet cs) {
+    cs instanceof TOptionalStep or
+    cs instanceof TOptionalBarrier
+  }
 
   /** Holds if `n` is a flow source of kind `kind`. */
   cached
@@ -1105,3 +1130,31 @@ private module Cached {
 }
 
 import Cached
+
+cached
+private module OptionalSteps {
+  /**
+   * A step in a flow summary defined using `OptionalStep[name]`. An `OptionalStep` is "opt-in", which means
+   * that by default the step is not present in the flow summary and needs to be explicitly enabled by defining
+   * an additional flow step.
+   */
+  cached
+  predicate optionalStep(Node node1, string name, Node node2) {
+    FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(),
+      TOptionalStep(name), node2.(FlowSummaryNode).getSummaryNode()) or
+    FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(),
+      TOptionalStep(name), node2.(FlowSummaryNode).getSummaryNode())
+  }
+
+  /**
+   * A step in a flow summary defined using `OptionalBarrier[name]`. An `OptionalBarrier` is "opt-out", by default
+   * data can flow freely through the step. Flow through the step can be explicity blocked by defining its node as a barrier.
+   */
+  cached
+  predicate optionalBarrier(Node node, string name) {
+    FlowSummaryImpl::Private::Steps::summaryReadStep(_, TOptionalBarrier(name),
+      node.(FlowSummaryNode).getSummaryNode())
+  }
+}
+
+import OptionalSteps

rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll

@@ -54,7 +54,7 @@ module Input implements InputSig<Location, RustDataFlow> {
 
   RustDataFlow::ArgumentPosition callbackSelfParameterPosition() { result.isClosureSelf() }
 
-  ReturnKind getStandardReturnValueKind() { result = TNormalReturnKind() }
+  ReturnKind getStandardReturnValueKind() { result = TNormalReturnKind() and Stage::ref() }
 
   string encodeParameterPosition(ParameterPosition pos) { result = pos.toString() }
 
@@ -105,6 +105,10 @@ module Input implements InputSig<Location, RustDataFlow> {
       c = TFutureContent() and
       arg = ""
     )
+    or
+    cs = TOptionalStep(arg) and result = "OptionalStep"
+    or
+    cs = TOptionalBarrier(arg) and result = "OptionalBarrier"
   }
 
   string encodeReturn(ReturnKind rk, string arg) { none() }
@@ -193,3 +197,12 @@ module ParsePositions {
     i = AccessPath::parseInt(c)
   }
 }
+
+cached
+module Stage {
+  cached
+  predicate ref() { 1 = 1 }
+
+  cached
+  predicate backref() { optionalStep(_, _, _) }
+}

rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll

@@ -69,7 +69,9 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
     exists(Content c | c = cs.(SingletonContentSet).getContent() |
       c instanceof ElementContent or
       c instanceof ReferenceContent
-    )
+    ) and
+    // Optional steps are added through isAdditionalFlowStep but we don't want the implicit reads
+    not optionalStep(node, _, _)
   }
 
   /**

rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll

@@ -21,18 +21,3 @@ private class StartswithCall extends Path::SafeAccessCheck::Range, CfgNodes::Met
     branch = true
   }
 }
-
-/**
- * A call to `Path.canonicalize`.
- * See https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize
- */
-private class PathCanonicalizeCall extends Path::PathNormalization::Range {
-  CfgNodes::MethodCallExprCfgNode call;
-
-  PathCanonicalizeCall() {
-    call = this.asExpr() and
-    call.getAstNode().(Resolvable).getResolvedPath() = "<crate::path::Path>::canonicalize"
-  }
-
-  override DataFlow::Node getPathArg() { result.asExpr() = call.getReceiver() }
-}

rust/ql/lib/codeql/rust/frameworks/stdlib/fs.model.yml

@@ -16,4 +16,5 @@ extensions:
       - ["lang:std", "<crate::path::PathBuf as crate::convert::From>::from", "Argument[0]", "ReturnValue", "taint", "manual"]
       - ["lang:std", "<crate::path::Path>::join", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["lang:std", "<crate::path::Path>::join", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["lang:std", "<crate::path::Path>::canonicalize", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)].OptionalStep[normalize-path]", "taint", "manual"]
       - ["lang:std", "<crate::path::Path>::canonicalize", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]

rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml

@@ -32,6 +32,3 @@ extensions:
       - ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["lang:alloc", "<crate::string::String>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["lang:alloc", "<_ as crate::string::ToString>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
-      # Result
-      - ["lang:core", "<crate::result::Result>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"]
-