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

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

@@ -365,6 +365,7 @@ module RustDataFlow implements InputSig<Location> {
     node instanceof CaptureNode or
     node instanceof ClosureParameterNode or
     node instanceof DerefBorrowNode or
+    node instanceof DerefOutNode or
     node.asExpr() instanceof ParenExpr or
     nodeIsHidden(node.(PostUpdateNode).getPreUpdateNode())
   }
@@ -586,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
@@ -721,10 +721,6 @@ module RustDataFlow implements InputSig<Location> {
     VariableCapture::storeStep(node1, c, node2)
     or
     implicitBorrow(node1, node2, c)
-    or
-    // A store step dual to the read step for implicit dereferences.
-    implicitDeref(node2.(PostUpdateNode).getPreUpdateNode(),
-      node1.(PostUpdateNode).getPreUpdateNode(), c)
   }
 
   /**

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

@@ -348,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_;
@@ -434,6 +465,18 @@ final class DerefBorrowPostUpdateNode extends PostUpdateNode, TDerefBorrowNode {
   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 {
   private FlowSummaryNode pre;
 
@@ -499,6 +542,7 @@ newtype TNode =
     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/frameworks/stdlib/Builtins.qll

@@ -27,8 +27,19 @@ private class BuiltinsTypesFile extends File {
 class BuiltinType extends Struct {
   BuiltinType() { this.getFile() instanceof BuiltinsTypesFile }
 
-  /** Gets the name of this type. */
+  /**
+   * Gets the name of this type.
+   *
+   * This is the name used internally to represent the type, for example `Ref`.
+   */
   string getName() { result = super.getName().getText() }
+
+  /**
+   * Gets a display name for this type.
+   *
+   * This is the name used in code, for example `&`.
+   */
+  string getDisplayName() { result = this.getName() }
 }
 
 /**
@@ -140,21 +151,43 @@ class F64 extends FloatingPointTypeImpl {
 /** The builtin slice type `[T]`. */
 class SliceType extends BuiltinType {
   SliceType() { this.getName() = "Slice" }
+
+  override string getDisplayName() { result = "[]" }
 }
 
 /** The builtin array type `[T; N]`. */
 class ArrayType extends BuiltinType {
   ArrayType() { this.getName() = "Array" }
+
+  override string getDisplayName() { result = "[;]" }
 }
 
-/** The builtin reference type `&T` or `&mut T`. */
+/** The builtin reference type `&T`. */
 class RefType extends BuiltinType {
   RefType() { this.getName() = "Ref" }
+
+  override string getDisplayName() { result = "&" }
+}
+
+/** The builtin reference type `&mut T`. */
+class RefMutType extends BuiltinType {
+  RefMutType() { this.getName() = "RefMut" }
+
+  override string getDisplayName() { result = "&mut" }
 }
 
-/** The builtin pointer type `*const T` or `*mut T`. */
+/** The builtin pointer type `*const T`. */
 class PtrType extends BuiltinType {
   PtrType() { this.getName() = "Ptr" }
+
+  override string getDisplayName() { result = "*const" }
+}
+
+/** The builtin pointer type `*mut T`. */
+class PtrMutType extends BuiltinType {
+  PtrMutType() { this.getName() = "PtrMut" }
+
+  override string getDisplayName() { result = "*mut" }
 }
 
 /** A builtin tuple type `(T1, T2, ...)`. */
@@ -168,4 +201,13 @@ class TupleType extends BuiltinType {
     or
     result = this.getGenericParamList().getNumberOfGenericParams()
   }
+
+  override string getDisplayName() {
+    // Note: This produces "(,,)" for a 2-tuple, "(,,,)" for a 3-tuple, etc.
+    // This is in order to distinguish the unit type `()` from the 1-tuple `(,)`.
+    exists(string commas |
+      commas = concat(int i | i = [0 .. this.getArity() - 1] | ",") and
+      result = "(" + commas + ")"
+    )
+  }
 }

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

@@ -24,9 +24,10 @@ extensions:
       extensible: summaryModel
     data:
       # Box
