Skip to content

Rust: Remove visibility check in path resolution #19431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 2, 2025
Merged

Rust: Remove visibility check in path resolution #19431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 30 additions & 63 deletions rust/ql/lib/codeql/rust/internal/PathResolution.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ abstract class ItemNode extends Locatable {
/** Gets the `i`th type parameter of this item, if any. */
abstract TypeParam getTypeParam(int i);

/** Holds if this item is declared as `pub`. */
bindingset[this]
pragma[inline_late]
predicate isPublic() { exists(this.getVisibility()) }

/** Gets an element that has this item as immediately enclosing item. */
pragma[nomagic]
Element getADescendant() {
Expand Down Expand Up @@ -207,6 +202,11 @@ abstract class ItemNode extends Locatable {
result.(CrateItemNode).isPotentialDollarCrateTarget()
}

pragma[nomagic]
private predicate hasSourceFunction(string name) {
this.getASuccessorFull(name).(Function).fromSource()
}

/** Gets a successor named `name` of this item, if any. */
pragma[nomagic]
ItemNode getASuccessor(string name) {
Expand All @@ -219,7 +219,7 @@ abstract class ItemNode extends Locatable {
or
not result instanceof Function
or
not this.getASuccessorFull(name).(Function).fromSource()
not this.hasSourceFunction(name)
)
}

Expand Down Expand Up @@ -266,8 +266,6 @@ private class SourceFileItemNode extends ModuleLikeNode, SourceFile {

override Visibility getVisibility() { none() }

override predicate isPublic() { any() }

override TypeParam getTypeParam(int i) { none() }
}

Expand Down Expand Up @@ -330,8 +328,6 @@ class CrateItemNode extends ItemNode instanceof Crate {

override Visibility getVisibility() { none() }

override predicate isPublic() { any() }

override TypeParam getTypeParam(int i) { none() }
}

Expand Down Expand Up @@ -436,17 +432,17 @@ abstract class ImplOrTraitItemNode extends ItemNode {

pragma[nomagic]
private TypeParamItemNode resolveTypeParamPathTypeRepr(PathTypeRepr ptr) {
result = resolvePath(ptr.getPath())
result = resolvePathFull(ptr.getPath())
}

class ImplItemNode extends ImplOrTraitItemNode instanceof Impl {
Path getSelfPath() { result = super.getSelfTy().(PathTypeRepr).getPath() }

Path getTraitPath() { result = super.getTrait().(PathTypeRepr).getPath() }

ItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) }
ItemNode resolveSelfTy() { result = resolvePathFull(this.getSelfPath()) }

TraitItemNode resolveTraitTy() { result = resolvePath(this.getTraitPath()) }
TraitItemNode resolveTraitTy() { result = resolvePathFull(this.getTraitPath()) }

pragma[nomagic]
private TypeRepr getASelfTyArg() {
Expand Down Expand Up @@ -560,7 +556,7 @@ class TraitItemNode extends ImplOrTraitItemNode instanceof Trait {
}

pragma[nomagic]
ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) }
ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) }

override AssocItemNode getAnAssocItem() { result = super.getAssocItemList().getAnAssocItem() }

Expand Down Expand Up @@ -634,7 +630,7 @@ class TypeParamItemNode extends ItemNode instanceof TypeParam {
}

pragma[nomagic]
ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) }
ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) }

