Skip to content

Commit bab84d0

Browse files
paldepindpaldepind
authored
Merge pull request #19419 from paldepind/rust-precise-implicit-deref-borrow
Rust: Use type inference to insert implicit borrows and derefs
2 parents 359aa02 + c263d3f commit bab84d0

File tree

5 files changed

+27
-94
lines changed

5 files changed

+27
-94
lines changed

‎rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+3-19
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ private import rust
1111
private import SsaImpl as SsaImpl
1212
private import codeql.rust.controlflow.internal.Scope as Scope
1313
private import codeql.rust.internal.PathResolution
14+
private import codeql.rust.internal.TypeInference as TypeInference
1415
private import codeql.rust.controlflow.ControlFlowGraph
1516
private import codeql.rust.controlflow.CfgNodes
1617
private import codeql.rust.dataflow.Ssa
@@ -321,23 +322,6 @@ predicate lambdaCallExpr(CallExprCfgNode call, LambdaCallKind kind, ExprCfgNode
321322
exists(kind)
322323
}
323324

324-
/** Holds if `mc` implicitly borrows its receiver. */
325-
private predicate implicitBorrow(MethodCallExpr mc) {
326-
// Determining whether an implicit borrow happens depends on the type of the
327-
// receiever as well as the target. As a heuristic we simply check if the
328-
// target takes `self` as a borrow and limit the approximation to cases where
329-
// the receiver is a simple variable.
330-
mc.getReceiver() instanceof VariableAccess and
331-
mc.getStaticTarget().getParamList().getSelfParam().isRef()
332-
}
333-
334-
/** Holds if `mc` implicitly dereferences its receiver. */
335-
private predicate implicitDeref(MethodCallExpr mc) {
336-
// Similarly to `implicitBorrow` this is an approximation.
337-
mc.getReceiver() instanceof VariableAccess and
338-
not mc.getStaticTarget().getParamList().getSelfParam().isRef()
339-
}
340-
341325
// Defines a set of aliases needed for the `RustDataFlow` module
342326
private module Aliases {
343327
class DataFlowCallableAlias = DataFlowCallable;
@@ -520,15 +504,15 @@ module RustDataFlow implements InputSig<Location> {
520504

521505
pragma[nomagic]
522506
private predicate implicitDerefToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) {
507+
TypeInference::receiverHasImplicitDeref(node1.asExpr().getExpr()) and
523508
node1.asExpr() = node2.getReceiver() and
524-
implicitDeref(node2.getMethodCall().getMethodCallExpr()) and
525509
exists(c)
526510
}
527511

528512
pragma[nomagic]
529513
private predicate implicitBorrowToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) {
514+
TypeInference::receiverHasImplicitBorrow(node1.asExpr().getExpr()) and
530515
node1.asExpr() = node2.getReceiver() and
531-
implicitBorrow(node2.getMethodCall().getMethodCallExpr()) and
532516
exists(c)
533517
}
534518

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

+24-20
Original file line numberDiff line numberDiff line change
@@ -662,15 +662,6 @@ private module CallExprBaseMatchingInput implements MatchingInputSig {
662662
tAdj = t
663663
)
664664
}
665-
666-
pragma[nomagic]
667-
additional Type inferReceiverType(AstNode n) {
668-
exists(Access a, AccessPosition apos |
669-
result = inferType(n) and
670-
n = a.getNodeAt(apos) and
671-
apos.isSelf()
672-
)
673-
}
674665
}
675666

676667
private module CallExprBaseMatching = Matching<CallExprBaseMatchingInput>;
@@ -690,7 +681,7 @@ private Type inferCallExprBaseType(AstNode n, TypePath path) {
690681
|
691682
if apos.isSelf()
692683
then
693-
exists(Type receiverType | receiverType = CallExprBaseMatchingInput::inferReceiverType(n) |
684+
exists(Type receiverType | receiverType = inferType(n) |
694685
if receiverType = TRefType()
695686
then
696687
path = path0 and
@@ -813,15 +804,6 @@ private module FieldExprMatchingInput implements MatchingInputSig {
813804
tAdj = t
814805
)
815806
}
816-
817-
pragma[nomagic]
818-
additional Type inferReceiverType(AstNode n) {
819-
exists(Access a, AccessPosition apos |
820-
result = inferType(n) and
821-
n = a.getNodeAt(apos) and
822-
apos.isSelf()
823-
)
824-
}
825807
}
826808

827809
private module FieldExprMatching = Matching<FieldExprMatchingInput>;
@@ -840,7 +822,7 @@ private Type inferFieldExprType(AstNode n, TypePath path) {
840822
|
841823
if apos.isSelf()
842824
then
843-
exists(Type receiverType | receiverType = FieldExprMatchingInput::inferReceiverType(n) |
825+
exists(Type receiverType | receiverType = inferType(n) |
844826
if receiverType = TRefType()
845827
then
846828
// adjust for implicit deref
@@ -895,6 +877,28 @@ cached
895877
private module Cached {
896878
private import codeql.rust.internal.CachedStages
897879

880+
/** Holds if `receiver` is the receiver of a method call with an implicit dereference. */
881+
cached
882+
predicate receiverHasImplicitDeref(AstNode receiver) {
883+
exists(CallExprBaseMatchingInput::Access a, CallExprBaseMatchingInput::AccessPosition apos |
884+
apos.isSelf() and
885+
receiver = a.getNodeAt(apos) and
886+
inferType(receiver) = TRefType() and
887+
CallExprBaseMatching::inferAccessType(a, apos, TypePath::nil()) != TRefType()
888+
)
889+
}
890+
891+
/** Holds if `receiver` is the receiver of a method call with an implicit borrow. */
892+
cached
893+
predicate receiverHasImplicitBorrow(AstNode receiver) {
894+
exists(CallExprBaseMatchingInput::Access a, CallExprBaseMatchingInput::AccessPosition apos |
895+
apos.isSelf() and
896+
receiver = a.getNodeAt(apos) and
897+
CallExprBaseMatching::inferAccessType(a, apos, TypePath::nil()) = TRefType() and
898+
inferType(receiver) != TRefType()
899+
)
900+
}
901+
898902
pragma[inline]
899903
private Type getLookupType(AstNode n) {
900904
exists(Type t |

‎rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected

-35
Original file line numberDiff line numberDiff line change
@@ -1931,33 +1931,16 @@ readStep
19311931
| main.rs:221:9:221:23 | ...::Some(...) | Some | main.rs:221:22:221:22 | n |
19321932
| main.rs:230:9:230:15 | Some(...) | Some | main.rs:230:14:230:14 | n |
19331933
| main.rs:234:9:234:15 | Some(...) | Some | main.rs:234:14:234:14 | n |
1934-
| main.rs:241:10:241:11 | s1 | &ref | main.rs:241:10:241:11 | receiver for s1 |
1935-
| main.rs:246:10:246:11 | s1 | &ref | main.rs:246:10:246:11 | receiver for s1 |
1936-
| main.rs:249:10:249:11 | s2 | &ref | main.rs:249:10:249:11 | receiver for s2 |
1937-
| main.rs:254:10:254:11 | s1 | &ref | main.rs:254:10:254:11 | receiver for s1 |
1938-
| main.rs:257:10:257:11 | s2 | &ref | main.rs:257:10:257:11 | receiver for s2 |
19391934
| main.rs:263:14:263:15 | s1 | Ok | main.rs:263:14:263:16 | TryExpr |
19401935
| main.rs:263:14:263:15 | s1 | Some | main.rs:263:14:263:16 | TryExpr |
19411936
| main.rs:265:10:265:11 | s2 | Ok | main.rs:265:10:265:12 | TryExpr |
19421937
| main.rs:265:10:265:11 | s2 | Some | main.rs:265:10:265:12 | TryExpr |
1943-
| main.rs:271:29:271:30 | r1 | &ref | main.rs:271:29:271:30 | receiver for r1 |
1944-
| main.rs:272:29:272:30 | r1 | &ref | main.rs:272:29:272:30 | receiver for r1 |
1945-
| main.rs:273:10:273:12 | o1a | &ref | main.rs:273:10:273:12 | receiver for o1a |
1946-
| main.rs:274:10:274:12 | o1b | &ref | main.rs:274:10:274:12 | receiver for o1b |
1947-
| main.rs:277:29:277:30 | r2 | &ref | main.rs:277:29:277:30 | receiver for r2 |
1948-
| main.rs:278:29:278:30 | r2 | &ref | main.rs:278:29:278:30 | receiver for r2 |
1949-
| main.rs:279:10:279:12 | o2a | &ref | main.rs:279:10:279:12 | receiver for o2a |
1950-
| main.rs:280:10:280:12 | o2b | &ref | main.rs:280:10:280:12 | receiver for o2b |
19511938
| main.rs:287:14:287:15 | s1 | Ok | main.rs:287:14:287:16 | TryExpr |
19521939
| main.rs:287:14:287:15 | s1 | Some | main.rs:287:14:287:16 | TryExpr |
19531940
| main.rs:288:14:288:15 | s2 | Ok | main.rs:288:14:288:16 | TryExpr |
19541941
| main.rs:288:14:288:15 | s2 | Some | main.rs:288:14:288:16 | TryExpr |
19551942
| main.rs:291:14:291:15 | s3 | Ok | main.rs:291:14:291:16 | TryExpr |
19561943
| main.rs:291:14:291:15 | s3 | Some | main.rs:291:14:291:16 | TryExpr |
1957-
| main.rs:298:10:298:11 | s1 | &ref | main.rs:298:10:298:11 | receiver for s1 |
1958-
| main.rs:299:10:299:11 | s1 | &ref | main.rs:299:10:299:11 | receiver for s1 |
1959-
| main.rs:302:10:302:11 | s2 | &ref | main.rs:302:10:302:11 | receiver for s2 |
1960-
| main.rs:303:10:303:11 | s2 | &ref | main.rs:303:10:303:11 | receiver for s2 |
19611944
| main.rs:315:9:315:25 | ...::A(...) | A | main.rs:315:24:315:24 | n |
19621945
| main.rs:316:9:316:25 | ...::B(...) | B | main.rs:316:24:316:24 | n |
19631946
| main.rs:319:9:319:25 | ...::A(...) | A | main.rs:319:24:319:24 | n |
@@ -1997,40 +1980,22 @@ readStep
19971980
| main.rs:442:9:442:20 | TuplePat | tuple.0 | main.rs:442:10:442:13 | cond |
19981981
| main.rs:442:9:442:20 | TuplePat | tuple.1 | main.rs:442:16:442:19 | name |
19991982
| main.rs:442:25:442:29 | names | element | main.rs:442:9:442:20 | TuplePat |
2000-
| main.rs:444:21:444:24 | name | &ref | main.rs:444:21:444:24 | receiver for name |
20011983
| main.rs:444:41:444:67 | [post] \|...\| ... | captured default_name | main.rs:444:41:444:67 | [post] default_name |
2002-
| main.rs:444:44:444:55 | default_name | &ref | main.rs:444:44:444:55 | receiver for default_name |
20031984
| main.rs:444:44:444:55 | this | captured default_name | main.rs:444:44:444:55 | default_name |
2004-
| main.rs:445:18:445:18 | n | &ref | main.rs:445:18:445:18 | receiver for n |
2005-
| main.rs:468:13:468:13 | a | &ref | main.rs:468:13:468:13 | receiver for a |
2006-
| main.rs:469:13:469:13 | b | &ref | main.rs:469:13:469:13 | receiver for b |
2007-
| main.rs:470:19:470:19 | b | &ref | main.rs:470:19:470:19 | receiver for b |
20081985
| main.rs:481:10:481:11 | vs | element | main.rs:481:10:481:14 | vs[0] |
2009-
| main.rs:482:11:482:12 | vs | &ref | main.rs:482:11:482:12 | receiver for vs |
20101986
| main.rs:482:11:482:35 | ... .unwrap() | &ref | main.rs:482:10:482:35 | * ... |
2011-
| main.rs:483:11:483:12 | vs | &ref | main.rs:483:11:483:12 | receiver for vs |
20121987
| main.rs:483:11:483:35 | ... .unwrap() | &ref | main.rs:483:10:483:35 | * ... |
20131988
| main.rs:485:14:485:15 | vs | element | main.rs:485:9:485:9 | v |
20141989
| main.rs:488:9:488:10 | &... | &ref | main.rs:488:10:488:10 | v |
2015-
| main.rs:488:15:488:16 | vs | &ref | main.rs:488:15:488:16 | receiver for vs |
20161990
| main.rs:488:15:488:23 | vs.iter() | element | main.rs:488:9:488:10 | &... |
2017-
| main.rs:492:27:492:28 | vs | &ref | main.rs:492:27:492:28 | receiver for vs |
20181991
| main.rs:493:9:493:10 | &... | &ref | main.rs:493:10:493:10 | v |
20191992
| main.rs:493:15:493:17 | vs2 | element | main.rs:493:9:493:10 | &... |
2020-
| main.rs:497:5:497:6 | vs | &ref | main.rs:497:5:497:6 | receiver for vs |
20211993
| main.rs:497:29:497:29 | x | &ref | main.rs:497:28:497:29 | * ... |
2022-
| main.rs:498:5:498:6 | vs | &ref | main.rs:498:5:498:6 | receiver for vs |
20231994
| main.rs:498:34:498:34 | x | &ref | main.rs:498:33:498:34 | * ... |
2024-
| main.rs:500:14:500:15 | vs | &ref | main.rs:500:14:500:15 | receiver for vs |
20251995
| main.rs:500:14:500:27 | vs.into_iter() | element | main.rs:500:9:500:9 | v |
20261996
| main.rs:506:10:506:15 | vs_mut | element | main.rs:506:10:506:18 | vs_mut[0] |
2027-
| main.rs:507:11:507:16 | vs_mut | &ref | main.rs:507:11:507:16 | receiver for vs_mut |
20281997
| main.rs:507:11:507:39 | ... .unwrap() | &ref | main.rs:507:10:507:39 | * ... |
2029-
| main.rs:508:11:508:16 | vs_mut | &ref | main.rs:508:11:508:16 | receiver for vs_mut |
20301998
| main.rs:508:11:508:39 | ... .unwrap() | &ref | main.rs:508:10:508:39 | * ... |
20311999
| main.rs:510:9:510:14 | &mut ... | &ref | main.rs:510:14:510:14 | v |
2032-
| main.rs:510:19:510:24 | vs_mut | &ref | main.rs:510:19:510:24 | receiver for vs_mut |
20332000
| main.rs:510:19:510:35 | vs_mut.iter_mut() | element | main.rs:510:9:510:14 | &mut ... |
20342001
| main.rs:524:11:524:15 | c_ref | &ref | main.rs:524:10:524:15 | * ... |
2035-
| main.rs:531:10:531:10 | a | &ref | main.rs:531:10:531:10 | receiver for a |
2036-
| main.rs:537:10:537:10 | b | &ref | main.rs:537:10:537:10 | receiver for b |

‎rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected

-12
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,11 @@ edges
3131
| sqlx.rs:52:32:52:87 | ...::must_use(...) | sqlx.rs:52:9:52:20 | safe_query_3 | provenance | |
3232
| sqlx.rs:52:32:52:87 | MacroExpr | sqlx.rs:52:32:52:87 | ...::format(...) | provenance | MaD:4 |
3333
| sqlx.rs:52:32:52:87 | { ... } | sqlx.rs:52:32:52:87 | ...::must_use(...) | provenance | MaD:9 |
34-
| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:63:26:63:39 | unsafe_query_1 [&ref] | provenance | |
3534
| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:63:26:63:48 | unsafe_query_1.as_str() | provenance | MaD:3 |
36-
| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:74:25:74:38 | unsafe_query_1 [&ref] | provenance | |
3735
| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:74:25:74:47 | unsafe_query_1.as_str() | provenance | MaD:3 |
3836
| sqlx.rs:53:26:53:36 | &arg_string [&ref] | sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | provenance | |
3937
| sqlx.rs:53:27:53:36 | arg_string | sqlx.rs:53:26:53:36 | &arg_string [&ref] | provenance | |
40-
| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:65:30:65:43 | unsafe_query_2 [&ref] | provenance | |
4138
| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:65:30:65:52 | unsafe_query_2.as_str() | provenance | MaD:3 |
42-
| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:76:29:76:42 | unsafe_query_2 [&ref] | provenance | |
4339
| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:76:29:76:51 | unsafe_query_2.as_str() | provenance | MaD:3 |
4440
| sqlx.rs:54:26:54:39 | &remote_string [&ref] | sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | provenance | |
4541
| sqlx.rs:54:27:54:39 | remote_string | sqlx.rs:54:26:54:39 | &remote_string [&ref] | provenance | |
@@ -50,10 +46,6 @@ edges
5046
| sqlx.rs:56:34:56:89 | ...::must_use(...) | sqlx.rs:56:9:56:22 | unsafe_query_4 | provenance | |
5147
| sqlx.rs:56:34:56:89 | MacroExpr | sqlx.rs:56:34:56:89 | ...::format(...) | provenance | MaD:4 |
5248
| sqlx.rs:56:34:56:89 | { ... } | sqlx.rs:56:34:56:89 | ...::must_use(...) | provenance | MaD:9 |
53-
| sqlx.rs:63:26:63:39 | unsafe_query_1 [&ref] | sqlx.rs:63:26:63:48 | unsafe_query_1.as_str() | provenance | MaD:3 |
54-
| sqlx.rs:65:30:65:43 | unsafe_query_2 [&ref] | sqlx.rs:65:30:65:52 | unsafe_query_2.as_str() | provenance | MaD:3 |
55-
| sqlx.rs:74:25:74:38 | unsafe_query_1 [&ref] | sqlx.rs:74:25:74:47 | unsafe_query_1.as_str() | provenance | MaD:3 |
56-
| sqlx.rs:76:29:76:42 | unsafe_query_2 [&ref] | sqlx.rs:76:29:76:51 | unsafe_query_2.as_str() | provenance | MaD:3 |
5749
models
5850
| 1 | Source: lang:std; crate::env::args; commandargs; ReturnValue.Element |
5951
| 2 | Source: repo:https://github.com/seanmonstar/reqwest:reqwest; crate::blocking::get; remote; ReturnValue.Field[crate::result::Result::Ok(0)] |
@@ -100,15 +92,11 @@ nodes
10092
| sqlx.rs:56:34:56:89 | MacroExpr | semmle.label | MacroExpr |
10193
| sqlx.rs:56:34:56:89 | { ... } | semmle.label | { ... } |
10294
| sqlx.rs:62:26:62:46 | safe_query_3.as_str() | semmle.label | safe_query_3.as_str() |
103-
| sqlx.rs:63:26:63:39 | unsafe_query_1 [&ref] | semmle.label | unsafe_query_1 [&ref] |
10495
| sqlx.rs:63:26:63:48 | unsafe_query_1.as_str() | semmle.label | unsafe_query_1.as_str() |
105-
| sqlx.rs:65:30:65:43 | unsafe_query_2 [&ref] | semmle.label | unsafe_query_2 [&ref] |
10696
| sqlx.rs:65:30:65:52 | unsafe_query_2.as_str() | semmle.label | unsafe_query_2.as_str() |
10797
| sqlx.rs:67:30:67:52 | unsafe_query_4.as_str() | semmle.label | unsafe_query_4.as_str() |
10898
| sqlx.rs:73:25:73:45 | safe_query_3.as_str() | semmle.label | safe_query_3.as_str() |
109-
| sqlx.rs:74:25:74:38 | unsafe_query_1 [&ref] | semmle.label | unsafe_query_1 [&ref] |
11099
| sqlx.rs:74:25:74:47 | unsafe_query_1.as_str() | semmle.label | unsafe_query_1.as_str() |
111-
| sqlx.rs:76:29:76:42 | unsafe_query_2 [&ref] | semmle.label | unsafe_query_2 [&ref] |
112100
| sqlx.rs:76:29:76:51 | unsafe_query_2.as_str() | semmle.label | unsafe_query_2.as_str() |
113101
| sqlx.rs:78:29:78:51 | unsafe_query_4.as_str() | semmle.label | unsafe_query_4.as_str() |
114102
subpaths

‎rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected

-8
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,4 @@
11
unexpectedModel
22
| Unexpected summary found: repo::test;<crate::option::MyOption as crate::clone::Clone>::clone;Argument[self].Field[crate::option::MyOption::MySome(0)].Reference;ReturnValue.Field[crate::option::MyOption::MySome(0)];value;dfc-generated |
3-
| Unexpected summary found: repo::test;<crate::option::MyOption as crate::convert::From>::from;Argument[0].Field[crate::option::MyOption::MySome(0)];ReturnValue.Field[crate::option::MyOption::MySome(0)].Reference;value;dfc-generated |
43
| Unexpected summary found: repo::test;<crate::option::MyOption>::cloned;Argument[self].Field[crate::option::MyOption::MySome(0)].Reference;ReturnValue.Field[crate::option::MyOption::MySome(0)];value;dfc-generated |
5-
| Unexpected summary found: repo::test;<crate::option::MyOption>::get_or_insert;Argument[0];Argument[self].Field[crate::option::MyOption::MySome(0)];value;dfc-generated |
6-
| Unexpected summary found: repo::test;<crate::option::MyOption>::get_or_insert;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated |
7-
| Unexpected summary found: repo::test;<crate::option::MyOption>::get_or_insert_default;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated |
8-
| Unexpected summary found: repo::test;<crate::option::MyOption>::get_or_insert_with;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated |
9-
| Unexpected summary found: repo::test;<crate::option::MyOption>::insert;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated |
104
expectedModel
11-
| Expected summary missing: repo::test;<crate::option::MyOption>::take_if;Argument[self].Reference.Field[crate::option::MyOption::MySome(0)];Argument[0].Parameter[0].Reference;value;dfc-generated |
12-
| Expected summary missing: repo::test;<crate::option::MyOption>::take_if;Argument[self].Reference;ReturnValue;value;dfc-generated |

0 commit comments

Comments
 (0)