Skip to content

Commit ae2fd52

Browse files
hvitvedhvitved
authored
Merge pull request #19431 from hvitved/rust/path-resolution-remove-visibility-check
Rust: Remove visibility check in path resolution
2 parents 07829e5 + 73fa381 commit ae2fd52

File tree

4 files changed

+87
-64
lines changed

4 files changed

+87
-64
lines changed

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

+30-63
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,6 @@ abstract class ItemNode extends Locatable {
8787
/** Gets the `i`th type parameter of this item, if any. */
8888
abstract TypeParam getTypeParam(int i);
8989

90-
/** Holds if this item is declared as `pub`. */
91-
bindingset[this]
92-
pragma[inline_late]
93-
predicate isPublic() { exists(this.getVisibility()) }
94-
9590
/** Gets an element that has this item as immediately enclosing item. */
9691
pragma[nomagic]
9792
Element getADescendant() {
@@ -207,6 +202,11 @@ abstract class ItemNode extends Locatable {
207202
result.(CrateItemNode).isPotentialDollarCrateTarget()
208203
}
209204

205+
pragma[nomagic]
206+
private predicate hasSourceFunction(string name) {
207+
this.getASuccessorFull(name).(Function).fromSource()
208+
}
209+
210210
/** Gets a successor named `name` of this item, if any. */
211211
pragma[nomagic]
212212
ItemNode getASuccessor(string name) {
@@ -219,7 +219,7 @@ abstract class ItemNode extends Locatable {
219219
or
220220
not result instanceof Function
221221
or
222-
not this.getASuccessorFull(name).(Function).fromSource()
222+
not this.hasSourceFunction(name)
223223
)
224224
}
225225

@@ -266,8 +266,6 @@ private class SourceFileItemNode extends ModuleLikeNode, SourceFile {
266266

267267
override Visibility getVisibility() { none() }
268268

269-
override predicate isPublic() { any() }
270-
271269
override TypeParam getTypeParam(int i) { none() }
272270
}
273271

@@ -330,8 +328,6 @@ class CrateItemNode extends ItemNode instanceof Crate {
330328

331329
override Visibility getVisibility() { none() }
332330

333-
override predicate isPublic() { any() }
334-
335331
override TypeParam getTypeParam(int i) { none() }
336332
}
337333

@@ -436,17 +432,17 @@ abstract class ImplOrTraitItemNode extends ItemNode {
436432

437433
pragma[nomagic]
438434
private TypeParamItemNode resolveTypeParamPathTypeRepr(PathTypeRepr ptr) {
439-
result = resolvePath(ptr.getPath())
435+
result = resolvePathFull(ptr.getPath())
440436
}
441437

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

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

447-
ItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) }
443+
ItemNode resolveSelfTy() { result = resolvePathFull(this.getSelfPath()) }
448444

449-
TraitItemNode resolveTraitTy() { result = resolvePath(this.getTraitPath()) }
445+
TraitItemNode resolveTraitTy() { result = resolvePathFull(this.getTraitPath()) }
450446

451447
pragma[nomagic]
452448
private TypeRepr getASelfTyArg() {
@@ -560,7 +556,7 @@ class TraitItemNode extends ImplOrTraitItemNode instanceof Trait {
560556
}
561557

562558
pragma[nomagic]
563-
ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) }
559+
ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) }
564560

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

@@ -634,7 +630,7 @@ class TypeParamItemNode extends ItemNode instanceof TypeParam {
634630
}
635631

636632
pragma[nomagic]
637-
ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) }
633+
ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) }
638634

639635
/**
640636
* Holds if this type parameter has a trait bound. Examples:
@@ -897,12 +893,6 @@ class RelevantPath extends Path {
897893
this.getQualifier().(RelevantPath).isCratePath("$crate", _) and
898894
this.getText() = name
899895
}
900-
901-
// TODO: Remove once the crate graph extractor generates publicly visible paths
902-
predicate requiresExtractorWorkaround() {
903-
not this.fromSource() and
904-
this = any(RelevantPath p).getQualifier()
905-
}
906896
}
907897

908898
private predicate isModule(ItemNode m) { m instanceof Module }
@@ -1056,8 +1046,14 @@ private predicate pathUsesNamespace(Path p, Namespace n) {
10561046
)
10571047
}
10581048

1049+
/**
1050+
* Gets the item that `path` resolves to, if any.
1051+
*
1052+
* Whenever `path` can resolve to both a function in source code and in library
1053+
* code, both are included
1054+
*/
10591055
pragma[nomagic]
1060-
private ItemNode resolvePath1(RelevantPath path) {
1056+
private ItemNode resolvePathFull(RelevantPath path) {
10611057
exists(Namespace ns | result = resolvePath0(path, ns) |
10621058
pathUsesNamespace(path, ns)
10631059
or
@@ -1067,58 +1063,29 @@ private ItemNode resolvePath1(RelevantPath path) {
10671063
}
10681064

10691065
pragma[nomagic]
1070-
private ItemNode resolvePathPrivate(
1071-
RelevantPath path, ModuleLikeNode itemParent, ModuleLikeNode pathParent
1072-
) {
1073-
not path.requiresExtractorWorkaround() and
1074-
result = resolvePath1(path) and
1075-
itemParent = result.getImmediateParentModule() and
1076-
not result.isPublic() and
1077-
(
1078-
pathParent.getADescendant() = path
1079-
or
1080-
pathParent = any(ItemNode mid | path = mid.getADescendant()).getImmediateParentModule()
1081-
)
1082-
}
1083-
1084-
pragma[nomagic]
1085-
private predicate isItemParent(ModuleLikeNode itemParent) {
1086-
exists(resolvePathPrivate(_, itemParent, _))
1087-
}
1088-
1089-
/**
1090-
* Gets a module that has access to private items defined inside `itemParent`.
1091-
*
1092-
* According to [The Rust Reference][1] this is either `itemParent` itself or any
1093-
* descendant of `itemParent`.
1094-
*
1095-
* [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access
1096-
*/
1097-
pragma[nomagic]
1098-
private ModuleLikeNode getAPrivateVisibleModule(ModuleLikeNode itemParent) {
1099-
isItemParent(itemParent) and
1100-
result.getImmediateParentModule*() = itemParent
1066+
private predicate resolvesSourceFunction(RelevantPath path) {
1067+
resolvePathFull(path).(Function).fromSource()
11011068
}
11021069

11031070
/** Gets the item that `path` resolves to, if any. */
11041071
cached
11051072
ItemNode resolvePath(RelevantPath path) {
1106-
result = resolvePath1(path) and
1073+
result = resolvePathFull(path) and
11071074
(
1108-
result.isPublic()
1075+
// when a function exists in both source code and in library code, it is because
1076+
// we also extracted the source code as library code, and hence we only want
1077+
// the function from source code
1078+
result.fromSource()
11091079
or
1110-
path.requiresExtractorWorkaround()
1111-
)
1112-
or
1113-
exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
1114-
result = resolvePathPrivate(path, itemParent, pathParent) and
1115-
pathParent = getAPrivateVisibleModule(itemParent)
1080+
not result instanceof Function
1081+
or
1082+
not resolvesSourceFunction(path)
11161083
)
11171084
}
11181085

11191086
pragma[nomagic]
11201087
private ItemNode resolvePathQualifier(RelevantPath path, string name) {
1121-
result = resolvePath(path.getQualifier()) and
1088+
result = resolvePathFull(path.getQualifier()) and
11221089
name = path.getText()
11231090
}
11241091

@@ -1164,7 +1131,7 @@ private ItemNode resolveUseTreeListItemQualifier(
11641131
pragma[nomagic]
11651132
private ItemNode resolveUseTreeListItem(Use use, UseTree tree) {
11661133
tree = use.getUseTree() and
1167-
result = resolvePath(tree.getPath())
1134+
result = resolvePathFull(tree.getPath())
11681135
or
11691136
result = resolveUseTreeListItem(use, tree, tree.getPath())
11701137
}

‎rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected

+15
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,15 @@ edges
2929
| main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | main.rs:41:13:41:13 | w [Wrapper] | provenance | |
3030
| main.rs:41:30:41:39 | source(...) | main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | provenance | |
3131
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | provenance | |
32+
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:45:17:45:17 | w [Wrapper] | provenance | |
3233
| main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | main.rs:43:26:43:26 | n | provenance | |
3334
| main.rs:43:26:43:26 | n | main.rs:43:38:43:38 | n | provenance | |
35+
| main.rs:45:13:45:13 | u [Wrapper] | main.rs:46:15:46:15 | u [Wrapper] | provenance | |
36+
| main.rs:45:17:45:17 | w [Wrapper] | main.rs:45:17:45:25 | w.clone() [Wrapper] | provenance | generated |
37+
| main.rs:45:17:45:25 | w.clone() [Wrapper] | main.rs:45:13:45:13 | u [Wrapper] | provenance | |
38+
| main.rs:46:15:46:15 | u [Wrapper] | main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | provenance | |
39+
| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | main.rs:47:26:47:26 | n | provenance | |
40+
| main.rs:47:26:47:26 | n | main.rs:47:38:47:38 | n | provenance | |
3441
| main.rs:58:13:58:13 | b [Some] | main.rs:59:23:59:23 | b [Some] | provenance | |
3542
| main.rs:58:17:58:32 | Some(...) [Some] | main.rs:58:13:58:13 | b [Some] | provenance | |
3643
| main.rs:58:22:58:31 | source(...) | main.rs:58:17:58:32 | Some(...) [Some] | provenance | |
@@ -71,6 +78,13 @@ nodes
7178
| main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] |
7279
| main.rs:43:26:43:26 | n | semmle.label | n |
7380
| main.rs:43:38:43:38 | n | semmle.label | n |
81+
| main.rs:45:13:45:13 | u [Wrapper] | semmle.label | u [Wrapper] |
82+
| main.rs:45:17:45:17 | w [Wrapper] | semmle.label | w [Wrapper] |
83+
| main.rs:45:17:45:25 | w.clone() [Wrapper] | semmle.label | w.clone() [Wrapper] |
84+
| main.rs:46:15:46:15 | u [Wrapper] | semmle.label | u [Wrapper] |
85+
| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] |
86+
| main.rs:47:26:47:26 | n | semmle.label | n |
87+
| main.rs:47:38:47:38 | n | semmle.label | n |
7488
| main.rs:58:13:58:13 | b [Some] | semmle.label | b [Some] |
7589
| main.rs:58:17:58:32 | Some(...) [Some] | semmle.label | Some(...) [Some] |
7690
| main.rs:58:22:58:31 | source(...) | semmle.label | source(...) |
@@ -95,5 +109,6 @@ testFailures
95109
| 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(...) |
96110
| 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(...) |
97111
| 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(...) |
112+
| 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(...) |
98113
| 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(...) |
99114
| 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(...) |

‎rust/ql/test/library-tests/dataflow/modeled/main.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ mod my_clone {
4444
}
4545
let u = w.clone();
4646
match u {
47-
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
47+
Wrapper { n: n } => sink(n), // $ hasValueFlow=73
4848
}
4949
}
5050
}

‎rust/ql/test/query-tests/security/CWE-022/CONSISTENCY/PathResolutionConsistency.expected

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
multiplePathResolutions
2+
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
3+
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
4+
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
5+
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
6+
| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from |
7+
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
8+
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
9+
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
10+
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
11+
| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from |
12+
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
13+
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
14+
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
15+
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
16+
| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from |
17+
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
18+
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
19+
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
20+
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
21+
| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from |
22+
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
23+
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
24+
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
25+
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
26+
| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from |
27+
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
28+
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
29+
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
30+
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
31+
| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from |
32+
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
33+
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
34+
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
35+
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
36+
| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from |
37+
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
38+
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
39+
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
40+
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |
41+
| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |

0 commit comments

Comments
 (0)