-      - ["<alloc::boxed::Box>::pin", "Argument[0]", "ReturnValue.Reference", "value", "manual"]
-      - ["<alloc::boxed::Box>::new", "Argument[0]", "ReturnValue.Reference", "value", "manual"]
-      - ["<alloc::boxed::Box>::into_pin", "Argument[0]", "ReturnValue", "value", "manual"]
+      - ["<alloc::boxed::Box as core::ops::deref::Deref>::deref", "Argument[self].Reference.Field[alloc::boxed::Box(0)]", "ReturnValue.Reference", "value", "manual"]
+      - ["<alloc::boxed::Box>::pin", "Argument[0]", "ReturnValue.Field[core::pin::Pin::pointer].Field[alloc::boxed::Box(0)]", "value", "manual"]
+      - ["<alloc::boxed::Box>::new", "Argument[0]", "ReturnValue.Field[alloc::boxed::Box(0)]", "value", "manual"]
+      - ["<alloc::boxed::Box>::into_pin", "Argument[0]", "ReturnValue.Field[core::pin::Pin::pointer]", "value", "manual"]
       # Fmt
       - ["alloc::fmt::format", "Argument[0]", "ReturnValue", "taint", "manual"]
       # Layout

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

@@ -3,6 +3,9 @@ extensions:
       pack: codeql/rust-all
       extensible: summaryModel
     data:
+      # Builtin deref
+      - ["<& as core::ops::deref::Deref>::deref", "Argument[self].Reference", "ReturnValue", "value", "manual"]
+      - ["<&mut as core::ops::deref::Deref>::deref", "Argument[self].Reference", "ReturnValue", "value", "manual"]
       # Arithmetic
       - ["<_ as core::ops::arith::Add>::add", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["<_ as core::ops::arith::Add>::add", "Argument[0]", "ReturnValue", "taint", "manual"]
@@ -41,8 +44,9 @@ extensions:
       - ["core::ptr::write", "Argument[1]", "Argument[0].Reference", "value", "manual"]
       - ["core::ptr::write_unaligned", "Argument[1]", "Argument[0].Reference", "value", "manual"]
       - ["core::ptr::write_volatile", "Argument[1]", "Argument[0].Reference", "value", "manual"]
-        # https://doc.rust-lang.org/std/pin/struct.Pin.html#impl-Deref-for-Pin%3CPtr%3E, but limited to `Ptr = &`
+        # https://doc.rust-lang.org/std/pin/struct.Pin.html#impl-Deref-for-Pin%3CPtr%3E, but limited to `Ptr = &` and `Ptr = Box`
       - ["<core::pin::Pin as core::ops::deref::Deref>::deref", "Argument[self].Reference.Field[core::pin::Pin::pointer].Reference", "ReturnValue.Reference", "value", "manual"]
+      - ["<core::pin::Pin as core::ops::deref::Deref>::deref", "Argument[self].Reference.Field[core::pin::Pin::pointer].Field[alloc::boxed::Box(0)]", "ReturnValue.Reference", "value", "manual"]
       # Str
       - ["<core::str>::as_str", "Argument[self]", "ReturnValue", "value", "manual"]
       - ["<core::str>::as_bytes", "Argument[self]", "ReturnValue", "value", "manual"]

rust/ql/lib/codeql/rust/internal/PathResolution.qll

@@ -958,7 +958,16 @@ private class StructItemNode extends TypeItemNode, ParameterizableItemNode insta
   language[monotonicAggregates]
   override string getCanonicalPath(Crate c) {
     this.hasCanonicalPath(c) and
-    result = strictconcat(int i | i in [0 .. 2] | this.getCanonicalPathPart(c, i) order by i)
+    (
+      this =
+        any(Builtins::BuiltinType t |
+          not t.hasVisibility() and
+          result = t.getDisplayName()
+        )
+      or
+      not this = any(Builtins::BuiltinType t | not t.hasVisibility()) and
+      result = strictconcat(int i | i in [0 .. 2] | this.getCanonicalPathPart(c, i) order by i)
+    )
   }
 }
 

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

@@ -1,5 +1,6 @@
 models
-| 1 | Summary: futures_executor::local_pool::block_on; Argument[0]; ReturnValue; value |
+| 1 | Summary: <& as core::ops::deref::Deref>::deref; Argument[self].Reference; ReturnValue; value |
+| 2 | Summary: futures_executor::local_pool::block_on; Argument[0]; ReturnValue; value |
 edges
 | main.rs:12:28:14:1 | { ... } | main.rs:17:13:17:23 | get_data(...) | provenance |  |
 | main.rs:13:5:13:13 | source(...) | main.rs:12:28:14:1 | { ... } | provenance |  |
@@ -118,7 +119,7 @@ edges
 | main.rs:236:9:236:22 | &... [&ref] | main.rs:235:38:237:5 | { ... } [&ref] | provenance |  |
 | main.rs:236:10:236:22 | ... .value | main.rs:236:9:236:22 | &... [&ref] | provenance |  |
 | main.rs:236:11:236:15 | * ... [MyInt] | main.rs:236:10:236:22 | ... .value | provenance |  |
