Rust: Address PR comments

Author: paldepind

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

@@ -133,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())
 }
 
 /**
@@ -370,19 +370,15 @@ module Node {
     private CallExprBaseCfgNode call_;
     private RustDataFlow::ArgumentPosition pos_;
 
-    ExprArgumentNode() {
-      isArgumentForCall(n, call_, pos_) and
-      // For receivers in method calls the `ReceiverNode` is the argument.
-      not call_.(MethodCallExprCfgNode).getReceiver() = n
-    }
+    ExprArgumentNode() { isArgumentForCall(n, call_, pos_) }
 
     override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
       call.asCallBaseExprCfgNode() = call_ and pos = pos_
     }
   }
 
   /**
-   * The receiver of a method call _after_ any implicit borrow or dereferences
+   * The receiver of a method call _after_ any implicit borrow or dereferencing
    * has taken place.
    */
   final class ReceiverNode extends ArgumentNode, TReceiverNode {
@@ -400,7 +396,7 @@ module Node {
 
     override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() }
 
-    override Location getLocation() { result = n.getLocation() }
+    override Location getLocation() { result = this.getReceiver().getLocation() }
 
     override string toString() { result = "receiver for " + this.getReceiver() }
   }
@@ -559,7 +555,7 @@ module Node {
 
     override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() }
 
