Address review comments

github · Jan 30, 2025 · 4a57786 · 4a57786
1 parent 4581f02
commit 4a57786
Show file tree

Hide file tree

Showing 10 changed files with 100 additions and 80 deletions.
diff --git a/rust/ql/lib/codeql/rust/AstConsistency.qll b/rust/ql/lib/codeql/rust/AstConsistency.qll
@@ -75,8 +75,8 @@ query predicate multiplePositions(Element parent, int pos1, int pos2, string acc
 
 private import codeql.rust.elements.internal.PathResolution
 
-/** Holds if `p` may resolve to multiple items. */
-query predicate multiplePathResolutions(Path p, Item i) {
+/** Holds if `p` may resolve to multiple items including `i`. */
+query predicate multiplePathResolutions(Path p, ItemNode i) {
   i = resolvePath(p) and
   strictcount(resolvePath(p)) > 1
 }

diff --git a/rust/ql/lib/codeql/rust/elements/internal/PathImpl.qll b/rust/ql/lib/codeql/rust/elements/internal/PathImpl.qll
@@ -27,5 +27,11 @@ module Impl {
       then result = "...::" + this.getPart().toAbbreviatedString()
       else result = this.getPart().toAbbreviatedString()
     }
+
+    /**
+     * Gets the text of this path, if it exists.
+     */
+    pragma[nomagic]
+    string getText() { result = this.getPart().getNameRef().getText() }
   }
 }