-| main.rs:236:12:236:15 | self [&ref, MyInt] | main.rs:236:11:236:15 | * ... [MyInt] | provenance |  |
+| main.rs:236:12:236:15 | self [&ref, MyInt] | main.rs:236:11:236:15 | * ... [MyInt] | provenance | MaD:1 |
 | main.rs:242:9:242:9 | a [MyInt] | main.rs:244:13:244:13 | a [MyInt] | provenance |  |
 | main.rs:242:13:242:38 | MyInt {...} [MyInt] | main.rs:242:9:242:9 | a [MyInt] | provenance |  |
 | main.rs:242:28:242:36 | source(...) | main.rs:242:13:242:38 | MyInt {...} [MyInt] | provenance |  |
@@ -148,17 +149,17 @@ edges
 | main.rs:270:28:270:37 | source(...) | main.rs:270:13:270:39 | MyInt {...} [MyInt] | provenance |  |
 | main.rs:272:9:272:9 | c | main.rs:273:10:273:10 | c | provenance |  |
 | main.rs:272:13:272:29 | * ... | main.rs:272:9:272:9 | c | provenance |  |
-| main.rs:272:14:272:29 | ...::deref(...) [&ref] | main.rs:272:13:272:29 | * ... | provenance |  |
+| main.rs:272:14:272:29 | ...::deref(...) [&ref] | main.rs:272:13:272:29 | * ... | provenance | MaD:1 |
 | main.rs:272:27:272:28 | &a [&ref, MyInt] | main.rs:235:14:235:18 | SelfParam [&ref, MyInt] | provenance |  |
-| main.rs:272:27:272:28 | &a [&ref, MyInt] | main.rs:272:14:272:29 | ...::deref(...) [&ref] | provenance |  |
+| main.rs:272:27:272:28 | &a [&ref, MyInt] | main.rs:272:14:272:29 | ...::deref(...) [&ref] | provenance | MaD:1 |
 | main.rs:272:28:272:28 | a [MyInt] | main.rs:272:27:272:28 | &a [&ref, MyInt] | provenance |  |
 | main.rs:275:9:275:9 | a [MyInt] | main.rs:276:14:276:14 | a [MyInt] | provenance |  |
 | main.rs:275:13:275:39 | MyInt {...} [MyInt] | main.rs:275:9:275:9 | a [MyInt] | provenance |  |
 | main.rs:275:28:275:37 | source(...) | main.rs:275:13:275:39 | MyInt {...} [MyInt] | provenance |  |
-| main.rs:276:9:276:9 | c [&ref] | main.rs:277:10:277:10 | c | provenance |  |
-| main.rs:276:13:276:14 | * ... [&ref] | main.rs:276:9:276:9 | c [&ref] | provenance |  |
+| main.rs:276:9:276:9 | c | main.rs:277:10:277:10 | c | provenance |  |
+| main.rs:276:13:276:14 | * ... | main.rs:276:9:276:9 | c | provenance |  |
 | main.rs:276:14:276:14 | a [MyInt] | main.rs:235:14:235:18 | SelfParam [&ref, MyInt] | provenance |  |
-| main.rs:276:14:276:14 | a [MyInt] | main.rs:276:13:276:14 | * ... [&ref] | provenance |  |
+| main.rs:276:14:276:14 | a [MyInt] | main.rs:276:13:276:14 | * ... | provenance | MaD:1 |
 | main.rs:289:18:289:21 | SelfParam [MyInt] | main.rs:289:48:291:5 | { ... } [MyInt] | provenance |  |
 | main.rs:293:26:293:37 | ...: MyInt [MyInt] | main.rs:293:49:295:5 | { ... } [MyInt] | provenance |  |
 | main.rs:299:9:299:9 | a [MyInt] | main.rs:301:50:301:50 | a [MyInt] | provenance |  |
@@ -185,7 +186,7 @@ edges
 | main.rs:326:17:326:25 | source(...) | main.rs:326:13:326:13 | c | provenance |  |
 | main.rs:334:9:334:9 | a | main.rs:335:10:335:10 | a | provenance |  |
 | main.rs:334:13:334:55 | ...::block_on(...) | main.rs:334:9:334:9 | a | provenance |  |
-| main.rs:334:41:334:54 | async_source(...) | main.rs:334:13:334:55 | ...::block_on(...) | provenance | MaD:1 |
+| main.rs:334:41:334:54 | async_source(...) | main.rs:334:13:334:55 | ...::block_on(...) | provenance | MaD:2 |
 nodes
 | main.rs:12:28:14:1 | { ... } | semmle.label | { ... } |
 | main.rs:13:5:13:13 | source(...) | semmle.label | source(...) |
