Merge pull request #18657 from hvitved/rust/dataflow-node-api

Rust: Hide internal implementation details from `DataFlow::Node`

Author: hvitved

rust/ql/lib/codeql/rust/dataflow/DataFlow.qll

@@ -13,7 +13,7 @@ private import DataFlowImpl::Node as Node
  * (inter-procedural) data flow analyses.
  */
 module DataFlow {
-  final class Node = Node::Node;
+  final class Node = Node::NodePublic;
 
   /**
    * The value of a parameter at function entry, viewed as a node in a data
@@ -24,7 +24,7 @@ module DataFlow {
     ParamBase getParameter() { result = super.getParameter().getParamBase() }
   }
 
-  final class PostUpdateNode = Node::PostUpdateNode;
+  final class PostUpdateNode = Node::PostUpdateNodePublic;
 
   final class Content = DataFlowImpl::Content;
 

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

@@ -128,12 +128,15 @@ private predicate isArgumentForCall(ExprCfgNode arg, CallExprBaseCfgNode call, P
   arg = call.(MethodCallExprCfgNode).getReceiver() and pos.isSelf()
 }
 
+/**
+ * Provides the `Node` class and subclasses thereof.
+ *
+ * Classes with names ending in `Public` are exposed as `final` aliases in the
+ * public `DataFlow` API, so they should not expose internal implementation details.
+ */
 module Node {
-  /**
-   * An element, viewed as a node in a data flow graph. Either an expression
-   * (`ExprNode`) or a parameter (`ParameterNode`).
-   */
-  abstract class Node extends TNode {
+  /** An element, viewed as a node in a data flow graph. */
+  abstract class NodePublic extends TNode {
     /** Gets the location of this node. */
     abstract Location getLocation();
 
@@ -149,7 +152,9 @@ module Node {
      * Gets the pattern that corresponds to this node, if any.
      */
     PatCfgNode asPat() { none() }
+  }
 
+  abstract class Node extends NodePublic {
     /** Gets the enclosing callable. */
     DataFlowCallable getEnclosingCallable() { result = TCfgScope(this.getCfgScope()) }
 
@@ -160,11 +165,6 @@ module Node {
      * Gets the control flow node that corresponds to this data flow node.
      */
     CfgNode getCfgNode() { none() }
-
-    /**
-     * Gets this node's underlying SSA definition, if any.
-     */
-    Ssa::Definition asDefinition() { none() }
   }
 
   /** A node type that is not implemented. */
@@ -462,10 +462,12 @@ module Node {
    * Nodes corresponding to AST elements, for example `ExprNode`, usually refer
    * to the value before the update.
    */
-  abstract class PostUpdateNode extends Node {
+  abstract class PostUpdateNodePublic extends NodePublic {
     /** Gets the node before the state update. */
-    abstract Node getPreUpdateNode();
+    abstract NodePublic getPreUpdateNode();
+  }
 
+  abstract class PostUpdateNode extends PostUpdateNodePublic, Node {
     override string toString() { result = "[post] " + this.getPreUpdateNode().toString() }
   }
 
@@ -857,12 +859,13 @@ private module Aliases {
 
 module RustDataFlow implements InputSig<Location> {
   private import Aliases
+  private import codeql.rust.dataflow.DataFlow
 
   /**
    * An element, viewed as a node in a data flow graph. Either an expression
    * (`ExprNode`) or a parameter (`ParameterNode`).
    */
-  final class Node = Node::Node;
+  class Node = DataFlow::Node;
 
   final class ParameterNode = Node::ParameterNode;
 
@@ -872,7 +875,7 @@ module RustDataFlow implements InputSig<Location> {
 
   final class OutNode = Node::OutNode;
 
-  final class PostUpdateNode = Node::PostUpdateNode;
+  class PostUpdateNode = DataFlow::PostUpdateNode;
 
   final class CastNode = Node::NaNode;
 
@@ -886,7 +889,9 @@ module RustDataFlow implements InputSig<Location> {
     n.isArgumentOf(call, pos)
   }
 
-  DataFlowCallable nodeGetEnclosingCallable(Node node) { result = node.getEnclosingCallable() }
+  DataFlowCallable nodeGetEnclosingCallable(Node node) {
+    result = node.(Node::Node).getEnclosingCallable()
+  }
 
   DataFlowType getNodeType(Node node) { any() }
 
@@ -901,9 +906,9 @@ module RustDataFlow implements InputSig<Location> {
   }
 
   predicate neverSkipInPathGraph(Node node) {
-    node.getCfgNode() = any(LetStmtCfgNode s).getPat()
+    node.(Node::Node).getCfgNode() = any(LetStmtCfgNode s).getPat()
     or
-    node.getCfgNode() = any(AssignmentExprCfgNode a).getLhs()
+    node.(Node::Node).getCfgNode() = any(AssignmentExprCfgNode a).getLhs()
     or
     exists(MatchExprCfgNode match |
       node.asExpr() = match.getScrutinee() or

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

@@ -127,12 +127,12 @@ private module StepsInput implements Impl::Private::StepsInputSig {
     result.asCallBaseExprCfgNode().getCallExprBase() = sc.(LibraryCallable).getACall()
   }
 
-  Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
+  RustDataFlow::Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponent sc) {
     sc = Impl::Private::SummaryComponent::return(_) and
     result.asExpr().getExpr() = source.getCall()
   }
 
-  Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) {
+  RustDataFlow::Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) {
     exists(CallExprBase call, Expr arg, ParameterPosition pos |
       result.asExpr().getExpr() = arg and
       sc = Impl::Private::SummaryComponent::argument(pos) and

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

@@ -1,20 +1,21 @@
 private import rust
 private import codeql.dataflow.TaintTracking
 private import codeql.rust.controlflow.CfgNodes
+private import codeql.rust.dataflow.DataFlow
 private import codeql.rust.dataflow.FlowSummary
 private import DataFlowImpl
 private import FlowSummaryImpl as FlowSummaryImpl
 private import codeql.rust.internal.CachedStages
 
 module RustTaintTracking implements InputSig<Location, RustDataFlow> {
-  predicate defaultTaintSanitizer(Node::Node node) { none() }
+  predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
 
   /**
    * Holds if the additional step from `pred` to `succ` should be included in all
    * global taint flow configurations.
    */
   cached
-  predicate defaultAdditionalTaintStep(Node::Node pred, Node::Node succ, string model) {
+  predicate defaultAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ, string model) {
     Stages::DataFlowStage::ref() and
     model = "" and
     (
@@ -61,7 +62,7 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
    * and inputs to additional taint steps.
    */
   bindingset[node]
-  predicate defaultImplicitTaintRead(Node::Node node, ContentSet cs) {
+  predicate defaultImplicitTaintRead(DataFlow::Node node, ContentSet cs) {
     exists(node) and
     exists(Content c | c = cs.(SingletonContentSet).getContent() |
       c instanceof ElementContent or
@@ -73,5 +74,5 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
    * Holds if the additional step from `src` to `sink` should be considered in
    * speculative taint flow exploration.
    */
-  predicate speculativeTaintStep(Node::Node src, Node::Node sink) { none() }
+  predicate speculativeTaintStep(DataFlow::Node src, DataFlow::Node sink) { none() }
 }

rust/ql/src/queries/security/CWE-312/CleartextLogging.ql

@@ -36,7 +36,7 @@ module CleartextLoggingConfig implements DataFlow::ConfigSig {
     isSource(node)
   }
 
-  predicate isAdditionalFlowStep(Node node1, Node node2) {
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     // flow from `a` to `&a`
     node2.asExpr().getExpr().(RefExpr).getExpr() = node1.asExpr().getExpr()
   }