Skip to content

Commit edc38da

Browse files
hvitvedhvitved
committed
Rust: Remove visibility check in path resolution
1 parent 302680c commit edc38da

File tree

3 files changed

+66
-65
lines changed

3 files changed

+66
-65
lines changed

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

Lines changed: 22 additions & 65 deletions
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() {
@@ -266,8 +261,6 @@ private class SourceFileItemNode extends ModuleLikeNode, SourceFile {
266261

267262
override Visibility getVisibility() { none() }
268263

269-
override predicate isPublic() { any() }
270-
271264
override TypeParam getTypeParam(int i) { none() }
272265
}
273266

@@ -330,8 +323,6 @@ class CrateItemNode extends ItemNode instanceof Crate {
330323

331324
override Visibility getVisibility() { none() }
332325

333-
override predicate isPublic() { any() }
334-
335326
override TypeParam getTypeParam(int i) { none() }
336327
}
337328

@@ -436,17 +427,17 @@ abstract class ImplOrTraitItemNode extends ItemNode {
436427

437428
pragma[nomagic]
438429
private TypeParamItemNode resolveTypeParamPathTypeRepr(PathTypeRepr ptr) {
439-
result = resolvePath(ptr.getPath())
430+
result = resolvePathFull(ptr.getPath())
440431
}
441432

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

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

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

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

451442
pragma[nomagic]
452443
private TypeRepr getASelfTyArg() {
@@ -560,7 +551,7 @@ class TraitItemNode extends ImplOrTraitItemNode instanceof Trait {
560551
}
561552

562553
pragma[nomagic]
563-
ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) }
554+
ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) }
564555

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

@@ -634,7 +625,7 @@ class TypeParamItemNode extends ItemNode instanceof TypeParam {
634625
}
635626

636627
pragma[nomagic]
637-
ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) }
628+
ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) }
638629

639630
/**
640631
* Holds if this type parameter has a trait bound. Examples:
@@ -897,12 +888,6 @@ class RelevantPath extends Path {
897888
this.getQualifier().(RelevantPath).isCratePath("$crate", _) and
898889
this.getText() = name
899890
}
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-
}
906891
}
907892

908893
private predicate isModule(ItemNode m) { m instanceof Module }
@@ -1056,8 +1041,14 @@ private predicate pathUsesNamespace(Path p, Namespace n) {
10561041
)
10571042
}
10581043

1044+
/**
1045+
* Gets the item that `path` resolves to, if any.
1046+
*
1047+
* Whenever `path` can resolve to both a function in source code and in library
1048+
* code, both are included
1049+
*/
10591050
pragma[nomagic]
1060-
private ItemNode resolvePath1(RelevantPath path) {
1051+
private ItemNode resolvePathFull(RelevantPath path) {
10611052
exists(Namespace ns | result = resolvePath0(path, ns) |
10621053
pathUsesNamespace(path, ns)
10631054
or
@@ -1066,59 +1057,25 @@ private ItemNode resolvePath1(RelevantPath path) {
10661057
)
10671058
}
10681059

1069-
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
1101-
}
1102-
11031060
/** Gets the item that `path` resolves to, if any. */
11041061
cached
11051062
ItemNode resolvePath(RelevantPath path) {
1106-
result = resolvePath1(path) and
1063+
result = resolvePathFull(path) and
11071064
(
1108-
result.isPublic()
1065+
// when a function exists in both source code and in library code, it is because
1066+
// we also extracted the source code as library code, and hence we only want
1067+
// the function from source code
1068+
result.fromSource()
11091069
or
1110-
path.requiresExtractorWorkaround()
1111-
)
1112-
or
1113-
exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
1114-
result = resolvePathPrivate(path, itemParent, pathParent) and
1115-
pathParent = getAPrivateVisibleModule(itemParent)
1070+
not result instanceof Function
1071+
or
1072+
not resolvePathFull(path).(Function).fromSource()
11161073
)
11171074
}
11181075

11191076
pragma[nomagic]
11201077
private ItemNode resolvePathQualifier(RelevantPath path, string name) {
1121-
result = resolvePath(path.getQualifier()) and
1078+
result = resolvePathFull(path.getQualifier()) and
11221079
name = path.getText()
11231080
}
11241081

@@ -1164,7 +1121,7 @@ private ItemNode resolveUseTreeListItemQualifier(
11641121
pragma[nomagic]
11651122
private ItemNode resolveUseTreeListItem(Use use, UseTree tree) {
11661123
tree = use.getUseTree() and
1167-
result = resolvePath(tree.getPath())
1124+
result = resolvePathFull(tree.getPath())
11681125
or
11691126
result = resolveUseTreeListItem(use, tree, tree.getPath())
11701127
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,12 @@ edges
3434
| main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | main.rs:41:13:41:13 | w [Wrapper] | provenance | |
3535
| main.rs:41:30:41:39 | source(...) | main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | provenance | |
3636
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | provenance | |
37+
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:45:17:45:17 | w [Wrapper] | provenance | |
3738
| main.rs:42:15:42:15 | w [Wrapper] | main.rs:45:17:45:25 | w.clone() [Wrapper] | provenance | generated |
3839
| main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | main.rs:43:26:43:26 | n | provenance | |
3940
| main.rs:43:26:43:26 | n | main.rs:43:38:43:38 | n | provenance | |
4041
| main.rs:45:13:45:13 | u [Wrapper] | main.rs:46:15:46:15 | u [Wrapper] | provenance | |
42+
| main.rs:45:17:45:17 | w [Wrapper] | main.rs:45:17:45:25 | w.clone() [Wrapper] | provenance | generated |
4143
| main.rs:45:17:45:25 | w.clone() [Wrapper] | main.rs:45:13:45:13 | u [Wrapper] | provenance | |
4244
| main.rs:46:15:46:15 | u [Wrapper] | main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | provenance | |
4345
| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | main.rs:47:26:47:26 | n | provenance | |
@@ -86,6 +88,7 @@ nodes
8688
| main.rs:43:26:43:26 | n | semmle.label | n |
8789
| main.rs:43:38:43:38 | n | semmle.label | n |
8890
| main.rs:45:13:45:13 | u [Wrapper] | semmle.label | u [Wrapper] |
91+
| main.rs:45:17:45:17 | w [Wrapper] | semmle.label | w [Wrapper] |
8992
| main.rs:45:17:45:25 | w.clone() [Wrapper] | semmle.label | w.clone() [Wrapper] |
9093
| main.rs:46:15:46:15 | u [Wrapper] | semmle.label | u [Wrapper] |
9194
| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] |

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

Lines changed: 41 additions & 0 deletions
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)