@@ -355,8 +356,8 @@ nodes
 | main.rs:275:9:275:9 | a [MyInt] | semmle.label | a [MyInt] |
 | main.rs:275:13:275:39 | MyInt {...} [MyInt] | semmle.label | MyInt {...} [MyInt] |
 | main.rs:275:28:275:37 | source(...) | semmle.label | source(...) |
-| main.rs:276:9:276:9 | c [&ref] | semmle.label | c [&ref] |
-| main.rs:276:13:276:14 | * ... [&ref] | semmle.label | * ... [&ref] |
+| main.rs:276:9:276:9 | c | semmle.label | c |
+| main.rs:276:13:276:14 | * ... | semmle.label | * ... |
 | main.rs:276:14:276:14 | a [MyInt] | semmle.label | a [MyInt] |
 | main.rs:277:10:277:10 | c | semmle.label | c |
 | main.rs:289:18:289:21 | SelfParam [MyInt] | semmle.label | SelfParam [MyInt] |
@@ -407,7 +408,7 @@ subpaths
 | main.rs:254:13:254:13 | a [MyInt] | main.rs:220:12:220:15 | SelfParam [MyInt] | main.rs:220:42:223:5 | { ... } [MyInt] | main.rs:254:13:254:20 | a.add(...) [MyInt] |
 | main.rs:261:35:261:35 | b [MyInt] | main.rs:227:30:227:39 | ...: MyInt [MyInt] | main.rs:227:19:227:27 | SelfParam [Return] [&ref, MyInt] | main.rs:261:27:261:32 | [post] &mut a [&ref, MyInt] |
 | main.rs:272:27:272:28 | &a [&ref, MyInt] | main.rs:235:14:235:18 | SelfParam [&ref, MyInt] | main.rs:235:38:237:5 | { ... } [&ref] | main.rs:272:14:272:29 | ...::deref(...) [&ref] |
-| main.rs:276:14:276:14 | a [MyInt] | main.rs:235:14:235:18 | SelfParam [&ref, MyInt] | main.rs:235:38:237:5 | { ... } [&ref] | main.rs:276:13:276:14 | * ... [&ref] |
+| main.rs:276:14:276:14 | a [MyInt] | main.rs:235:14:235:18 | SelfParam [&ref, MyInt] | main.rs:235:38:237:5 | { ... } [&ref] | main.rs:276:13:276:14 | * ... |
 | main.rs:301:50:301:50 | a [MyInt] | main.rs:289:18:289:21 | SelfParam [MyInt] | main.rs:289:48:291:5 | { ... } [MyInt] | main.rs:301:30:301:54 | ...::take_self(...) [MyInt] |
 | main.rs:306:55:306:55 | b [MyInt] | main.rs:293:26:293:37 | ...: MyInt [MyInt] | main.rs:293:49:295:5 | { ... } [MyInt] | main.rs:306:30:306:56 | ...::take_second(...) [MyInt] |
 testFailures

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

@@ -274,7 +274,7 @@ fn test_operator_overloading() {
 
     let a = MyInt { value: source(28) };
     let c = *a;
-    sink(c); // $ hasTaintFlow=28 MISSING: hasValueFlow=28
+    sink(c); // $ hasValueFlow=28
 }
 
 trait MyTrait2 {

rust/ql/test/library-tests/dataflow/global/viableCallable.expected

@@ -59,6 +59,8 @@
 | main.rs:212:13:212:34 | ...::new(...) | main.rs:205:5:208:5 | fn new |
 | main.rs:212:24:212:33 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:214:5:214:11 | sink(...) | main.rs:5:1:7:1 | fn sink |
+| main.rs:236:11:236:15 | * ... | {EXTERNAL LOCATION} | fn deref |
+| main.rs:236:11:236:15 | * ... | {EXTERNAL LOCATION} | fn deref |
 | main.rs:242:28:242:36 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:244:13:244:17 | ... + ... | main.rs:220:5:223:5 | fn add |
 | main.rs:245:5:245:17 | sink(...) | main.rs:5:1:7:1 | fn sink |
@@ -75,6 +77,8 @@
 | main.rs:266:5:266:10 | ... *= ... | main.rs:227:5:229:5 | fn mul_assign |
 | main.rs:267:5:267:17 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:270:28:270:37 | source(...) | main.rs:1:1:3:1 | fn source |
+| main.rs:272:13:272:29 | * ... | {EXTERNAL LOCATION} | fn deref |
+| main.rs:272:13:272:29 | * ... | {EXTERNAL LOCATION} | fn deref |
 | main.rs:272:14:272:29 | ...::deref(...) | main.rs:235:5:237:5 | fn deref |
 | main.rs:273:5:273:11 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:275:28:275:37 | source(...) | main.rs:1:1:3:1 | fn source |