diff --git a/rust/ql/lib/codeql/rust/elements/internal/PathResolution.qll b/rust/ql/lib/codeql/rust/elements/internal/PathResolution.qll
@@ -57,7 +57,7 @@ abstract class ItemNode extends AstNode {
   pragma[inline_late]
   predicate isPublic() { exists(this.getVisibility()) }
 
-  /** Gets an element that has this item as immediately enlcosing item. */
+  /** Gets an element that has this item as immediately enclosing item. */
   pragma[nomagic]
   Element getADescendant() {
     getImmediateParent(result) = this
@@ -77,9 +77,8 @@ abstract class ItemNode extends AstNode {
   pragma[nomagic]
   ModuleLikeNode getImmediateParentModule() { this = result.getAnItemInScope() }
 
-  /** Gets a successor named `name` of this item, if any. */
   pragma[nomagic]
-  ItemNode getASuccessor(string name) {
+  private ItemNode getASuccessorRec(string name) {
     sourceFileEdge(this, name, result)
     or
     this = result.getImmediateParent() and
@@ -91,23 +90,30 @@ abstract class ItemNode extends AstNode {
     or
     // items made available through `use` are available to nodes that contain the `use`
     exists(UseItemNode use |
-      use = this.getASuccessor(_) and
-      result = use.getASuccessor(name)
+      use = this.getASuccessorRec(_) and
+      result = use.(ItemNode).getASuccessorRec(name)
     )
     or
     // items made available through macro calls are available to nodes that contain the macro call
     exists(MacroCallItemNode call |
-      call = this.getASuccessor(_) and
-      result = call.getASuccessor(name)
+      call = this.getASuccessorRec(_) and
+      result = call.(ItemNode).getASuccessorRec(name)
     )
+  }
+
+  /** Gets a successor named `name` of this item, if any. */
+  pragma[nomagic]
+  ItemNode getASuccessor(string name) {
+    result = this.getASuccessorRec(name)
     or
     name = "super" and
     if this instanceof Module
     then result = this.getImmediateParentModule()
     else result = this.getImmediateParentModule().getImmediateParentModule()
     or
     name = "self" and
-    if this instanceof Module then result = this else result = this.getImmediateParentModule()
+    not this instanceof Module and
+    result = this.getImmediateParentModule()
     or
     name = "Self" and
     this = result.(ImplOrTraitItemNode).getAnItemInSelfScope()
@@ -224,6 +230,7 @@ private class BlockExprItemNode extends ItemNode instanceof BlockExpr {
   override Visibility getVisibility() { none() }
 }
 
+/** Holds if `item` has the name `name` and is a top-level item inside `f`. */
 private predicate sourceFileEdge(SourceFile f, string name, ItemNode item) {
   item = f.getAnItem() and
   name = item.getName()
@@ -257,7 +264,7 @@ private predicate modImport(Module m, string fileName, string name, Folder paren
     // #[path = "foo.rs"]
     // mod bar;
     // ```
-    not m.getAnAttr().getMeta().getPath().getPart().getNameRef().getText() = "path" and
+    not m.getAnAttr().getMeta().getPath().getText() = "path" and
     name = m.getName().getText() and
     parent = f.getParentContainer() and
     fileName = f.getStem()
@@ -303,7 +310,7 @@ private predicate useTreeDeclares(UseTree tree, string name) {
     name != "_"
     or
     not tree.hasRename() and
-    name = tree.getPath().getPart().getNameRef().getText()
+    name = tree.getPath().getText()
   )
   or
   exists(UseTree mid |
@@ -330,31 +337,32 @@ private predicate declares(ItemNode item, string name) {
   )
 }
 
+/** A path that does not access a local variable. */
 private class RelevantPath extends Path {
   RelevantPath() { not this = any(VariableAccess va).(PathExpr).getPath() }
 
   pragma[nomagic]
-  predicate isRoot(string name) {
+  predicate isUnqualified(string name) {
     not exists(this.getQualifier()) and
     not this = any(UseTreeList list).getAUseTree().getPath() and
-    name = this.getPart().getNameRef().getText()
+    name = this.getText()
   }
 }
 
 /**
- * Holds if the root path `root` references an item named `name`, and `name`
+ * Holds if the unqualified path `p` references an item named `name`, and `name`
  * may be looked up inside enclosing item `encl`.
  */
 pragma[nomagic]
-private predicate rootPathLookup(RelevantPath root, string name, ItemNode encl) {
+private predicate unqualifiedPathLookup(RelevantPath p, string name, ItemNode encl) {
   exists(ItemNode encl0 |
     // lookup in the immediately enclosing item
-    root.isRoot(name) and
-    encl0.getADescendant() = root
+    p.isUnqualified(name) and
+    encl0.getADescendant() = p
     or
     // lookup in an outer scope, but only if the item is not declared in inner scope
     exists(ItemNode mid |
-      rootPathLookup(root, name, mid) and
+      unqualifiedPathLookup(p, name, mid) and
       not declares(mid, name)
     |
       // nested modules do not have unqualified access to items from outer modules,
@@ -374,7 +382,7 @@ private predicate rootPathLookup(RelevantPath root, string name, ItemNode encl)
 cached
 ItemNode resolvePath(RelevantPath path) {
   exists(ItemNode encl, string name |
-    rootPathLookup(path, name, encl) and
+    unqualifiedPathLookup(path, name, encl) and
     result = encl.getASuccessor(name)
   )
   or
@@ -389,7 +397,7 @@ ItemNode resolvePath(RelevantPath path) {
 pragma[nomagic]
 private ItemNode resolvePathQualifier(RelevantPath path, string name) {
   result = resolvePath(path.getQualifier()) and
-  name = path.getPart().getNameRef().getText()
+  name = path.getText()
 }
 
 private predicate isUseTreeSubPath(UseTree tree, RelevantPath path) {
@@ -405,7 +413,7 @@ pragma[nomagic]
 private predicate isUseTreeSubPathUnqualified(UseTree tree, RelevantPath path, string name) {
   isUseTreeSubPath(tree, path) and
   not exists(path.getQualifier()) and
-  name = path.getPart().getNameRef().getText()
+  name = path.getText()
 }
 
 pragma[nomagic]
@@ -428,7 +436,7 @@ private ItemNode resolveUseTreeListItemQualifier(
   Use use, UseTree tree, RelevantPath path, string name
 ) {
   result = resolveUseTreeListItem(use, tree, path.getQualifier()) and
-  name = path.getPart().getNameRef().getText()
+  name = path.getText()
 }
 
 pragma[nomagic]

diff --git a/rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll b/rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll
@@ -25,10 +25,9 @@ class StreamCipherInit extends Cryptography::CryptographicOperation::Range {
     exists(PathExpr p, string rawAlgorithmName |
       this.asExpr().getExpr().(CallExpr).getFunction() = p and
       p.getResolvedCrateOrigin().matches("%/RustCrypto%") and
-      p.getPath().getPart().getNameRef().getText() =
-        ["new", "new_from_slice", "new_from_slices", "new_with_eff_key_len"] and
+      p.getPath().getText() = ["new", "new_from_slice", "new_from_slices", "new_with_eff_key_len"] and
       (
-        rawAlgorithmName = p.getPath().getQualifier().getPart().getNameRef().getText() or
+        rawAlgorithmName = p.getPath().getQualifier().(Path).getText() or // todo: remove infix cast when codegenerator has been fixed
         rawAlgorithmName =
           p.getPath()
               .getQualifier()

diff --git a/rust/ql/lib/codeql/rust/security/WeakSensitiveDataHashingExtensions.qll b/rust/ql/lib/codeql/rust/security/WeakSensitiveDataHashingExtensions.qll
@@ -186,8 +186,7 @@ class ModeledHashOperation extends Cryptography::CryptographicOperation::Range {
       sinkNode(input, "hasher-input") and
       call = input.(Node::FlowSummaryNode).getSinkElement().getCall() and
       call = this.asExpr().getExpr() and
-      algorithmName =
-        call.getFunction().(PathExpr).getPath().getQualifier().getPart().getNameRef().getText()
+      algorithmName = call.getFunction().(PathExpr).getPath().getQualifier().getText()
     )
   }
 

diff --git a/rust/ql/src/queries/security/CWE-696/BadCtorInitialization.ql b/rust/ql/src/queries/security/CWE-696/BadCtorInitialization.ql
@@ -20,7 +20,7 @@ class CtorAttr extends Attr {
   string whichAttr;
 
   CtorAttr() {
-    whichAttr = this.getMeta().getPath().getPart().getNameRef().getText() and
+    whichAttr = this.getMeta().getPath().getText() and
     whichAttr = ["ctor", "dtor"]
   }
 

diff --git a/rust/ql/test/library-tests/path-resolution/main.rs b/rust/ql/test/library-tests/path-resolution/main.rs
@@ -181,6 +181,16 @@ mod m8 {
     } // I55
 } // I46
 
+mod m9 {
+    pub struct MyStruct {} // I56
+
+    #[rustfmt::skip]
+    pub fn f() -> self::MyStruct { // $ item=I56
+        println!("main.rs::m9::f");
+        self::MyStruct {} // $ item=I56
+    } // I57
+}
+
 fn main() {
     my::nested::nested1::nested2::f(); // $ item=I4
     my::f(); // $ item=I38
@@ -198,4 +208,5 @@ fn main() {
     m6::g(); // $ item=I36
     m7::f(); // $ item=I45
     m8::g(); // $ item=I55
+    m9::f(); // $ item=I57
 }
diff --git a/rust/ql/test/library-tests/path-resolution/path-resolution.expected b/rust/ql/test/library-tests/path-resolution/path-resolution.expected
@@ -11,6 +11,7 @@ mod
 | main.rs:109:1:120:1 | mod m6 |
 | main.rs:122:1:137:1 | mod m7 |
 | main.rs:139:1:182:1 | mod m8 |
+| main.rs:184:1:192:1 | mod m9 |
 | my2/mod.rs:1:1:1:16 | mod nested2 |
 | my2/nested2.rs:1:1:11:1 | mod nested3 |
 | my2/nested2.rs:2:5:10:5 | mod nested4 |
@@ -36,7 +37,7 @@ resolvePath
 | main.rs:30:17:30:21 | super | main.rs:18:5:36:5 | mod m2 |
 | main.rs:30:17:30:24 | ...::f | main.rs:19:9:21:9 | fn f |
 | main.rs:33:17:33:17 | f | main.rs:19:9:21:9 | fn f |
-| main.rs:40:9:40:13 | super | main.rs:1:1:201:2 | SourceFile |
+| main.rs:40:9:40:13 | super | main.rs:1:1:212:2 | SourceFile |
 | main.rs:40:9:40:17 | ...::m1 | main.rs:13:1:37:1 | mod m1 |
 | main.rs:40:9:40:21 | ...::m2 | main.rs:18:5:36:5 | mod m2 |
 | main.rs:40:9:40:24 | ...::g | main.rs:23:9:27:9 | fn g |
@@ -48,7 +49,7 @@ resolvePath
 | main.rs:61:17:61:19 | Foo | main.rs:59:9:59:21 | struct Foo |
 | main.rs:64:13:64:15 | Foo | main.rs:53:5:53:17 | struct Foo |
 | main.rs:66:5:66:5 | f | main.rs:55:5:62:5 | fn f |
-| main.rs:68:5:68:8 | self | main.rs:1:1:201:2 | SourceFile |
+| main.rs:68:5:68:8 | self | main.rs:1:1:212:2 | SourceFile |
 | main.rs:68:5:68:11 | ...::i | main.rs:71:1:83:1 | fn i |
 | main.rs:74:13:74:15 | Foo | main.rs:48:1:48:13 | struct Foo |
 | main.rs:81:17:81:19 | Foo | main.rs:77:9:79:9 | struct Foo |
@@ -62,7 +63,7 @@ resolvePath
 | main.rs:87:57:87:66 | ...::g | my2/nested2.rs:7:9:9:9 | fn g |
 | main.rs:87:80:87:86 | nested4 | my2/nested2.rs:2:5:10:5 | mod nested4 |
 | main.rs:100:5:100:22 | f_defined_in_macro | main.rs:99:18:99:42 | fn f_defined_in_macro |
-| main.rs:117:13:117:17 | super | main.rs:1:1:201:2 | SourceFile |
+| main.rs:117:13:117:17 | super | main.rs:1:1:212:2 | SourceFile |
 | main.rs:117:13:117:21 | ...::m5 | main.rs:103:1:107:1 | mod m5 |
 | main.rs:118:9:118:9 | f | main.rs:104:5:106:5 | fn f |
 | main.rs:118:9:118:9 | f | main.rs:110:5:112:5 | fn f |
@@ -88,40 +89,46 @@ resolvePath
 | main.rs:173:10:173:17 | MyStruct | main.rs:150:5:150:22 | struct MyStruct |
 | main.rs:177:17:177:24 | MyStruct | main.rs:150:5:150:22 | struct MyStruct |
 | main.rs:179:17:179:24 | MyStruct | main.rs:150:5:150:22 | struct MyStruct |
-| main.rs:185:5:185:6 | my | main.rs:1:1:1:7 | mod my |
-| main.rs:185:5:185:14 | ...::nested | my.rs:1:1:1:15 | mod nested |
-| main.rs:185:5:185:23 | ...::nested1 | my/nested.rs:1:1:17:1 | mod nested1 |
-| main.rs:185:5:185:32 | ...::nested2 | my/nested.rs:2:5:11:5 | mod nested2 |
-| main.rs:185:5:185:35 | ...::f | my/nested.rs:3:9:5:9 | fn f |
-| main.rs:186:5:186:6 | my | main.rs:1:1:1:7 | mod my |
-| main.rs:186:5:186:9 | ...::f | my.rs:5:1:7:1 | fn f |
-| main.rs:187:5:187:11 | nested2 | my2/mod.rs:1:1:1:16 | mod nested2 |
-| main.rs:187:5:187:20 | ...::nested3 | my2/nested2.rs:1:1:11:1 | mod nested3 |
-| main.rs:187:5:187:29 | ...::nested4 | my2/nested2.rs:2:5:10:5 | mod nested4 |
-| main.rs:187:5:187:32 | ...::f | my2/nested2.rs:3:9:5:9 | fn f |
-| main.rs:188:5:188:5 | f | my2/nested2.rs:3:9:5:9 | fn f |
-| main.rs:189:5:189:5 | g | my2/nested2.rs:7:9:9:9 | fn g |
-| main.rs:190:5:190:9 | crate | main.rs:1:1:201:2 | SourceFile |
-| main.rs:190:5:190:12 | ...::h | main.rs:50:1:69:1 | fn h |
-| main.rs:191:5:191:6 | m1 | main.rs:13:1:37:1 | mod m1 |
-| main.rs:191:5:191:10 | ...::m2 | main.rs:18:5:36:5 | mod m2 |
-| main.rs:191:5:191:13 | ...::g | main.rs:23:9:27:9 | fn g |
-| main.rs:192:5:192:6 | m1 | main.rs:13:1:37:1 | mod m1 |
-| main.rs:192:5:192:10 | ...::m2 | main.rs:18:5:36:5 | mod m2 |
-| main.rs:192:5:192:14 | ...::m3 | main.rs:29:9:35:9 | mod m3 |
-| main.rs:192:5:192:17 | ...::h | main.rs:30:27:34:13 | fn h |
-| main.rs:193:5:193:6 | m4 | main.rs:39:1:46:1 | mod m4 |
-| main.rs:193:5:193:9 | ...::i | main.rs:42:5:45:5 | fn i |
-| main.rs:194:5:194:5 | h | main.rs:50:1:69:1 | fn h |
-| main.rs:195:5:195:11 | f_alias | my2/nested2.rs:3:9:5:9 | fn f |
-| main.rs:196:5:196:11 | g_alias | my2/nested2.rs:7:9:9:9 | fn g |
-| main.rs:197:5:197:5 | j | main.rs:97:1:101:1 | fn j |
-| main.rs:198:5:198:6 | m6 | main.rs:109:1:120:1 | mod m6 |
-| main.rs:198:5:198:9 | ...::g | main.rs:114:5:119:5 | fn g |
-| main.rs:199:5:199:6 | m7 | main.rs:122:1:137:1 | mod m7 |
-| main.rs:199:5:199:9 | ...::f | main.rs:129:5:136:5 | fn f |
-| main.rs:200:5:200:6 | m8 | main.rs:139:1:182:1 | mod m8 |
-| main.rs:200:5:200:9 | ...::g | main.rs:169:5:181:5 | fn g |
+| main.rs:188:19:188:22 | self | main.rs:184:1:192:1 | mod m9 |
+| main.rs:188:19:188:32 | ...::MyStruct | main.rs:185:5:185:26 | struct MyStruct |
+| main.rs:190:9:190:12 | self | main.rs:184:1:192:1 | mod m9 |
+| main.rs:190:9:190:22 | ...::MyStruct | main.rs:185:5:185:26 | struct MyStruct |
+| main.rs:195:5:195:6 | my | main.rs:1:1:1:7 | mod my |
+| main.rs:195:5:195:14 | ...::nested | my.rs:1:1:1:15 | mod nested |
+| main.rs:195:5:195:23 | ...::nested1 | my/nested.rs:1:1:17:1 | mod nested1 |
+| main.rs:195:5:195:32 | ...::nested2 | my/nested.rs:2:5:11:5 | mod nested2 |
+| main.rs:195:5:195:35 | ...::f | my/nested.rs:3:9:5:9 | fn f |
+| main.rs:196:5:196:6 | my | main.rs:1:1:1:7 | mod my |
+| main.rs:196:5:196:9 | ...::f | my.rs:5:1:7:1 | fn f |
+| main.rs:197:5:197:11 | nested2 | my2/mod.rs:1:1:1:16 | mod nested2 |
+| main.rs:197:5:197:20 | ...::nested3 | my2/nested2.rs:1:1:11:1 | mod nested3 |
+| main.rs:197:5:197:29 | ...::nested4 | my2/nested2.rs:2:5:10:5 | mod nested4 |
+| main.rs:197:5:197:32 | ...::f | my2/nested2.rs:3:9:5:9 | fn f |
+| main.rs:198:5:198:5 | f | my2/nested2.rs:3:9:5:9 | fn f |
+| main.rs:199:5:199:5 | g | my2/nested2.rs:7:9:9:9 | fn g |
+| main.rs:200:5:200:9 | crate | main.rs:1:1:212:2 | SourceFile |
+| main.rs:200:5:200:12 | ...::h | main.rs:50:1:69:1 | fn h |
+| main.rs:201:5:201:6 | m1 | main.rs:13:1:37:1 | mod m1 |
+| main.rs:201:5:201:10 | ...::m2 | main.rs:18:5:36:5 | mod m2 |
+| main.rs:201:5:201:13 | ...::g | main.rs:23:9:27:9 | fn g |
+| main.rs:202:5:202:6 | m1 | main.rs:13:1:37:1 | mod m1 |
+| main.rs:202:5:202:10 | ...::m2 | main.rs:18:5:36:5 | mod m2 |
+| main.rs:202:5:202:14 | ...::m3 | main.rs:29:9:35:9 | mod m3 |
+| main.rs:202:5:202:17 | ...::h | main.rs:30:27:34:13 | fn h |
+| main.rs:203:5:203:6 | m4 | main.rs:39:1:46:1 | mod m4 |
+| main.rs:203:5:203:9 | ...::i | main.rs:42:5:45:5 | fn i |
+| main.rs:204:5:204:5 | h | main.rs:50:1:69:1 | fn h |
+| main.rs:205:5:205:11 | f_alias | my2/nested2.rs:3:9:5:9 | fn f |
+| main.rs:206:5:206:11 | g_alias | my2/nested2.rs:7:9:9:9 | fn g |
+| main.rs:207:5:207:5 | j | main.rs:97:1:101:1 | fn j |
+| main.rs:208:5:208:6 | m6 | main.rs:109:1:120:1 | mod m6 |
+| main.rs:208:5:208:9 | ...::g | main.rs:114:5:119:5 | fn g |
+| main.rs:209:5:209:6 | m7 | main.rs:122:1:137:1 | mod m7 |
+| main.rs:209:5:209:9 | ...::f | main.rs:129:5:136:5 | fn f |
+| main.rs:210:5:210:6 | m8 | main.rs:139:1:182:1 | mod m8 |
+| main.rs:210:5:210:9 | ...::g | main.rs:169:5:181:5 | fn g |
+| main.rs:211:5:211:6 | m9 | main.rs:184:1:192:1 | mod m9 |
+| main.rs:211:5:211:9 | ...::f | main.rs:187:5:191:5 | fn f |
 | my2/mod.rs:5:5:5:11 | nested2 | my2/mod.rs:1:1:1:16 | mod nested2 |
 | my2/mod.rs:5:5:5:20 | ...::nested3 | my2/nested2.rs:1:1:11:1 | mod nested3 |
 | my2/mod.rs:5:5:5:29 | ...::nested4 | my2/nested2.rs:2:5:10:5 | mod nested4 |

diff --git a/rust/ql/test/library-tests/path-resolution/path-resolution.ql b/rust/ql/test/library-tests/path-resolution/path-resolution.ql
@@ -23,14 +23,9 @@ module ResolveTest implements TestSig {
 
   private predicate item(ItemNode i, string value) {
     exists(string filepath, int line, boolean inMacro | itemAt(i, filepath, line, inMacro) |
-      commmentAt(value, filepath, line) and
-      inMacro = false
+      commmentAt(value, filepath, line) and inMacro = false
       or
-      (
-        not commmentAt(_, filepath, line)
-        or
-        inMacro = true
-      ) and
+      not (commmentAt(_, filepath, line) and inMacro = false) and
       value = i.getName()
     )
   }

diff --git a/rust/ql/test/library-tests/variables/variables.ql b/rust/ql/test/library-tests/variables/variables.ql
@@ -37,14 +37,9 @@ module VariableAccessTest implements TestSig {
 
   private predicate decl(Variable v, string value) {
     exists(string filepath, int line, boolean inMacro | declAt(v, filepath, line, inMacro) |
-      commmentAt(value, filepath, line) and
-      inMacro = false
+      commmentAt(value, filepath, line) and inMacro = false
       or
-      (
-        not commmentAt(_, filepath, line)
-        or
-        inMacro = true
-      ) and
+      not (commmentAt(_, filepath, line) and inMacro = true) and
       value = v.getName()
     )
   }