Rust: Also check visibility in method call resolution

Author: hvitved

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

@@ -1112,7 +1112,7 @@ private predicate pathUsesNamespace(Path p, Namespace n) {
 }
 
 pragma[nomagic]
-private ItemNode resolvePath1(RelevantPath path) {
+private ItemNode resolvePathCand(AstNode path) {
   exists(Namespace ns | result = resolvePath0(path, ns) |
     pathUsesNamespace(path, ns)
     or
@@ -1121,54 +1121,65 @@ private ItemNode resolvePath1(RelevantPath path) {
   )
 }
 
-pragma[nomagic]
-private ItemNode resolvePathPrivate(
-  RelevantPath path, ModuleLikeNode itemParent, ModuleLikeNode pathParent
-) {
-  result = resolvePath1(path) and
-  result.isPrivate() and
-  (
-    pathParent.getADescendant() = path
-    or
-    pathParent = any(ItemNode mid | path = mid.getADescendant()).getImmediateParentModule()
-  ) and
-  (
-    itemParent = result.getVisibilityParent().getImmediateParentModule()
-    or
-    not result.hasVisibilityParent() and
-    itemParent = result.getImmediateParentModule()
-  )
-}
+/** Gets an item that `n` may resolve to, not taking visibility into account. */
+signature ItemNode resolveCandidateSig(AstNode n);
 
-pragma[nomagic]
-private predicate isItemParent(ModuleLikeNode itemParent) {
-  exists(resolvePathPrivate(_, itemParent, _))
-}
+/** Provides the predicate `resolve` for resolving items while taking visibility into account. */
+module ResolveWithVisibility<resolveCandidateSig/1 resolvePrivateCandidate> {
+  pragma[nomagic]
+  private ItemNode resolvePathPrivate(
+    AstNode n, ModuleLikeNode itemParent, ModuleLikeNode pathParent
+  ) {
+    result = resolvePrivateCandidate(n) and
+    result.isPrivate() and
+    (
+      pathParent.getADescendant() = n
+      or
+      pathParent = any(ItemNode mid | n = mid.getADescendant()).getImmediateParentModule()
+    ) and
+    (
+      itemParent = result.getVisibilityParent().getImmediateParentModule()
+      or
+      not result.hasVisibilityParent() and
+      itemParent = result.getImmediateParentModule()
+    )
+  }
 
-/**
- * Gets a module that has access to private items defined inside `itemParent`.
- *
- * According to [The Rust Reference][1] this is either `itemParent` itself or any
- * descendant of `itemParent`.
- *
- * [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access
- */
-pragma[nomagic]
-private ModuleLikeNode getAPrivateVisibleModule(ModuleLikeNode itemParent) {
-  isItemParent(itemParent) and
-  result.getImmediateParentModule*() = itemParent
+  pragma[nomagic]
+  private predicate isItemParent(ModuleLikeNode itemParent) {
+    exists(resolvePathPrivate(_, itemParent, _))
+  }
+
+  /**
+   * Gets a module that has access to private items defined inside `itemParent`.
+   *
+   * According to [The Rust Reference][1] this is either `itemParent` itself or any
+   * descendant of `itemParent`.
+   *
+   * [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access
+   */
+  pragma[nomagic]
+  private ModuleLikeNode getAPrivateVisibleModule(ModuleLikeNode itemParent) {
+    isItemParent(itemParent) and
+    result.getImmediateParentModule*() = itemParent
+  }
+
+  /** Gets an item that `n` may resolve to, taking visibility into account. */
+  ItemNode resolve(AstNode n) {
+    result = resolvePrivateCandidate(n) and
+    result.isPublic()
+    or
+    exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
+      result = resolvePathPrivate(n, itemParent, pathParent) and
+      pathParent = getAPrivateVisibleModule(itemParent)
+    )
+  }
 }
 
 /** Gets the item that `path` resolves to, if any. */
 cached
 ItemNode resolvePath(RelevantPath path) {
-  result = resolvePath1(path) and
-  result.isPublic()
-  or
-  exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
-    result = resolvePathPrivate(path, itemParent, pathParent) and
-    pathParent = getAPrivateVisibleModule(itemParent)
-  )
+  result = ResolveWithVisibility<resolvePathCand/1>::resolve(path)
 }
 
 pragma[nomagic]

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

@@ -913,12 +913,16 @@ private module Cached {
     name = mce.getIdentifier().getText()
   }
 
+  private ItemNode resolveMethodCallExprCand(AstNode mce) {
+    exists(string name | result = getMethodCallExprLookupType(mce, name).getMethod(name))
+  }
+
   /**
    * Gets a method that the method call `mce` resolves to, if any.
    */
   cached
   Function resolveMethodCallExpr(MethodCallExpr mce) {
-    exists(string name | result = getMethodCallExprLookupType(mce, name).getMethod(name))
+    result = ResolveWithVisibility<resolveMethodCallExprCand/1>::resolve(mce)
   }
 
   pragma[nomagic]

rust/ql/test/library-tests/path-resolution/illegal/main.rs

@@ -47,7 +47,7 @@ mod m1 {
         pub fn f() {
             println!("m1::nested2::f");
             super::S(). // $ item=I1
-                f(); // $ SPURIOUS: item=I4 (private)
+                f(); // illegal: private
             super::S::f( // illegal: private
                 super::S() // $ item=I1
             );
@@ -62,7 +62,7 @@ mod m1 {
                 super::S() // $ item=I1
             );
             super::S(). // $ item=I1
-                f2(); // $ SPURIOUS: item=I7 (private)
+                f2(); // illegal: private
             super::S::f2(
                 super::S() // $ item=I1
             ); // illegal: private