Merge pull request #18872 from paldepind/rust-ref-mut

Rust: Allow SSA and some data flow for mutable borrows

Author: paldepind

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

@@ -11,7 +11,13 @@ private module Input implements InputSig<Location, RustDataFlow> {
     not exists(n.asExpr().getLocation())
   }
 
-  predicate postWithInFlowExclude(RustDataFlow::Node n) { n instanceof Node::FlowSummaryNode }
+  predicate postWithInFlowExclude(RustDataFlow::Node n) {
+    n instanceof Node::FlowSummaryNode
+    or
+    // We allow flow into post-update node for receiver expressions (from the
+    // synthetic post receiever node).
+    n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = any(Node::ReceiverNode r).getReceiver()
+  }
 
   predicate missingLocationExclude(RustDataFlow::Node n) { not exists(n.asExpr().getLocation()) }
 }

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

@@ -4,6 +4,7 @@
 
 private import codeql.util.Void
 private import codeql.util.Unit
+private import codeql.util.Boolean
 private import codeql.dataflow.DataFlow
 private import codeql.dataflow.internal.DataFlowImpl
 private import rust
@@ -96,6 +97,8 @@ final class ParameterPosition extends TParameterPosition {
   /** Gets the underlying integer position, if any. */
   int getPosition() { this = TPositionalParameterPosition(result) }
 
+  predicate hasPosition() { exists(this.getPosition()) }
+
   /** Holds if this position represents the `self` position. */
   predicate isSelf() { this = TSelfParameterPosition() }
 
@@ -130,21 +133,21 @@ private predicate callToMethod(CallExpr call) {
   )
 }
 
-/** Holds if `arg` is an argument of `call` at the position `pos`. */
+/**
+ * Holds if `arg` is an argument of `call` at the position `pos`.
+ *
+ * Note that this does not hold for the receiever expression of a method call
+ * as the synthetic `ReceiverNode` is the argument for the `self` parameter.
+ */
 private predicate isArgumentForCall(ExprCfgNode arg, CallExprBaseCfgNode call, ParameterPosition pos) {
   if callToMethod(call.(CallExprCfgNode).getCallExpr())
-  then (
+  then
     // The first argument is for the `self` parameter
     arg = call.getArgument(0) and pos.isSelf()
     or
     // Succeeding arguments are shifted left
     arg = call.getArgument(pos.getPosition() + 1)
-  ) else (
-    // The self argument in a method call.
-    arg = call.(MethodCallExprCfgNode).getReceiver() and pos.isSelf()
-    or
-    arg = call.getArgument(pos.getPosition())
-  )
+  else arg = call.getArgument(pos.getPosition())
 }
 
 /**
@@ -374,6 +377,30 @@ module Node {
     }
   }
 
+  /**
+   * The receiver of a method call _after_ any implicit borrow or dereferencing
+   * has taken place.
+   */
+  final class ReceiverNode extends ArgumentNode, TReceiverNode {
+    private MethodCallExprCfgNode n;
+
+    ReceiverNode() { this = TReceiverNode(n, false) }
+
+    ExprCfgNode getReceiver() { result = n.getReceiver() }
+
+    MethodCallExprCfgNode getMethodCall() { result = n }
+
+    override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
+      call.asMethodCallExprCfgNode() = n and pos = TSelfParameterPosition()
+    }
+
+    override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() }
+
+    override Location getLocation() { result = this.getReceiver().getLocation() }
+
+    override string toString() { result = "receiver for " + this.getReceiver() }
+  }
+
   final class SummaryArgumentNode extends FlowSummaryNode, ArgumentNode {
     private FlowSummaryImpl::Private::SummaryNode receiver;
     private RustDataFlow::ArgumentPosition pos_;
@@ -519,6 +546,18 @@ module Node {
     override Location getLocation() { result = n.getLocation() }
   }
 
+  final class ReceiverPostUpdateNode extends PostUpdateNode, TReceiverNode {
+    private MethodCallExprCfgNode n;
+
+    ReceiverPostUpdateNode() { this = TReceiverNode(n, true) }
+
+    override Node getPreUpdateNode() { result = TReceiverNode(n, false) }
+
+    override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() }
+
+    override Location getLocation() { result = n.getReceiver().getLocation() }
+  }
+
   final class SummaryPostUpdateNode extends FlowSummaryNode, PostUpdateNode {
     private FlowSummaryNode pre;
 
@@ -648,6 +687,14 @@ module LocalFlow {
     )
     or
     nodeFrom.asPat().(OrPatCfgNode).getAPat() = nodeTo.asPat()
+    or
+    // Simple value step from receiver expression to receiver node, in case
+    // there is no implicit deref or borrow operation.
+    nodeFrom.asExpr() = nodeTo.(Node::ReceiverNode).getReceiver()
+    or
+    // The dual step of the above, for the post-update nodes.
+    nodeFrom.(Node::PostUpdateNode).getPreUpdateNode().(Node::ReceiverNode).getReceiver() =
+      nodeTo.(Node::PostUpdateNode).getPreUpdateNode().asExpr()
   }
 }
 
