Merge pull request #18462 from hvitved/rust/variable-without-location

Rust: Avoid location-based variable analysis

Author: hvitved

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

@@ -165,7 +165,7 @@ module Impl {
 
   /**
    * A path expression that may access a local variable. These are paths that
-   * only consists of a simple name (i.e., without generic arguments,
+   * only consist of a simple name (i.e., without generic arguments,
    * qualifiers, etc.).
    */
   private class VariableAccessCand extends PathExprBase {
@@ -231,80 +231,169 @@ module Impl {
     )
   }
 
+  /** A subset of `Element`s for which we want to compute pre-order numbers. */
+  private class RelevantElement extends Element {
+    RelevantElement() {
+      this instanceof VariableScope or
+      this instanceof VariableAccessCand or
+      this instanceof LetStmt or
+      getImmediateChildAndAccessor(this, _, _) instanceof RelevantElement
+    }
+
+    pragma[nomagic]
+    private RelevantElement getChild(int index) {
+      result = getImmediateChildAndAccessor(this, index, _)
+    }
+
+    pragma[nomagic]
+    private RelevantElement getImmediateChildMin(int index) {
+      // A child may have multiple positions for different accessors,
+      // so always use the first
+      result = this.getChild(index) and
+      index = min(int i | result = this.getChild(i) | i)
+    }
+
+    pragma[nomagic]
+    RelevantElement getImmediateChild(int index) {
+      result =
+        rank[index + 1](Element res, int i | res = this.getImmediateChildMin(i) | res order by i)
+    }
+
+    pragma[nomagic]
+    RelevantElement getImmediateLastChild() {
+      exists(int last |
+        result = this.getImmediateChild(last) and
+        not exists(this.getImmediateChild(last + 1))
+      )
+    }
+  }
+
+  /**
+   * Gets the pre-order numbering of `n`, where the immediately enclosing
+   * variable scope of `n` is `scope`.
+   */
+  pragma[nomagic]
+  private int getPreOrderNumbering(VariableScope scope, RelevantElement n) {
+    n = scope and
+    result = 0
+    or
+    exists(RelevantElement parent |
+      not parent instanceof VariableScope
+      or
+      parent = scope
+    |
+      // first child of a previously numbered node
+      result = getPreOrderNumbering(scope, parent) + 1 and
+      n = parent.getImmediateChild(0)
+      or
+      // non-first child of a previously numbered node
+      exists(RelevantElement child, int i |
+        result = getLastPreOrderNumbering(scope, child) + 1 and
+        child = parent.getImmediateChild(i) and
+        n = parent.getImmediateChild(i + 1)
+      )
+    )
+  }
+
+  /**
+   * Gets the pre-order numbering of the _last_ node nested under `n`, where the
+   * immediately enclosing variable scope of `n` (and the last node) is `scope`.
+   */
+  pragma[nomagic]
+  private int getLastPreOrderNumbering(VariableScope scope, RelevantElement n) {
+    exists(RelevantElement leaf |
+      result = getPreOrderNumbering(scope, leaf) and
+      leaf != scope and
+      (
+        not exists(leaf.getImmediateChild(_))
+        or
+        leaf instanceof VariableScope
+      )
+    |
+      n = leaf
+      or
+      n.getImmediateLastChild() = leaf and
+      not n instanceof VariableScope
+    )
+    or
+    exists(RelevantElement mid |
+      mid = n.getImmediateLastChild() and
+      result = getLastPreOrderNumbering(scope, mid) and
+      not mid instanceof VariableScope and
+      not n instanceof VariableScope
+    )
+  }
+
   /**
    * Holds if `v` is named `name` and is declared inside variable scope
-   * `scope`, and `v` is bound starting from `(line, column)`.
+   * `scope`. The pre-order numbering of the binding site of `v`, amongst
+   * all nodes nester under `scope`, is `ord`.
    */
-  private predicate variableDeclInScope(
-    Variable v, VariableScope scope, string name, int line, int column
-  ) {
+  private predicate variableDeclInScope(Variable v, VariableScope scope, string name, int ord) {
     name = v.getName() and
     (
       parameterDeclInScope(v, scope) and
-      scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
+      ord = getPreOrderNumbering(scope, scope)
       or
       exists(Pat pat | pat = getAVariablePatAncestor(v) |
         scope =
           any(MatchArmScope arm |
             arm.getPat() = pat and
-            arm.getLocation().hasLocationFileInfo(_, line, column, _, _)
+            ord = getPreOrderNumbering(scope, arm)
           )
         or
         exists(LetStmt let |
           let.getPat() = pat and
           scope = getEnclosingScope(let) and
           // for `let` statements, variables are bound _after_ the statement, i.e.
           // not in the RHS
-          let.getLocation().hasLocationFileInfo(_, _, _, line, column)
+          ord = getLastPreOrderNumbering(scope, let) + 1
         )
         or
         exists(IfExpr ie, LetExpr let |
           let.getPat() = pat and
           ie.getCondition() = let and
           scope = ie.getThen() and
-          scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
+          ord = getPreOrderNumbering(scope, scope)
         )
         or
         exists(ForExpr fe |
           fe.getPat() = pat and
           scope = fe.getLoopBody() and
-          scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
+          ord = getPreOrderNumbering(scope, scope)
         )
         or
         exists(WhileExpr we, LetExpr let |
           let.getPat() = pat and
           we.getCondition() = let and
           scope = we.getLoopBody() and
-          scope.getLocation().hasLocationFileInfo(_, line, column, _, _)
+          ord = getPreOrderNumbering(scope, scope)
         )
       )
     )
   }
 
   /**
-   * Holds if `cand` may access a variable named `name` at
-   * `(startline, startcolumn, endline, endcolumn)` in the variable scope
-   * `scope`.
+   * Holds if `cand` may access a variable named `name` at pre-order number `ord`
+   * in the variable scope `scope`.
    *
    * `nestLevel` is the number of nested scopes that need to be traversed
    * to reach `scope` from `cand`.
    */
   private predicate variableAccessCandInScope(
-    VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int startline,
-    int startcolumn, int endline, int endcolumn
+    VariableAccessCand cand, VariableScope scope, string name, int nestLevel, int ord
   ) {
     name = cand.getName() and
     scope = [cand.(VariableScope), getEnclosingScope(cand)] and
-    cand.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn) and
+    ord = getPreOrderNumbering(scope, cand) and
     nestLevel = 0
     or
     exists(VariableScope inner |
-      variableAccessCandInScope(cand, inner, name, nestLevel - 1, _, _, _, _) and
+      variableAccessCandInScope(cand, inner, name, nestLevel - 1, _) and
       scope = getEnclosingScope(inner) and
-      // Use the location of the inner scope as the location of the access, instead of the
-      // actual access location. This allows us to collapse multiple accesses in inner
-      // scopes to a single entity
-      inner.getLocation().hasLocationFileInfo(_, startline, startcolumn, endline, endcolumn)
+      // Use the pre-order number of the inner scope as the number of the access. This allows
+      // us to collapse multiple accesses in inner scopes to a single entity
+      ord = getPreOrderNumbering(scope, inner)
     )
   }
 
@@ -375,15 +464,12 @@ module Impl {
     }
 
     pragma[nomagic]
-    predicate rankBy(
-      string name, VariableScope scope, int startline, int startcolumn, int endline, int endcolumn
-    ) {
-      variableDeclInScope(this.asVariable(), scope, name, startline, startcolumn) and
-      endline = -1 and
-      endcolumn = -1
+    predicate rankBy(string name, VariableScope scope, int ord, int kind) {
+      variableDeclInScope(this.asVariable(), scope, name, ord) and
+      kind = 0
       or
-      variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, startline, startcolumn,
-        endline, endcolumn)
+      variableAccessCandInScope(this.asVariableAccessCand(), scope, name, _, ord) and
+      kind = 1
     }
   }
 
@@ -396,11 +482,10 @@ module Impl {
 
     int getRank(VariableScope scope, string name, VariableOrAccessCand v) {
       v =
-        rank[result](VariableOrAccessCand v0, int startline, int startcolumn, int endline,
-          int endcolumn |
-          v0.rankBy(name, scope, startline, startcolumn, endline, endcolumn)
+        rank[result](VariableOrAccessCand v0, int ord, int kind |
+          v0.rankBy(name, scope, ord, kind)
         |
-          v0 order by startline, startcolumn, endline, endcolumn
+          v0 order by ord, kind
         )
     }
   }
@@ -440,7 +525,7 @@ module Impl {
     exists(int rnk |
       variableReachesRank(scope, name, v, rnk) and
       rnk = rankVariableOrAccess(scope, name, TVariableOrAccessCandVariableAccessCand(cand)) and
-      variableAccessCandInScope(cand, scope, name, nestLevel, _, _, _, _)
+      variableAccessCandInScope(cand, scope, name, nestLevel, _)
     )
   }
 

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

@@ -1,6 +1,7 @@
 models
 | 1 | Summary: lang:alloc; <crate::string::String>::as_str; Argument[self]; ReturnValue; taint |
 | 2 | Summary: lang:alloc; crate::fmt::format; Argument[0]; ReturnValue; taint |
+| 3 | Summary: lang:core; crate::hint::must_use; Argument[0]; ReturnValue; value |
 edges
 | main.rs:26:9:26:9 | s | main.rs:27:19:27:25 | s[...] | provenance |  |
 | main.rs:26:13:26:22 | source(...) | main.rs:26:9:26:9 | s | provenance |  |
@@ -27,6 +28,19 @@ edges
 | main.rs:77:9:77:18 | formatted3 | main.rs:78:10:78:19 | formatted3 | provenance |  |
 | main.rs:77:22:77:75 | ...::format(...) | main.rs:77:9:77:18 | formatted3 | provenance |  |
 | main.rs:77:34:77:74 | MacroExpr | main.rs:77:22:77:75 | ...::format(...) | provenance | MaD:2 |
+| main.rs:82:9:82:10 | s1 | main.rs:86:18:86:25 | MacroExpr | provenance |  |
+| main.rs:82:9:82:10 | s1 | main.rs:87:18:87:32 | MacroExpr | provenance |  |
+| main.rs:82:14:82:23 | source(...) | main.rs:82:9:82:10 | s1 | provenance |  |
+| main.rs:86:10:86:26 | res | main.rs:86:18:86:25 | { ... } | provenance |  |
+| main.rs:86:18:86:25 | ...::format(...) | main.rs:86:10:86:26 | res | provenance |  |
+| main.rs:86:18:86:25 | ...::must_use(...) | main.rs:86:10:86:26 | MacroExpr | provenance |  |
+| main.rs:86:18:86:25 | MacroExpr | main.rs:86:18:86:25 | ...::format(...) | provenance | MaD:2 |
+| main.rs:86:18:86:25 | { ... } | main.rs:86:18:86:25 | ...::must_use(...) | provenance | MaD:3 |
+| main.rs:87:10:87:33 | res | main.rs:87:18:87:32 | { ... } | provenance |  |
+| main.rs:87:18:87:32 | ...::format(...) | main.rs:87:10:87:33 | res | provenance |  |
+| main.rs:87:18:87:32 | ...::must_use(...) | main.rs:87:10:87:33 | MacroExpr | provenance |  |
+| main.rs:87:18:87:32 | MacroExpr | main.rs:87:18:87:32 | ...::format(...) | provenance | MaD:2 |
+| main.rs:87:18:87:32 | { ... } | main.rs:87:18:87:32 | ...::must_use(...) | provenance | MaD:3 |
 nodes
 | main.rs:26:9:26:9 | s | semmle.label | s |
 | main.rs:26:13:26:22 | source(...) | semmle.label | source(...) |
@@ -58,6 +72,20 @@ nodes
 | main.rs:77:22:77:75 | ...::format(...) | semmle.label | ...::format(...) |
 | main.rs:77:34:77:74 | MacroExpr | semmle.label | MacroExpr |
 | main.rs:78:10:78:19 | formatted3 | semmle.label | formatted3 |
+| main.rs:82:9:82:10 | s1 | semmle.label | s1 |
+| main.rs:82:14:82:23 | source(...) | semmle.label | source(...) |
+| main.rs:86:10:86:26 | MacroExpr | semmle.label | MacroExpr |
+| main.rs:86:10:86:26 | res | semmle.label | res |
+| main.rs:86:18:86:25 | ...::format(...) | semmle.label | ...::format(...) |
+| main.rs:86:18:86:25 | ...::must_use(...) | semmle.label | ...::must_use(...) |
+| main.rs:86:18:86:25 | MacroExpr | semmle.label | MacroExpr |
+| main.rs:86:18:86:25 | { ... } | semmle.label | { ... } |
+| main.rs:87:10:87:33 | MacroExpr | semmle.label | MacroExpr |
+| main.rs:87:10:87:33 | res | semmle.label | res |
+| main.rs:87:18:87:32 | ...::format(...) | semmle.label | ...::format(...) |
+| main.rs:87:18:87:32 | ...::must_use(...) | semmle.label | ...::must_use(...) |
+| main.rs:87:18:87:32 | MacroExpr | semmle.label | MacroExpr |
+| main.rs:87:18:87:32 | { ... } | semmle.label | { ... } |
 subpaths
 testFailures
 #select
@@ -67,3 +95,5 @@ testFailures
 | main.rs:71:10:71:19 | formatted1 | main.rs:68:13:68:22 | source(...) | main.rs:71:10:71:19 | formatted1 | $@ | main.rs:68:13:68:22 | source(...) | source(...) |
 | main.rs:74:10:74:19 | formatted2 | main.rs:68:13:68:22 | source(...) | main.rs:74:10:74:19 | formatted2 | $@ | main.rs:68:13:68:22 | source(...) | source(...) |
 | main.rs:78:10:78:19 | formatted3 | main.rs:76:17:76:32 | source_usize(...) | main.rs:78:10:78:19 | formatted3 | $@ | main.rs:76:17:76:32 | source_usize(...) | source_usize(...) |
+| main.rs:86:10:86:26 | MacroExpr | main.rs:82:14:82:23 | source(...) | main.rs:86:10:86:26 | MacroExpr | $@ | main.rs:82:14:82:23 | source(...) | source(...) |
+| main.rs:87:10:87:33 | MacroExpr | main.rs:82:14:82:23 | source(...) | main.rs:87:10:87:33 | MacroExpr | $@ | main.rs:82:14:82:23 | source(...) | source(...) |

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

@@ -83,8 +83,8 @@ fn format_macro() {
     let s2 = "2";
     let s3 = "3";
 
-    sink(format!("{}", s1)); // $ MISSING: hasTaintFlow=34
-    sink(format!("{s1} and {s3}")); // $ MISSING: hasTaintFlow=34
+    sink(format!("{}", s1)); // $ hasTaintFlow=34
+    sink(format!("{s1} and {s3}")); // $ hasTaintFlow=34
     sink(format!("{s2} and {s3}"));
 }