Rust: Account for trait visibility when resolving paths and methods

2025-12-16 16:53:25 +01:00 · 2025-08-29 12:00:20 +02:00
parent c1c7127894
commit 322ef4d985
11 changed files with 125 additions and 45 deletions
--- a/rust/ql/lib/codeql/rust/internal/PathResolution.qll
+++ b/rust/ql/lib/codeql/rust/internal/PathResolution.qll
@@ -216,7 +216,7 @@ abstract class ItemNode extends Locatable {
    // items made available through `use` are available to nodes that contain the `use`
    exists(UseItemNode use |
      use = this.getASuccessor(_, _) and
-      result = use.(ItemNode).getASuccessor(name, kind)
+      result = use.getASuccessor(name, kind)
    )
    or
    exists(ExternCrateItemNode ec | result = ec.(ItemNode).getASuccessor(name, kind) |
@@ -240,12 +240,7 @@ abstract class ItemNode extends Locatable {
      )
    or
    // items made available by an implementation where `this` is the implementing type
-    exists(ItemNode node |
-      this = node.(ImplItemNodeImpl).resolveSelfTyCand() and
-      result = node.getASuccessor(name, kind) and
-      kind.isExternalOrBoth() and
-      result instanceof AssocItemNode
-    )
+    typeImplEdge(this, _, name, kind, result)
    or
    // trait items with default implementations made available in an implementation
    exists(ImplItemNodeImpl impl, ItemNode trait |
@@ -1311,6 +1306,7 @@ private predicate declares(ItemNode item, Namespace ns, string name) {
 class RelevantPath extends Path {
  RelevantPath() { not this = any(VariableAccess va).(PathExpr).getPath() }

+  /** Holds if this is an unqualified path with the textual value `name`. */
  pragma[nomagic]
  predicate isUnqualified(string name) {
    not exists(this.getQualifier()) and
@@ -1421,6 +1417,35 @@ private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns, SuccessorKi
 pragma[nomagic]
 private predicate isUnqualifiedSelfPath(RelevantPath path) { path.isUnqualified("Self") }

+/** Provides the input to `TraitIsVisible`. */
+signature predicate relevantTraitVisibleSig(Element element, Trait trait);
+
+/**
+ * Provides the `traitIsVisible` predicate for determining if a trait is visible
+ * at a given element.
+ */
+module TraitIsVisible<relevantTraitVisibleSig/2 relevantTraitVisible> {
+  /** Holds if the trait might be looked up in `encl`. */
+  private predicate traitLookup(ItemNode encl, Element element, Trait trait) {
+    // lookup in immediately enclosing item
+    relevantTraitVisible(element, trait) and
+    encl.getADescendant() = element
+    or
+    // lookup in an outer scope, but only if the trait is not declared in inner scope
+    exists(ItemNode mid |
+      traitLookup(mid, element, trait) and
+      not trait = mid.getASuccessor(_, _) and
+      encl = getOuterScope(mid)
+    )
+  }
+
+  /** Holds if the trait `trait` is visible at `element`. */
+  pragma[nomagic]
+  predicate traitIsVisible(Element element, Trait trait) {
+    exists(ItemNode encl | traitLookup(encl, element, trait) and trait = encl.getASuccessor(_, _))
+  }
+}
+
 pragma[nomagic]
 private ItemNode resolvePathCand0(RelevantPath path, Namespace ns) {
  exists(ItemNode res |
@@ -1446,6 +1471,10 @@ private ItemNode resolvePathCandQualifier(RelevantPath qualifier, RelevantPath p
  name = path.getText()
 }

+/**
+ * Gets the item that `path` resolves to in `ns` when `qualifier` is the
+ * qualifier of `path` and `qualifier` resolves to `q`, if any.
+ */
 pragma[nomagic]
 private ItemNode resolvePathCandQualified(
  RelevantPath qualifier, ItemNode q, RelevantPath path, Namespace ns
@@ -1520,11 +1549,31 @@ private ItemNode resolvePathCand(RelevantPath path) {
  )
 }

+/** Get a trait that should be visible when `path` resolves to `node`, if any. */
+private Trait getResolvePathTraitUsed(RelevantPath path, AssocItemNode node) {
+  exists(TypeItemNode type, ImplItemNodeImpl impl |
+    node = resolvePathCandQualified(_, type, path, _) and
+    typeImplEdge(type, impl, _, _, node) and
+    result = impl.resolveTraitTyCand()
+  )
+}
+
+private predicate pathTraitUsed(Element path, Trait trait) {
+  trait = getResolvePathTraitUsed(path, _)
+}
+
 /** Gets the item that `path` resolves to, if any. */
 cached
 ItemNode resolvePath(RelevantPath path) {
  result = resolvePathCand(path) and
-  not path = any(Path parent | exists(resolvePathCand(parent))).getQualifier()
+  not path = any(Path parent | exists(resolvePathCand(parent))).getQualifier() and
+  (
+    // When the result is an associated item of a trait implementation the
+    // implemented trait must be visible.
+    TraitIsVisible<pathTraitUsed/2>::traitIsVisible(path, getResolvePathTraitUsed(path, result))
+    or
+    not exists(getResolvePathTraitUsed(path, result))
+  )
  or
  // if `path` is the qualifier of a resolvable `parent`, then we should
  // resolve `path` to something consistent with what `parent` resolves to
@@ -1606,8 +1655,16 @@ private predicate useImportEdge(Use use, string name, ItemNode item, SuccessorKi
        not tree.hasRename() and
        name = item.getName()
        or
-        name = tree.getRename().getName().getText() and
-        name != "_"
+        exists(Rename rename | rename = tree.getRename() |
+          name = rename.getName().getText()
+          or
+          // When the rename doesn't have a name it's an underscore import. This
+          // makes the imported item visible but unnameable. We represent this
+          // by using the name `_` which can never occur in a path.  See also:
+          // https://doc.rust-lang.org/reference/items/use-declarations.html#r-items.use.as-underscore
+          not rename.hasName() and
+          name = "_"
+        )
      )
    )
  )
@@ -1629,6 +1686,18 @@ private predicate externCrateEdge(ExternCrateItemNode ec, string name, CrateItem
  )
 }

+/**
+ * Holds if `typeItem` is the implementing type of `impl` and the implementation
+ * makes `assoc` available as `name` at `kind`.
+ */
+private predicate typeImplEdge(
+  TypeItemNode typeItem, ImplItemNodeImpl impl, string name, SuccessorKind kind, AssocItemNode assoc
+) {
+  typeItem = impl.resolveSelfTyCand() and
+  assoc = impl.getASuccessor(name, kind) and
+  kind.isExternalOrBoth()
+}
+
 pragma[nomagic]
 private predicate preludeItem(string name, ItemNode i) {
  exists(Crate stdOrCore, ModuleLikeNode mod, ModuleItemNode prelude, ModuleItemNode rust |
@@ -1693,7 +1762,7 @@ private module Debug {
    useImportEdge(use, name, item, kind)
  }

-  ItemNode debuggetASuccessor(ItemNode i, string name, SuccessorKind kind) {
+  ItemNode debugGetASuccessor(ItemNode i, string name, SuccessorKind kind) {
    i = getRelevantLocatable() and
    result = i.getASuccessor(name, kind)
  }
--- a/rust/ql/lib/codeql/rust/internal/TypeInference.qll
+++ b/rust/ql/lib/codeql/rust/internal/TypeInference.qll
@@ -1891,7 +1891,7 @@ private predicate methodCandidate(Type type, string name, int arity, Impl impl)
 */
 pragma[nomagic]
 private predicate methodCandidateTrait(Type type, Trait trait, string name, int arity, Impl impl) {
-  trait = resolvePath(impl.(ImplItemNode).getTraitPath()) and
+  trait = impl.(ImplItemNode).resolveTraitTy() and
  methodCandidate(type, name, arity, impl)
 }

@@ -1903,19 +1903,53 @@ private predicate isMethodCall(MethodCall mc, Type rootType, string name, int ar
 }

 private module IsInstantiationOfInput implements IsInstantiationOfInputSig<MethodCall> {
+  /** Holds if `mc` specifies a trait and might target a method in `impl`. */
  pragma[nomagic]
-  predicate potentialInstantiationOf(MethodCall mc, TypeAbstraction impl, TypeMention constraint) {
+  private predicate methodCallTraitCandidate(MethodCall mc, Impl impl) {
    exists(Type rootType, string name, int arity |
      isMethodCall(mc, rootType, name, arity) and
-      constraint = impl.(ImplTypeAbstraction).getSelfTy()
-    |
      methodCandidateTrait(rootType, mc.getTrait(), name, arity, impl)
-      or
+    )
+  }
+
+  /** Holds if `mc` does not specify a trait and might target a method in `impl`. */
+  pragma[nomagic]
+  private predicate methodCallCandidate(MethodCall mc, Impl impl) {
+    exists(Type rootType, string name, int arity |
      not exists(mc.getTrait()) and
+      isMethodCall(mc, rootType, name, arity) and
      methodCandidate(rootType, name, arity, impl)
    )
  }

+  private predicate relevantTraitVisible(Element mc, Trait trait) {
+    trait = any(ImplItemNode impl | methodCallCandidate(mc, impl)).resolveTraitTy()
+  }
+
+  bindingset[impl]
+  pragma[inline_late]
+  private TypeRepr getImplSelfTy(Impl impl) { result = impl.getSelfTy() }
+
+  pragma[nomagic]
+  predicate potentialInstantiationOf(MethodCall mc, TypeAbstraction impl, TypeMention constraint) {
+    constraint = getImplSelfTy(impl) and
+    (
+      methodCallTraitCandidate(mc, impl)
+      or
+      methodCallCandidate(mc, impl) and
+      (
+        not exists(impl.(ImplItemNode).resolveTraitTy())
+        or
+        // If the `impl` block implements a trait, that trait must be visible in
+        // order for the `impl` to be valid.
+        exists(Trait trait |
+          pragma[only_bind_into](trait) = impl.(ImplItemNode).resolveTraitTy() and
+          TraitIsVisible<relevantTraitVisible/2>::traitIsVisible(mc, pragma[only_bind_into](trait))
+        )
+      )
+    )
+  }
+
  predicate relevantTypeMention(TypeMention constraint) {
    exists(Impl impl | methodCandidate(_, _, _, impl) and constraint = impl.getSelfTy())
  }
--- a/rust/ql/test/library-tests/dataflow/sources/CONSISTENCY/PathResolutionConsistency.expected
+++ b/rust/ql/test/library-tests/dataflow/sources/CONSISTENCY/PathResolutionConsistency.expected
@@ -11,11 +11,8 @@ multipleCallTargets
 | test.rs:179:30:179:68 | ...::_print(...) |
 | test.rs:188:26:188:105 | ...::_print(...) |
 | test.rs:229:22:229:72 | ... .read_to_string(...) |
-| test.rs:513:22:513:50 | file.read_to_end(...) |
-| test.rs:519:22:519:53 | file.read_to_string(...) |
 | test.rs:697:18:697:38 | ...::_print(...) |
 | test.rs:702:18:702:45 | ...::_print(...) |
-| test.rs:706:25:706:49 | address.to_socket_addrs() |
 | test.rs:720:38:720:42 | ...::_print(...) |
 | test.rs:724:38:724:54 | ...::_print(...) |
 | test.rs:729:38:729:51 | ...::_print(...) |
@@ -76,12 +73,6 @@ multipleCallTargets
 | test.rs:977:14:977:29 | ...::_print(...) |
 | test.rs:979:27:979:36 | ...::_print(...) |
 | test.rs:980:28:980:41 | ...::_print(...) |
-| test_futures_io.rs:35:26:35:63 | pinned.poll_read(...) |
-| test_futures_io.rs:62:22:62:50 | pinned.poll_fill_buf(...) |
-| test_futures_io.rs:69:23:69:67 | ... .poll_fill_buf(...) |
-| test_futures_io.rs:93:26:93:63 | pinned.poll_read(...) |
-| test_futures_io.rs:116:22:116:50 | pinned.poll_fill_buf(...) |
-| test_futures_io.rs:145:26:145:49 | ...::with_capacity(...) |
 | web_frameworks.rs:13:14:13:22 | a.as_str() |
 | web_frameworks.rs:13:14:13:23 | a.as_str() |
 | web_frameworks.rs:14:14:14:24 | a.as_bytes() |
--- a/rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected
+++ b/rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected
@@ -1,6 +1,3 @@
 multipleCallTargets
 | main.rs:118:9:118:11 | f(...) |
-| main.rs:494:13:494:27 | ...::a_method(...) |
-| main.rs:498:13:498:27 | ...::a_method(...) |
-| main.rs:502:13:502:27 | ...::a_method(...) |
 | proc_macro.rs:9:5:9:10 | ...::new(...) |
--- a/rust/ql/test/library-tests/path-resolution/main.rs
+++ b/rust/ql/test/library-tests/path-resolution/main.rs
@@ -492,17 +492,17 @@ mod trait_visibility {
        {
            // Only the `Foo` trait is visible
            use m::Foo; // $ item=Foo
-            X::a_method(&x); // $ item=X_Foo::a_method SPURIOUS: item=X_Bar::a_method
+            X::a_method(&x); // $ item=X_Foo::a_method
        }
        {
            // Only the `Bar` trait is visible
            use m::Bar; // $ item=Bar
-            X::a_method(&x); // $ item=X_Bar::a_method SPURIOUS: item=X_Foo::a_method
+            X::a_method(&x); // $ item=X_Bar::a_method
        }
        {
            // Only the `Bar` trait is visible (but unnameable)
            use m::Bar as _; // $ item=Bar
-            X::a_method(&x); // $ item=X_Bar::a_method SPURIOUS: item=X_Foo::a_method
+            X::a_method(&x); // $ item=X_Bar::a_method
        }
        {
            // The `Bar` trait is not visible, but we can refer to its method
--- a/rust/ql/test/library-tests/path-resolution/path-resolution.expected
+++ b/rust/ql/test/library-tests/path-resolution/path-resolution.expected
@@ -236,16 +236,13 @@ resolvePath
 | main.rs:494:17:494:22 | ...::Foo | main.rs:465:9:467:9 | trait Foo |
 | main.rs:495:13:495:13 | X | main.rs:473:9:473:21 | struct X |
 | main.rs:495:13:495:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
-| main.rs:495:13:495:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
 | main.rs:499:17:499:17 | m | main.rs:464:5:486:5 | mod m |
 | main.rs:499:17:499:22 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
 | main.rs:500:13:500:13 | X | main.rs:473:9:473:21 | struct X |
-| main.rs:500:13:500:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
 | main.rs:500:13:500:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
 | main.rs:504:17:504:17 | m | main.rs:464:5:486:5 | mod m |
 | main.rs:504:17:504:22 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
 | main.rs:505:13:505:13 | X | main.rs:473:9:473:21 | struct X |
-| main.rs:505:13:505:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
 | main.rs:505:13:505:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
 | main.rs:510:13:510:13 | m | main.rs:464:5:486:5 | mod m |
 | main.rs:510:13:510:18 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
--- a/rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected
+++ b/rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected
@@ -1,8 +1,5 @@
 multipleCallTargets
 | dereference.rs:61:15:61:24 | e1.deref() |
-| main.rs:153:13:153:24 | x.a_method() |
-| main.rs:157:13:157:24 | x.a_method() |
-| main.rs:161:13:161:24 | x.a_method() |
 | main.rs:2357:13:2357:31 | ...::from(...) |
 | main.rs:2358:13:2358:31 | ...::from(...) |
 | main.rs:2359:13:2359:31 | ...::from(...) |
--- a/rust/ql/test/library-tests/type-inference/main.rs
+++ b/rust/ql/test/library-tests/type-inference/main.rs
@@ -150,15 +150,15 @@ mod trait_visibility {
        let x = X;
        {
            use m::Foo;
-            x.a_method(); // $ target=Foo::a_method SPURIOUS: target=Bar::a_method
+            x.a_method(); // $ target=Foo::a_method
        }
        {
            use m::Bar;
-            x.a_method(); // $ target=Bar::a_method SPURIOUS: target=Foo::a_method
+            x.a_method(); // $ target=Bar::a_method
        }
        {
            use m::Bar as _;
-            x.a_method(); // $ target=Bar::a_method SPURIOUS: target=Foo::a_method
+            x.a_method(); // $ target=Bar::a_method
        }
        {
            use m::Bar;
--- a/rust/ql/test/query-tests/security/CWE-117/CONSISTENCY/PathResolutionConsistency.expected
+++ b/rust/ql/test/query-tests/security/CWE-117/CONSISTENCY/PathResolutionConsistency.expected
@@ -1,3 +1,2 @@
 multipleCallTargets
 | main.rs:9:43:9:63 | ...::from(...) |
-| main.rs:44:19:44:32 | username.len() |
--- a/rust/ql/test/query-tests/security/CWE-312/CONSISTENCY/PathResolutionConsistency.expected
+++ b/rust/ql/test/query-tests/security/CWE-312/CONSISTENCY/PathResolutionConsistency.expected
@@ -72,7 +72,6 @@ multipleCallTargets
 | test_logging.rs:195:15:195:38 | ...::_eprint(...) |
 | test_logging.rs:229:30:229:71 | ... .as_str() |
 | test_logging.rs:242:16:242:61 | ... .as_bytes() |
-| test_logging.rs:243:5:245:66 | ... .write_all(...) |
 | test_logging.rs:245:20:245:65 | ... .as_bytes() |
 | test_logging.rs:248:15:248:60 | ... .as_bytes() |
 | test_logging.rs:251:15:251:60 | ... .as_bytes() |
--- a/rust/ql/test/query-tests/security/CWE-770/CONSISTENCY/PathResolutionConsistency.expected
+++ b/rust/ql/test/query-tests/security/CWE-770/CONSISTENCY/PathResolutionConsistency.expected
@@ -1,3 +0,0 @@
-multipleCallTargets
-| main.rs:229:13:229:40 | ...::with_capacity(...) |
-| main.rs:233:18:233:47 | ...::with_capacity(...) |