/**
* Holds if this type parameter has a trait bound. Examples:
Expand Down Expand Up @@ -897,12 +893,6 @@ class RelevantPath extends Path {
this.getQualifier().(RelevantPath).isCratePath("$crate", _) and
this.getText() = name
}

// TODO: Remove once the crate graph extractor generates publicly visible paths
predicate requiresExtractorWorkaround() {
not this.fromSource() and
this = any(RelevantPath p).getQualifier()
}
}

private predicate isModule(ItemNode m) { m instanceof Module }
Expand Down Expand Up @@ -1056,8 +1046,14 @@ private predicate pathUsesNamespace(Path p, Namespace n) {
)
}

/**
* Gets the item that `path` resolves to, if any.
*
* Whenever `path` can resolve to both a function in source code and in library
* code, both are included
*/
pragma[nomagic]
private ItemNode resolvePath1(RelevantPath path) {
private ItemNode resolvePathFull(RelevantPath path) {
exists(Namespace ns | result = resolvePath0(path, ns) |
pathUsesNamespace(path, ns)
or
Expand All @@ -1067,58 +1063,29 @@ private ItemNode resolvePath1(RelevantPath path) {
}

pragma[nomagic]
private ItemNode resolvePathPrivate(
RelevantPath path, ModuleLikeNode itemParent, ModuleLikeNode pathParent
) {
not path.requiresExtractorWorkaround() and
result = resolvePath1(path) and
itemParent = result.getImmediateParentModule() and
not result.isPublic() and
(
pathParent.getADescendant() = path
or
pathParent = any(ItemNode mid | path = mid.getADescendant()).getImmediateParentModule()
)
}

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
private predicate resolvesSourceFunction(RelevantPath path) {
resolvePathFull(path).(Function).fromSource()
}

/** Gets the item that `path` resolves to, if any. */
cached
ItemNode resolvePath(RelevantPath path) {
result = resolvePath1(path) and
result = resolvePathFull(path) and
(
result.isPublic()
// when a function exists in both source code and in library code, it is because
// we also extracted the source code as library code, and hence we only want
// the function from source code
result.fromSource()
or
path.requiresExtractorWorkaround()
)
or
exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
result = resolvePathPrivate(path, itemParent, pathParent) and
pathParent = getAPrivateVisibleModule(itemParent)
not result instanceof Function
or
not resolvesSourceFunction(path)
)
}

pragma[nomagic]
private ItemNode resolvePathQualifier(RelevantPath path, string name) {
result = resolvePath(path.getQualifier()) and
result = resolvePathFull(path.getQualifier()) and
name = path.getText()
}

Expand Down Expand Up @@ -1164,7 +1131,7 @@ private ItemNode resolveUseTreeListItemQualifier(
pragma[nomagic]
private ItemNode resolveUseTreeListItem(Use use, UseTree tree) {
tree = use.getUseTree() and
result = resolvePath(tree.getPath())
result = resolvePathFull(tree.getPath())
or
result = resolveUseTreeListItem(use, tree, tree.getPath())
}
Expand Down
15 changes: 15 additions & 0 deletions rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,15 @@ edges
| main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | main.rs:41:13:41:13 | w [Wrapper] | provenance | |
| main.rs:41:30:41:39 | source(...) | main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | provenance | |
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | provenance | |
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:45:17:45:17 | w [Wrapper] | provenance | |
| main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | main.rs:43:26:43:26 | n | provenance | |
| main.rs:43:26:43:26 | n | main.rs:43:38:43:38 | n | provenance | |
| main.rs:45:13:45:13 | u [Wrapper] | main.rs:46:15:46:15 | u [Wrapper] | provenance | |
| main.rs:45:17:45:17 | w [Wrapper] | main.rs:45:17:45:25 | w.clone() [Wrapper] | provenance | generated |
| main.rs:45:17:45:25 | w.clone() [Wrapper] | main.rs:45:13:45:13 | u [Wrapper] | provenance | |
| main.rs:46:15:46:15 | u [Wrapper] | main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | provenance | |
| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | main.rs:47:26:47:26 | n | provenance | |
| main.rs:47:26:47:26 | n | main.rs:47:38:47:38 | n | provenance | |
| main.rs:58:13:58:13 | b [Some] | main.rs:59:23:59:23 | b [Some] | provenance | |
| main.rs:58:17:58:32 | Some(...) [Some] | main.rs:58:13:58:13 | b [Some] | provenance | |
| main.rs:58:22:58:31 | source(...) | main.rs:58:17:58:32 | Some(...) [Some] | provenance | |
Expand Down Expand Up @@ -71,6 +78,13 @@ nodes
| main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] |
| main.rs:43:26:43:26 | n | semmle.label | n |
| main.rs:43:38:43:38 | n | semmle.label | n |
| main.rs:45:13:45:13 | u [Wrapper] | semmle.label | u [Wrapper] |
| main.rs:45:17:45:17 | w [Wrapper] | semmle.label | w [Wrapper] |
| main.rs:45:17:45:25 | w.clone() [Wrapper] | semmle.label | w.clone() [Wrapper] |
| main.rs:46:15:46:15 | u [Wrapper] | semmle.label | u [Wrapper] |
| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] |
| main.rs:47:26:47:26 | n | semmle.label | n |
| main.rs:47:38:47:38 | n | semmle.label | n |
| main.rs:58:13:58:13 | b [Some] | semmle.label | b [Some] |
| main.rs:58:17:58:32 | Some(...) [Some] | semmle.label | Some(...) [Some] |
| main.rs:58:22:58:31 | source(...) | semmle.label | source(...) |
Expand All @@ -95,5 +109,6 @@ testFailures
| main.rs:22:10:22:19 | b.unwrap() | main.rs:19:34:19:43 | source(...) | main.rs:22:10:22:19 | b.unwrap() | $@ | main.rs:19:34:19:43 | source(...) | source(...) |
| main.rs:27:10:27:10 | a | main.rs:26:13:26:22 | source(...) | main.rs:27:10:27:10 | a | $@ | main.rs:26:13:26:22 | source(...) | source(...) |
| main.rs:43:38:43:38 | n | main.rs:41:30:41:39 | source(...) | main.rs:43:38:43:38 | n | $@ | main.rs:41:30:41:39 | source(...) | source(...) |
| main.rs:47:38:47:38 | n | main.rs:41:30:41:39 | source(...) | main.rs:47:38:47:38 | n | $@ | main.rs:41:30:41:39 | source(...) | source(...) |
| main.rs:63:22:63:22 | m | main.rs:58:22:58:31 | source(...) | main.rs:63:22:63:22 | m | $@ | main.rs:58:22:58:31 | source(...) | source(...) |
| main.rs:85:18:85:34 | ...::read(...) | main.rs:84:32:84:41 | source(...) | main.rs:85:18:85:34 | ...::read(...) | $@ | main.rs:84:32:84:41 | source(...) | source(...) |
2 changes: 1 addition & 1 deletion rust/ql/test/library-tests/dataflow/modeled/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ mod my_clone {
}
let u = w.clone();
match u {
Wrapper { n: n } => sink(n), // $ MISSING: hasValueFlow=73 - lack of expanded derives means that we cannot resolve clone call above, and hence not insert implicit borrow
Wrapper { n: n } => sink(n), // $ hasValueFlow=73
}
}
}
Expand Down
41 changes: 41 additions & 0 deletions rust/ql/test/query-tests/security/CWE-022/CONSISTENCY/PathResolutionConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
multiplePathResolutions
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |