Skip to content

Commit aea83fa

Browse files
paldepindpaldepind
committed
Rust: Account for trait visibility when resolving paths and methods
1 parent 884b94a commit aea83fa

File tree

11 files changed

+41
-35
lines changed

11 files changed

+41
-35
lines changed

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

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ abstract class ItemNode extends Locatable {
216216
// items made available through `use` are available to nodes that contain the `use`
217217
exists(UseItemNode use |
218218
use = this.getASuccessor(_, _) and
219-
result = use.(ItemNode).getASuccessor(name, kind)
219+
result = use.getASuccessor(name, kind)
220220
)
221221
or
222222
exists(ExternCrateItemNode ec | result = ec.(ItemNode).getASuccessor(name, kind) |
@@ -1311,6 +1311,7 @@ private predicate declares(ItemNode item, Namespace ns, string name) {
13111311
class RelevantPath extends Path {
13121312
RelevantPath() { not this = any(VariableAccess va).(PathExpr).getPath() }
13131313

1314+
/** Holds if this is an unqualified path with the textual value `name`. */
13141315
pragma[nomagic]
13151316
predicate isUnqualified(string name) {
13161317
not exists(this.getQualifier()) and
@@ -1421,6 +1422,12 @@ private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns, SuccessorKi
14211422
pragma[nomagic]
14221423
private predicate isUnqualifiedSelfPath(RelevantPath path) { path.isUnqualified("Self") }
14231424

1425+
/** Holds if the trait `trait` is visible at the element `element`. */
1426+
bindingset[element, trait]
1427+
predicate traitIsVisible(Element element, TraitItemNode trait) {
1428+
exists(ItemNode encl | encl.getADescendant*() = element and trait = encl.getASuccessor(_, _))
1429+
}
1430+
14241431
pragma[nomagic]
14251432
private ItemNode resolvePathCand0(RelevantPath path, Namespace ns) {
14261433
exists(ItemNode res |
@@ -1454,6 +1461,15 @@ private ItemNode resolvePathCandQualified(
14541461
q = resolvePathCandQualifier(qualifier, path, name) and
14551462
result = getASuccessor(q, name, ns, kind) and
14561463
kind.isExternalOrBoth()
1464+
|
1465+
// When the result is an associated item of a trait implementation the
1466+
// implemented trait must be visible.
1467+
exists(ImplItemNodeImpl impl |
1468+
impl.getAnAssocItem() = result and
1469+
traitIsVisible(path, impl.resolveTraitTyCand())
1470+
)
1471+
or
1472+
not exists(ImplItemNode impl | impl.getAnAssocItem() = result and impl.(Impl).hasTrait())
14571473
)
14581474
}
14591475

@@ -1606,8 +1622,16 @@ private predicate useImportEdge(Use use, string name, ItemNode item, SuccessorKi
16061622
not tree.hasRename() and
16071623
name = item.getName()
16081624
or
1609-
name = tree.getRename().getName().getText() and
1610-
name != "_"
1625+
exists(Rename rename | rename = tree.getRename() |
1626+
name = rename.getName().getText()
1627+
or
1628+
// When the rename doesn't have a name it's an underscore import. This
1629+
// makes the imported item visible but unnameable. We represent this
1630+
// by using the name `_` which can never occur in a path. See also:
1631+
// https://doc.rust-lang.org/reference/items/use-declarations.html#r-items.use.as-underscore
1632+
not rename.hasName() and
1633+
name = "_"
1634+
)
16111635
)
16121636
)
16131637
)
@@ -1693,7 +1717,7 @@ private module Debug {
16931717
useImportEdge(use, name, item, kind)
16941718
}
16951719

1696-
ItemNode debuggetASuccessor(ItemNode i, string name, SuccessorKind kind) {
1720+
ItemNode debugGetASuccessor(ItemNode i, string name, SuccessorKind kind) {
16971721
i = getRelevantLocatable() and
16981722
result = i.getASuccessor(name, kind)
16991723
}

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1891,7 +1891,7 @@ private predicate methodCandidate(Type type, string name, int arity, Impl impl)
18911891
*/
18921892
pragma[nomagic]
18931893
private predicate methodCandidateTrait(Type type, Trait trait, string name, int arity, Impl impl) {
1894-
trait = resolvePath(impl.(ImplItemNode).getTraitPath()) and
1894+
trait = impl.(ImplItemNode).resolveTraitTy() and
18951895
methodCandidate(type, name, arity, impl)
18961896
}
18971897

@@ -1912,7 +1912,12 @@ private module IsInstantiationOfInput implements IsInstantiationOfInputSig<Metho
19121912
methodCandidateTrait(rootType, mc.getTrait(), name, arity, impl)
19131913
or
19141914
not exists(mc.getTrait()) and
1915-
methodCandidate(rootType, name, arity, impl)
1915+
methodCandidate(rootType, name, arity, impl) and
1916+
(
1917+
traitIsVisible(mc, impl.(ImplItemNode).resolveTraitTy())
1918+
or
1919+
not exists(impl.(ImplItemNode).resolveTraitTy())
1920+
)
19161921
)
19171922
}
19181923

‎rust/ql/test/library-tests/dataflow/sources/CONSISTENCY/PathResolutionConsistency.expected

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,8 @@ multipleCallTargets
1111
| test.rs:179:30:179:68 | ...::_print(...) |
1212
| test.rs:188:26:188:105 | ...::_print(...) |
1313
| test.rs:229:22:229:72 | ... .read_to_string(...) |
14-
| test.rs:513:22:513:50 | file.read_to_end(...) |
15-
| test.rs:519:22:519:53 | file.read_to_string(...) |
1614
| test.rs:697:18:697:38 | ...::_print(...) |
1715
| test.rs:702:18:702:45 | ...::_print(...) |
18-
| test.rs:706:25:706:49 | address.to_socket_addrs() |
1916
| test.rs:720:38:720:42 | ...::_print(...) |
2017
| test.rs:724:38:724:54 | ...::_print(...) |
2118
| test.rs:729:38:729:51 | ...::_print(...) |
@@ -76,12 +73,6 @@ multipleCallTargets
7673
| test.rs:977:14:977:29 | ...::_print(...) |
7774
| test.rs:979:27:979:36 | ...::_print(...) |
7875
| test.rs:980:28:980:41 | ...::_print(...) |
79-
| test_futures_io.rs:35:26:35:63 | pinned.poll_read(...) |
80-
| test_futures_io.rs:62:22:62:50 | pinned.poll_fill_buf(...) |
81-
| test_futures_io.rs:69:23:69:67 | ... .poll_fill_buf(...) |
82-
| test_futures_io.rs:93:26:93:63 | pinned.poll_read(...) |
83-
| test_futures_io.rs:116:22:116:50 | pinned.poll_fill_buf(...) |
84-
| test_futures_io.rs:145:26:145:49 | ...::with_capacity(...) |
8576
| web_frameworks.rs:13:14:13:22 | a.as_str() |
8677
| web_frameworks.rs:13:14:13:23 | a.as_str() |
8778
| web_frameworks.rs:14:14:14:24 | a.as_bytes() |

‎rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
11
multipleCallTargets
22
| main.rs:118:9:118:11 | f(...) |
3-
| main.rs:494:13:494:27 | ...::a_method(...) |
4-
| main.rs:498:13:498:27 | ...::a_method(...) |
5-
| main.rs:502:13:502:27 | ...::a_method(...) |
63
| proc_macro.rs:9:5:9:10 | ...::new(...) |

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -491,15 +491,15 @@ mod trait_visibility {
491491
let x = X; // $ item=X
492492
{
493493
use m::Foo; // $ item=Foo
494-
X::a_method(&x); // $ item=X_Foo::a_method SPURIOUS: item=X_Bar::a_method
494+
X::a_method(&x); // $ item=X_Foo::a_method
495495
}
496496
{
497497
use m::Bar; // $ item=Bar
498-
X::a_method(&x); // $ item=X_Bar::a_method SPURIOUS: item=X_Foo::a_method
498+
X::a_method(&x); // $ item=X_Bar::a_method
499499
}
500500
{
501501
use m::Bar as _; // $ item=Bar
502-
X::a_method(&x); // $ item=X_Bar::a_method SPURIOUS: item=X_Foo::a_method
502+
X::a_method(&x); // $ item=X_Bar::a_method
503503
}
504504
} // trait_visibility::f
505505
}

‎rust/ql/test/library-tests/path-resolution/path-resolution.expected

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -236,16 +236,13 @@ resolvePath
236236
| main.rs:493:17:493:22 | ...::Foo | main.rs:465:9:467:9 | trait Foo |
237237
| main.rs:494:13:494:13 | X | main.rs:473:9:473:21 | struct X |
238238
| main.rs:494:13:494:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
239-
| main.rs:494:13:494:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
240239
| main.rs:497:17:497:17 | m | main.rs:464:5:486:5 | mod m |
241240
| main.rs:497:17:497:22 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
242241
| main.rs:498:13:498:13 | X | main.rs:473:9:473:21 | struct X |
243-
| main.rs:498:13:498:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
244242
| main.rs:498:13:498:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
245243
| main.rs:501:17:501:17 | m | main.rs:464:5:486:5 | mod m |
246244
| main.rs:501:17:501:22 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
247245
| main.rs:502:13:502:13 | X | main.rs:473:9:473:21 | struct X |
248-
| main.rs:502:13:502:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
249246
| main.rs:502:13:502:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
250247
| main.rs:515:10:515:16 | MyTrait | main.rs:508:5:510:5 | trait MyTrait |
251248
| main.rs:516:9:516:9 | S | main.rs:512:5:512:13 | struct S |

‎rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
11
multipleCallTargets
22
| dereference.rs:61:15:61:24 | e1.deref() |
3-
| main.rs:153:13:153:24 | x.a_method() |
4-
| main.rs:157:13:157:24 | x.a_method() |
5-
| main.rs:161:13:161:24 | x.a_method() |
63
| main.rs:2357:13:2357:31 | ...::from(...) |
74
| main.rs:2358:13:2358:31 | ...::from(...) |
85
| main.rs:2359:13:2359:31 | ...::from(...) |

‎rust/ql/test/library-tests/type-inference/main.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -150,15 +150,15 @@ mod trait_visibility {
150150
let x = X;
151151
{
152152
use m::Foo;
153-
x.a_method(); // $ target=Foo::a_method SPURIOUS: target=Bar::a_method
153+
x.a_method(); // $ target=Foo::a_method
154154
}
155155
{
156156
use m::Bar;
157-
x.a_method(); // $ target=Bar::a_method SPURIOUS: target=Foo::a_method
157+
x.a_method(); // $ target=Bar::a_method
158158
}
159159
{
160160
use m::Bar as _;
161-
x.a_method(); // $ target=Bar::a_method SPURIOUS: target=Foo::a_method
161+
x.a_method(); // $ target=Bar::a_method
162162
}
163163
{
164164
use m::Bar;

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,2 @@
11
multipleCallTargets
22
| main.rs:9:43:9:63 | ...::from(...) |
3-
| main.rs:44:19:44:32 | username.len() |

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ multipleCallTargets
7272
| test_logging.rs:195:15:195:38 | ...::_eprint(...) |
7373
| test_logging.rs:229:30:229:71 | ... .as_str() |
7474
| test_logging.rs:242:16:242:61 | ... .as_bytes() |
75-
| test_logging.rs:243:5:245:66 | ... .write_all(...) |
7675
| test_logging.rs:245:20:245:65 | ... .as_bytes() |
7776
| test_logging.rs:248:15:248:60 | ... .as_bytes() |
7877
| test_logging.rs:251:15:251:60 | ... .as_bytes() |

0 commit comments

Comments
 (0)