-    override Location getLocation() { result = n.getLocation() }
+    override Location getLocation() { result = n.getReceiver().getLocation() }
   }
 
   final class SummaryPostUpdateNode extends FlowSummaryNode, PostUpdateNode {
@@ -1050,7 +1046,7 @@ predicate lambdaCallExpr(CallExprCfgNode call, LambdaCallKind kind, ExprCfgNode
 }
 
 /** Holds if `mc` implicitly borrows its receiver. */
-predicate implicitBorrow(MethodCallExpr mc) {
+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
@@ -1060,7 +1056,7 @@ predicate implicitBorrow(MethodCallExpr mc) {
 }
 
 /** Holds if `mc` implicitly dereferences its receiver. */
-predicate implicitDeref(MethodCallExpr mc) {
+private predicate implicitDeref(MethodCallExpr mc) {
   // Similarly to `implicitBorrow` this is an approximation.
   mc.getReceiver() instanceof VariableAccess and
   not mc.getStaticTarget().getParamList().getSelfParam().isRef()
@@ -1727,7 +1723,7 @@ private module Cached {
           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

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

@@ -47,13 +47,6 @@ 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 = Variable;
 
   predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain) {
@@ -381,8 +374,13 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
   }
 
   predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) {
-    exists(Variable v, BasicBlock bb, int i |
-      def.definesAt(v, bb, i) and mutablyBorrows(bb.getNode(i).getAstNode(), v)
+    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)
     )
   }
 

rust/ql/test/library-tests/dataflow/pointers/inline-flow.expected

@@ -40,11 +40,6 @@ edges
 | main.rs:73:10:73:10 | [post] b [&ref] | main.rs:74:15:74:15 | b [&ref] | provenance |  |
 | main.rs:73:14:73:23 | source(...) | main.rs:73:10:73:10 | [post] b [&ref] | provenance |  |
 | main.rs:74:15:74:15 | b [&ref] | main.rs:74:14:74:15 | * ... | provenance |  |
-| main.rs:90:11:90:16 | [post] &mut a [&ref] | main.rs:90:16:90:16 | [post] a | provenance |  |
-| main.rs:90:16:90:16 | [post] a | main.rs:91:14:91:14 | a | provenance |  |
-| main.rs:90:21:90:30 | source(...) | main.rs:90:11:90:16 | [post] &mut a [&ref] | provenance |  |
-| main.rs:95:13:95:29 | mut to_be_cleared | main.rs:98:14:98:26 | to_be_cleared | provenance |  |
-| main.rs:95:33:95:42 | source(...) | main.rs:95:13:95:29 | mut to_be_cleared | provenance |  |
 | main.rs:105:10:105:10 | [post] c [&ref] | main.rs:106:15:106:15 | c [&ref] | provenance |  |
 | main.rs:105:14:105:23 | source(...) | main.rs:105:10:105:10 | [post] c [&ref] | provenance |  |
 | main.rs:106:15:106:15 | c [&ref] | main.rs:106:14:106:15 | * ... | provenance |  |
@@ -178,13 +173,6 @@ nodes
 | main.rs:73:14:73:23 | source(...) | semmle.label | source(...) |
 | main.rs:74:14:74:15 | * ... | semmle.label | * ... |
 | main.rs:74:15:74:15 | b [&ref] | semmle.label | b [&ref] |
-| main.rs:90:11:90:16 | [post] &mut a [&ref] | semmle.label | [post] &mut a [&ref] |
-| main.rs:90:16:90:16 | [post] a | semmle.label | [post] a |
-| main.rs:90:21:90:30 | source(...) | semmle.label | source(...) |
-| main.rs:91:14:91:14 | a | semmle.label | a |
-| main.rs:95:13:95:29 | mut to_be_cleared | semmle.label | mut to_be_cleared |
-| main.rs:95:33:95:42 | source(...) | semmle.label | source(...) |
-| main.rs:98:14:98:26 | to_be_cleared | semmle.label | to_be_cleared |
 | main.rs:105:10:105:10 | [post] c [&ref] | semmle.label | [post] c [&ref] |
 | main.rs:105:14:105:23 | source(...) | semmle.label | source(...) |
 | main.rs:106:14:106:15 | * ... | semmle.label | * ... |
@@ -290,16 +278,13 @@ subpaths
 | main.rs:253:24:253:32 | my_number [MyNumber] | main.rs:149:14:149:24 | ...: MyNumber [MyNumber] | main.rs:149:34:153:1 | { ... } | main.rs:253:14:253:33 | to_number(...) |
 | main.rs:255:24:255:32 | my_number [MyNumber] | main.rs:149:14:149:24 | ...: MyNumber [MyNumber] | main.rs:149:34:153:1 | { ... } | main.rs:255:14:255:33 | to_number(...) |
 testFailures
-| main.rs:255:14:255:33 | to_number(...) | Unexpected result: hasValueFlow=99 |
 #select
 | main.rs:20:14:20:14 | c | main.rs:17:17:17:26 | source(...) | main.rs:20:14:20:14 | c | $@ | main.rs:17:17:17:26 | source(...) | source(...) |
 | main.rs:24:14:24:14 | n | main.rs:28:19:28:28 | source(...) | main.rs:24:14:24:14 | n | $@ | main.rs:28:19:28:28 | source(...) | source(...) |
 | main.rs:39:14:39:14 | b | main.rs:33:19:33:28 | source(...) | main.rs:39:14:39:14 | b | $@ | main.rs:33:19:33:28 | source(...) | source(...) |
 | main.rs:53:14:53:15 | * ... | main.rs:51:17:51:26 | source(...) | main.rs:53:14:53:15 | * ... | $@ | main.rs:51:17:51:26 | source(...) | source(...) |
 | main.rs:59:33:59:34 | * ... | main.rs:57:22:57:31 | source(...) | main.rs:59:33:59:34 | * ... | $@ | main.rs:57:22:57:31 | source(...) | source(...) |
 | main.rs:74:14:74:15 | * ... | main.rs:73:14:73:23 | source(...) | main.rs:74:14:74:15 | * ... | $@ | main.rs:73:14:73:23 | source(...) | source(...) |
-| main.rs:91:14:91:14 | a | main.rs:90:21:90:30 | source(...) | main.rs:91:14:91:14 | a | $@ | main.rs:90:21:90:30 | source(...) | source(...) |
-| main.rs:98:14:98:26 | to_be_cleared | main.rs:95:33:95:42 | source(...) | main.rs:98:14:98:26 | to_be_cleared | $@ | main.rs:95:33:95:42 | source(...) | source(...) |
 | main.rs:106:14:106:15 | * ... | main.rs:105:14:105:23 | source(...) | main.rs:106:14:106:15 | * ... | $@ | main.rs:105:14:105:23 | source(...) | source(...) |
 | main.rs:113:14:113:15 | * ... | main.rs:112:25:112:34 | source(...) | main.rs:113:14:113:15 | * ... | $@ | main.rs:112:25:112:34 | source(...) | source(...) |
 | main.rs:175:14:175:34 | my_number.to_number(...) | main.rs:174:44:174:53 | source(...) | main.rs:175:14:175:34 | my_number.to_number(...) | $@ | main.rs:174:44:174:53 | source(...) | source(...) |

rust/ql/test/library-tests/dataflow/pointers/main.rs

@@ -88,14 +88,14 @@ mod intraprocedural_mutable_borrows {
         let mut a = 1;
         sink(a);
         *(&mut a) = source(87);
-        sink(a); // $ hasValueFlow=87
+        sink(a); // $ MISSING: hasValueFlow=87
     }
 
     pub fn clear_through_borrow() {
         let mut to_be_cleared = source(34);
         let p = &mut to_be_cleared;
         *p = 0;
-        sink(to_be_cleared); // $ SPURIOUS: hasValueFlow=34
+        sink(to_be_cleared); // variable is cleared
     }
 
     pub fn write_through_borrow_in_match(cond: bool) {
@@ -252,7 +252,7 @@ mod interprocedural_mutable_borrows {
         (&mut my_number).set(source(99));
         sink(to_number(my_number)); // $ hasValueFlow=99
         (&mut my_number).set(0);
-        sink(to_number(my_number)); // SPURIOUS: hasValueFlow=99
+        sink(to_number(my_number)); // $ SPURIOUS: hasValueFlow=99
     }
 }