Merge pull request #20891 from hvitved/rust/data-flow-implicit-deref-borrow

Rust: Improve handling of implicit derefs/borrows in data flow

Author: hvitved

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

@@ -19,10 +19,6 @@ private module Input implements InputSig<Location, RustDataFlow> {
   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()
-    or
     n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = getPostUpdateReverseStep(_, _)
     or
     FlowSummaryImpl::Private::Steps::sourceLocalStep(_, n, _)

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

@@ -12,7 +12,6 @@ private import codeql.rust.elements.Call
 private import SsaImpl as SsaImpl
 private import codeql.rust.controlflow.internal.Scope as Scope
 private import codeql.rust.internal.PathResolution
-private import codeql.rust.internal.TypeInference as TypeInference
 private import codeql.rust.controlflow.ControlFlowGraph
 private import codeql.rust.dataflow.Ssa
 private import codeql.rust.dataflow.FlowSummary
@@ -141,18 +140,11 @@ final class ArgumentPosition extends ParameterPosition {
 
 /**
  * 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.
  */
-predicate isArgumentForCall(Expr arg, Call call, ParameterPosition pos) {
+predicate isArgumentForCall(Expr arg, Call call, ArgumentPosition pos) {
   // TODO: Handle index expressions as calls in data flow.
   not call instanceof IndexExpr and
-  (
-    call.getPositionalArgument(pos.getPosition()) = arg
-    or
-    call.getReceiver() = arg and pos.isSelf() and not call.receiverImplicitlyBorrowed()
-  )
+  arg = pos.getArgument(call)
 }
 
 /** Provides logic related to SSA. */
@@ -283,14 +275,6 @@ module LocalFlow {
     or
     nodeFrom.asPat().(OrPat).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.(ReceiverNode).getReceiver()
-    or
-    // The dual step of the above, for the post-update nodes.
-    nodeFrom.(PostUpdateNode).getPreUpdateNode().(ReceiverNode).getReceiver() =
-      nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr()
-    or
     nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr() =
       getPostUpdateReverseStep(nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr(), true)
   }
@@ -380,7 +364,8 @@ module RustDataFlow implements InputSig<Location> {
     node.(FlowSummaryNode).getSummaryNode().isHidden() or
     node instanceof CaptureNode or
     node instanceof ClosureParameterNode or
-    node instanceof ReceiverNode or
+    node instanceof DerefBorrowNode or
+    node instanceof DerefOutNode or
     node.asExpr() instanceof ParenExpr or
     nodeIsHidden(node.(PostUpdateNode).getPreUpdateNode())
   }
@@ -520,16 +505,16 @@ module RustDataFlow implements InputSig<Location> {
   }
 
   pragma[nomagic]
-  private predicate implicitDerefToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) {
-    TypeInference::receiverHasImplicitDeref(node1.asExpr()) and
-    node1.asExpr() = node2.getReceiver() and
+  private predicate implicitDeref(Node node1, DerefBorrowNode node2, ReferenceContent c) {
+    not node2.isBorrow() and
+    node1.asExpr() = node2.getNode() and
     exists(c)
   }
 
   pragma[nomagic]
-  private predicate implicitBorrowToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) {
-    TypeInference::receiverHasImplicitBorrow(node1.asExpr()) and
-    node1.asExpr() = node2.getReceiver() and
+  private predicate implicitBorrow(Node node1, DerefBorrowNode node2, ReferenceContent c) {
+    node2.isBorrow() and
+    node1.asExpr() = node2.getNode() and
     exists(c)
   }
 
@@ -539,6 +524,15 @@ module RustDataFlow implements InputSig<Location> {
     exists(c)
   }
 
+  private Node getFieldExprContainerNode(FieldExpr fe) {
+    exists(Expr container | container = fe.getContainer() |
+      not any(DerefBorrowNode n).getNode() = container and
+      result.asExpr() = container
+      or
+      result.(DerefBorrowNode).getNode() = container
+    )
+  }
+
   pragma[nomagic]
   additional predicate readContentStep(Node node1, Content c, Node node2) {
     exists(TupleStructPat pat, int pos |
@@ -563,7 +557,7 @@ module RustDataFlow implements InputSig<Location> {
     node1.asPat().(RefPat).getPat() = node2.asPat()
     or
     exists(FieldExpr access |
-      node1.asExpr() = access.getContainer() and
+      node1 = getFieldExprContainerNode(access) and
       node2.asExpr() = access and
       access = c.(FieldContent).getAnAccess()
     )
@@ -593,10 +587,9 @@ module RustDataFlow implements InputSig<Location> {
           .isVariantField([any(OptionEnum o).getSome(), any(ResultEnum r).getOk()], 0)
     )
     or
-    exists(PrefixExpr deref |
+    exists(DerefExpr deref |
       c instanceof ReferenceContent and
-      deref.getOperatorName() = "*" and
-      node1.asExpr() = deref.getExpr() and
+      node1.(DerefOutNode).getDerefExpr() = deref and
       node2.asExpr() = deref
     )
     or
@@ -616,12 +609,10 @@ module RustDataFlow implements InputSig<Location> {
     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)
+    implicitDeref(node1, node2, c)
     or
     // A read step dual to the store step for implicit borrows.
-    implicitBorrowToReceiver(node2.(PostUpdateNode).getPreUpdateNode(),
+    implicitBorrow(node2.(PostUpdateNode).getPreUpdateNode(),
       node1.(PostUpdateNode).getPreUpdateNode(), c)
     or
     VariableCapture::readStep(node1, c, node2)
@@ -657,7 +648,7 @@ module RustDataFlow implements InputSig<Location> {
     exists(AssignmentExpr assignment, FieldExpr access |
       assignment.getLhs() = access and
       node1.asExpr() = assignment.getRhs() and
-      node2.asExpr() = access.getContainer() and
+      node2 = getFieldExprContainerNode(access) and
       access = c.getAnAccess()
     )
   }
@@ -729,9 +720,7 @@ 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)
+    implicitBorrow(node1, node2, c)
   }
 
   /**

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

@@ -14,6 +14,7 @@ private import codeql.rust.controlflow.ControlFlowGraph
 private import codeql.rust.controlflow.CfgNodes
 private import codeql.rust.dataflow.Ssa
 private import codeql.rust.dataflow.FlowSummary
+private import codeql.rust.internal.TypeInference as TypeInference
 private import Node as Node
 private import DataFlowImpl
 private import FlowSummaryImpl as FlowSummaryImpl
@@ -226,35 +227,53 @@ final class ExprArgumentNode extends ArgumentNode, ExprNode {
   private Call call_;
   private RustDataFlow::ArgumentPosition pos_;
 
-  ExprArgumentNode() { isArgumentForCall(n, call_, pos_) }
+  ExprArgumentNode() {
+    isArgumentForCall(n, call_, pos_) and
+    not TypeInference::implicitDeref(n) and
+    not TypeInference::implicitBorrow(n)
+  }
 
   override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
     call.asCall() = call_ and pos = pos_
   }
 }
 
 /**
- * The receiver of a method call _after_ any implicit borrow or dereferencing
- * has taken place.
+ * A node that represents the value of an expression _after_ implicit dereferencing
+ * or borrowing.
  */
-final class ReceiverNode extends ArgumentNode, TReceiverNode {
-  private Call n;
+class DerefBorrowNode extends Node, TDerefBorrowNode {
+  AstNode n;
+  boolean isBorrow;
 
-  ReceiverNode() { this = TReceiverNode(n, false) }
+  DerefBorrowNode() { this = TDerefBorrowNode(n, isBorrow, false) }
 
-  Expr getReceiver() { result = n.getReceiver() }
+  AstNode getNode() { result = n }
 
-  MethodCallExpr getMethodCall() { result = n }
+  predicate isBorrow() { isBorrow = true }
 
-  override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
-    call.asCall() = n and pos = TSelfParameterPosition()
+  override CfgScope getCfgScope() { result = n.getEnclosingCfgScope() }
+
+  override Location getLocation() { result = n.getLocation() }
+
+  override string toString() {
+    if isBorrow = true then result = n + " [borrowed]" else result = n + " [dereferenced]"
   }
+}
 
-  override CfgScope getCfgScope() { result = n.getEnclosingCfgScope() }
+/**
+ * A node that represents the value of an argument of a call _after_ implicit
+ * dereferencing or borrowing.
+ */
+final class DerefBorrowArgNode extends DerefBorrowNode, ArgumentNode {
+  private DataFlowCall call_;
+  private RustDataFlow::ArgumentPosition pos_;
 
-  override Location getLocation() { result = this.getReceiver().getLocation() }
+  DerefBorrowArgNode() { isArgumentForCall(n, call_.asCall(), pos_) }
 
-  override string toString() { result = "receiver for " + this.getReceiver() }
+  override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
+    call = call_ and pos = pos_
+  }
 }
 
 final class SummaryArgumentNode extends FlowSummaryNode, ArgumentNode {
@@ -329,15 +348,46 @@ abstract class OutNode extends Node {
 }
 
 final private class ExprOutNode extends ExprNode, OutNode {
-  ExprOutNode() { this.asExpr() instanceof Call }
+  ExprOutNode() {
+    exists(Call call |
+      call = this.asExpr() and
+      not call instanceof DerefExpr // Handled by `DerefOutNode`
+    )
+  }
 
-  /** Gets the underlying call CFG node that includes this out node. */
+  /** Gets the underlying call node that includes this out node. */
   override DataFlowCall getCall(ReturnKind kind) {
     result.asCall() = n and
     kind = TNormalReturnKind()
   }
 }
 
+/**
+ * A node that represents the value of a `*` expression _before_ implicit
+ * dereferencing:
+ *
+ * `*v` equivalent to `*Deref::deref(&v)`, and this node represents the
+ * `Deref::deref(&v)` part.
+ */
+class DerefOutNode extends OutNode, TDerefOutNode {
+  DerefExpr de;
+
+  DerefOutNode() { this = TDerefOutNode(de, false) }
+
+  DerefExpr getDerefExpr() { result = de }
+
+  override CfgScope getCfgScope() { result = de.getEnclosingCfgScope() }
+
+  override DataFlowCall getCall(ReturnKind kind) {
+    result.asCall() = de and
+    kind = TNormalReturnKind()
+  }
+
+  override Location getLocation() { result = de.getLocation() }
+
+  override string toString() { result = de.toString() + " [pre-dereferenced]" }
+}
+
 final class SummaryOutNode extends FlowSummaryNode, OutNode {
   private DataFlowCall call;
   private ReturnKind kind_;
@@ -402,16 +452,29 @@ final class ExprPostUpdateNode extends PostUpdateNode, TExprPostUpdateNode {
   override Location getLocation() { result = e.getLocation() }
 }
 
-final class ReceiverPostUpdateNode extends PostUpdateNode, TReceiverNode {
-  private Call call;
+final class DerefBorrowPostUpdateNode extends PostUpdateNode, TDerefBorrowNode {
+  private Expr arg;
+  private boolean isBorrow;
 
-  ReceiverPostUpdateNode() { this = TReceiverNode(call, true) }
+  DerefBorrowPostUpdateNode() { this = TDerefBorrowNode(arg, isBorrow, true) }
 
-  override Node getPreUpdateNode() { result = TReceiverNode(call, false) }
+  override DerefBorrowNode getPreUpdateNode() { result = TDerefBorrowNode(arg, isBorrow, false) }
 
-  override CfgScope getCfgScope() { result = call.getEnclosingCfgScope() }
+  override CfgScope getCfgScope() { result = arg.getEnclosingCfgScope() }
 
-  override Location getLocation() { result = call.getReceiver().getLocation() }
+  override Location getLocation() { result = arg.getLocation() }
+}
+
+class DerefOutPostUpdateNode extends PostUpdateNode, TDerefOutNode {
+  DerefExpr de;
+
+  DerefOutPostUpdateNode() { this = TDerefOutNode(de, true) }
+
+  override DerefOutNode getPreUpdateNode() { result = TDerefOutNode(de, false) }
+
+  override CfgScope getCfgScope() { result = de.getEnclosingCfgScope() }
+
+  override Location getLocation() { result = de.getLocation() }
 }
 
 final class SummaryPostUpdateNode extends FlowSummaryNode, PostUpdateNode {
@@ -467,19 +530,19 @@ newtype TNode =
           any(IndexExpr i).getBase(), //
           any(FieldExpr access).getContainer(), //
           any(TryExpr try).getExpr(), //
-          any(PrefixExpr pe | pe.getOperatorName() = "*").getExpr(), //
           any(AwaitExpr a).getExpr(), //
-          any(MethodCallExpr mc).getReceiver(), //
           getPostUpdateReverseStep(any(PostUpdateNode n).getPreUpdateNode().asExpr(), _)
         ]
     )
   } or
-  TReceiverNode(Call call, Boolean isPost) {
-    call.hasEnclosingCfgScope() and
-    call.receiverImplicitlyBorrowed() and
-    // TODO: Handle index expressions as calls in data flow.
-    not call instanceof IndexExpr
+  TDerefBorrowNode(AstNode n, boolean borrow, Boolean isPost) {
+    TypeInference::implicitDeref(n) and
+    borrow = false
+    or
+    TypeInference::implicitBorrow(n) and
+    borrow = true
   } or
+  TDerefOutNode(DerefExpr de, Boolean isPost) or
   TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or
   TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) {
     forall(AstNode n | n = sn.getSinkElement() or n = sn.getSourceElement() |

rust/ql/lib/codeql/rust/elements/internal/FieldExprImpl.qll

@@ -23,10 +23,10 @@ module Impl {
    */
   class FieldExpr extends Generated::FieldExpr {
     /** Gets the record field that this access references, if any. */
-    StructField getStructField() { result = TypeInference::resolveStructFieldExpr(this) }
+    StructField getStructField() { result = TypeInference::resolveStructFieldExpr(this, _) }
 
     /** Gets the tuple field that this access references, if any. */
-    TupleField getTupleField() { result = TypeInference::resolveTupleFieldExpr(this) }
+    TupleField getTupleField() { result = TypeInference::resolveTupleFieldExpr(this, _) }
 
     override string toStringImpl() {
       exists(string abbr, string name |

rust/ql/lib/codeql/rust/frameworks/futures.model.yml

@@ -7,13 +7,10 @@ extensions:
       - ["<futures_util::io::buf_reader::BufReader>::new", "Argument[0]", "ReturnValue", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncReadExt>::read", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncReadExt>::read", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncReadExt>::read_to_end", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncReadExt>::read_to_end", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncBufReadExt>::read_line", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncBufReadExt>::read_line", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncBufReadExt>::read_until", "Argument[self]", "Argument[1].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncBufReadExt>::read_until", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncBufReadExt>::fill_buf", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
+      - ["<_ as futures_util::io::AsyncBufReadExt>::fill_buf", "Argument[self].Reference", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncBufReadExt>::lines", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["<_ as futures_io::if_std::AsyncBufRead>::poll_fill_buf", "Argument[self].Reference", "ReturnValue.Field[core::task::poll::Poll::Ready(0)].Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<_ as futures_io::if_std::AsyncRead>::poll_read", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"]

rust/ql/lib/codeql/rust/frameworks/reqwest.model.yml

@@ -22,10 +22,10 @@ extensions:
       - ["<reqwest::response::Response>::text", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::response::Response>::text_with_charset", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::response::Response>::bytes", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
-      - ["<reqwest::response::Response>::chunk", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]
+      - ["<reqwest::response::Response>::chunk", "Argument[self].Reference", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]
       - ["<reqwest::blocking::response::Response>::text", "Argument[self]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::blocking::response::Response>::text_with_charset", "Argument[self]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::blocking::response::Response>::bytes", "Argument[self]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::async_impl::response::Response>::text", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::async_impl::response::Response>::bytes", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
-      - ["<reqwest::async_impl::response::Response>::chunk", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]
+      - ["<reqwest::async_impl::response::Response>::chunk", "Argument[self].Reference", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]