@@ -998,6 +1045,23 @@ predicate lambdaCallExpr(CallExprCfgNode call, LambdaCallKind kind, ExprCfgNode
   exists(kind)
 }
 
+/** Holds if `mc` implicitly borrows its receiver. */
+private predicate implicitBorrow(MethodCallExpr mc) {
+  // Determining whether an implicit borrow happens depends on the type of the
+  // receiever as well as the target. As a heuristic we simply check if the
+  // target takes `self` as a borrow and limit the approximation to cases where
+  // the receiver is a simple variable.
+  mc.getReceiver() instanceof VariableAccess and
+  mc.getStaticTarget().getParamList().getSelfParam().isRef()
+}
+
+/** Holds if `mc` implicitly dereferences its receiver. */
+private predicate implicitDeref(MethodCallExpr mc) {
+  // Similarly to `implicitBorrow` this is an approximation.
+  mc.getReceiver() instanceof VariableAccess and
+  not mc.getStaticTarget().getParamList().getSelfParam().isRef()
+}
+
 // Defines a set of aliases needed for the `RustDataFlow` module
 private module Aliases {
   class DataFlowCallableAlias = DataFlowCallable;
@@ -1054,13 +1118,12 @@ module RustDataFlow implements InputSig<Location> {
   DataFlowType getNodeType(Node node) { any() }
 
   predicate nodeIsHidden(Node node) {
-    node instanceof Node::SsaNode
-    or
-    node.(Node::FlowSummaryNode).getSummaryNode().isHidden()
-    or
-    node instanceof Node::CaptureNode
-    or
-    node instanceof Node::ClosureParameterNode
+    node instanceof Node::SsaNode or
+    node.(Node::FlowSummaryNode).getSummaryNode().isHidden() or
+    node instanceof Node::CaptureNode or
+    node instanceof Node::ClosureParameterNode or
+    node instanceof Node::ReceiverNode or
+    node instanceof Node::ReceiverPostUpdateNode
   }
 
   predicate neverSkipInPathGraph(Node node) {
@@ -1169,6 +1232,28 @@ module RustDataFlow implements InputSig<Location> {
       node2.(Node::FlowSummaryNode).getSummaryNode())
   }
 
+  pragma[nomagic]
+  private predicate implicitDerefToReceiver(Node node1, Node::ReceiverNode node2, ReferenceContent c) {
+    node1.asExpr() = node2.getReceiver() and
+    implicitDeref(node2.getMethodCall().getMethodCallExpr()) and
+    exists(c)
+  }
+
+  pragma[nomagic]
+  private predicate implicitBorrowToReceiver(
+    Node node1, Node::ReceiverNode node2, ReferenceContent c
+  ) {
+    node1.asExpr() = node2.getReceiver() and
+    implicitBorrow(node2.getMethodCall().getMethodCallExpr()) and
+    exists(c)
+  }
+
+  pragma[nomagic]
+  private predicate referenceExprToExpr(Node node1, Node node2, ReferenceContent c) {
+    node1.asExpr() = node2.asExpr().(RefExprCfgNode).getExpr() and
+    exists(c)
+  }
+
   /**
    * Holds if data can flow from `node1` to `node2` via a read of `c`.  Thus,
    * `node1` references an object with a content `c.getAReadContent()` whose
@@ -1251,6 +1336,17 @@ module RustDataFlow implements InputSig<Location> {
         node2.asExpr() = await
       )
       or
+      referenceExprToExpr(node2.(PostUpdateNode).getPreUpdateNode(),
+        node1.(PostUpdateNode).getPreUpdateNode(), c)
+      or
+      // Step from receiver expression to receiver node, in case of an implicit
+      // dereference.
+      implicitDerefToReceiver(node1, node2, c)
+      or
+      // A read step dual to the store step for implicit borrows.
+      implicitBorrowToReceiver(node2.(PostUpdateNode).getPreUpdateNode(),
+        node1.(PostUpdateNode).getPreUpdateNode(), c)
+      or
       VariableCapture::readStep(node1, c, node2)
     )
     or
@@ -1327,11 +1423,7 @@ module RustDataFlow implements InputSig<Location> {
       node2.(PostUpdateNode).getPreUpdateNode().asExpr() = index.getBase()
     )
     or
-    exists(RefExprCfgNode ref |
-      c instanceof ReferenceContent and
-      node1.asExpr() = ref.getExpr() and
-      node2.asExpr() = ref
-    )
+    referenceExprToExpr(node1, node2, c)
     or
     // Store in function argument
     exists(DataFlowCall call, int i |
@@ -1341,6 +1433,10 @@ module RustDataFlow implements InputSig<Location> {
     )
     or
     VariableCapture::storeStep(node1, c, node2)
+    or
+    // Step from receiver expression to receiver node, in case of an implicit
+    // borrow.
+    implicitBorrowToReceiver(node1, node2, c)
   }
 
   /**
@@ -1612,17 +1708,25 @@ private module Cached {
     TPatNode(PatCfgNode p) or
     TNameNode(NameCfgNode n) { n.getName() = any(Variable v).getName() } or
     TExprPostUpdateNode(ExprCfgNode e) {
-      isArgumentForCall(e, _, _) or
-      lambdaCallExpr(_, _, e) or
-      lambdaCreationExpr(e.getExpr(), _) or
+      isArgumentForCall(e, _, _)
+      or
+      lambdaCallExpr(_, _, e)
+      or
+      lambdaCreationExpr(e.getExpr(), _)
+      or
+      // Whenever `&mut e` has a post-update node we also create one for `e`.
+      // E.g., for `e` in `f(..., &mut e, ...)` or `*(&mut e) = ...`.
+      e = any(RefExprCfgNode ref | ref.isMut() and exists(TExprPostUpdateNode(ref))).getExpr()
+      or
       e =
         [
           any(IndexExprCfgNode i).getBase(), any(FieldExprCfgNode access).getExpr(),
           any(TryExprCfgNode try).getExpr(),
           any(PrefixExprCfgNode pe | pe.getOperatorName() = "*").getExpr(),
-          any(AwaitExprCfgNode a).getExpr()
+          any(AwaitExprCfgNode a).getExpr(), any(MethodCallExprCfgNode mc).getReceiver()
         ]
     } or
+    TReceiverNode(MethodCallExprCfgNode mc, Boolean isPost) or
     TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or
     TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
     TClosureSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or

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

@@ -47,26 +47,7 @@ module SsaInput implements SsaImplCommon::InputSig<Location> {
 
   BasicBlock getABasicBlockSuccessor(BasicBlock bb) { result = bb.getASuccessor() }
 
-  /**
-   * A variable amenable to SSA construction.
-   *
-   * All immutable variables are amenable. Mutable variables are restricted to
-   * those that are not borrowed (either explicitly using `& mut`, or
-   * (potentially) implicit as borrowed receivers in a method call).
-   */
-  class SourceVariable extends Variable {
-    SourceVariable() {
-      this.isMutable()
-      implies
-      not exists(VariableAccess va | va = this.getAnAccess() |
-        va = any(RefExpr re | re.isMut()).getExpr()
-        or
-        // receivers can be borrowed implicitly, cf.
-        // https://doc.rust-lang.org/reference/expressions/method-call-expr.html
-        va = any(MethodCallExpr mce).getReceiver()
-      )
-    }
-  }
+  class SourceVariable = Variable;
 
   predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain) {
     (
@@ -76,7 +57,12 @@ module SsaInput implements SsaImplCommon::InputSig<Location> {
     ) and
     certain = true
     or
-    capturedCallWrite(_, bb, i, v) and certain = false
+    (
+      capturedCallWrite(_, bb, i, v)
+      or
+      mutablyBorrows(bb.getNode(i).getAstNode(), v)
+    ) and
+    certain = false
   }
 
   predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) {
@@ -229,6 +215,14 @@ predicate capturedCallWrite(Expr call, BasicBlock bb, int i, Variable v) {
   )
 }
 
+/** Holds if `v` may be mutably borrowed in `e`. */
+private predicate mutablyBorrows(Expr e, Variable v) {
+  e = any(MethodCallExpr mc).getReceiver() and
+  e.(VariableAccess).getVariable() = v
+  or
+  exists(RefExpr re | re = e and re.isMut() and re.getExpr().(VariableAccess).getVariable() = v)
+}
+
 /**
  * Holds if a pseudo read of captured variable `v` should be inserted
  * at index `i` in exit block `bb`.
@@ -379,6 +373,17 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
     none() // handled in `DataFlowImpl.qll` instead
   }
 
+  predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) {
+    exists(CfgNodes::CallExprBaseCfgNode call, Variable v, BasicBlock bb, int i |
+      def.definesAt(v, bb, i) and
+      mutablyBorrows(bb.getNode(i).getAstNode(), v)
+    |
+      call.getArgument(_) = bb.getNode(i)
+      or
+      call.(CfgNodes::MethodCallExprCfgNode).getReceiver() = bb.getNode(i)
+    )
+  }
+
   class Parameter = CfgNodes::ParamBaseCfgNode;
 
   /** Holds if SSA definition `def` initializes parameter `p` at function entry. */