Skip to content

Commit 1105576

Browse files
committed
Rust: Handle writes to references and add encoding of reference content
1 parent 11685a8 commit 1105576

File tree

9 files changed

+69
-19
lines changed

9 files changed

+69
-19
lines changed

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,6 +1211,17 @@ module RustDataFlow implements InputSig<Location> {
12111211
)
12121212
}
12131213

1214+
pragma[nomagic]
1215+
private predicate referenceAssignment(Node node1, Node node2, ReferenceContent c) {
1216+
exists(AssignmentExprCfgNode assignment, PrefixExprCfgNode deref |
1217+
assignment.getLhs() = deref and
1218+
deref.getOperatorName() = "*" and
1219+
node1.asExpr() = assignment.getRhs() and
1220+
node2.asExpr() = deref.getExpr() and
1221+
exists(c)
1222+
)
1223+
}
1224+
12141225
pragma[nomagic]
12151226
private predicate storeContentStep(Node node1, Content c, Node node2) {
12161227
exists(CallExprCfgNode call, int pos |
@@ -1242,6 +1253,8 @@ module RustDataFlow implements InputSig<Location> {
12421253
or
12431254
fieldAssignment(node1, node2.(PostUpdateNode).getPreUpdateNode(), c)
12441255
or
1256+
referenceAssignment(node1, node2.(PostUpdateNode).getPreUpdateNode(), c)
1257+
or
12451258
exists(AssignmentExprCfgNode assignment, IndexExprCfgNode index |
12461259
c instanceof ElementContent and
12471260
assignment.getLhs() = index and
@@ -1285,6 +1298,8 @@ module RustDataFlow implements InputSig<Location> {
12851298
predicate clearsContent(Node n, ContentSet cs) {
12861299
fieldAssignment(_, n, cs.(SingletonContentSet).getContent())
12871300
or
1301+
referenceAssignment(_, n, cs.(SingletonContentSet).getContent())
1302+
or
12881303
FlowSummaryImpl::Private::Steps::summaryClearsContent(n.(Node::FlowSummaryNode).getSummaryNode(),
12891304
cs)
12901305
or

rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ module Input implements InputSig<Location, RustDataFlow> {
8989
arg = v.getExtendedCanonicalPath() + "(" + v.getPosition() + ")"
9090
)
9191
or
92+
result = "Reference" and
93+
c = TReferenceContent() and
94+
arg = ""
95+
or
9296
result = "Element" and
9397
c = TElementContent() and
9498
arg = ""

rust/ql/src/utils/modelgenerator/internal/CaptureModels.qll

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,8 @@ module ModelGeneratorInput implements ModelGeneratorInputSig<Location, RustDataF
8181
}
8282

8383
bindingset[c]
84-
string paramReturnNodeAsOutput(R::Callable c, ParameterPosition pos) {
85-
// TODO: Implement this to support returning through parameters.
86-
result = "paramReturnNodeAsOutput(" + c + ", " + pos + ")"
84+
string paramReturnNodeAsOutput(Callable c, ParameterPosition pos) {
85+
result = paramReturnNodeAsContentOutput(c, pos)
8786
}
8887

8988
bindingset[c]

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,13 @@ edges
4242
| main.rs:101:13:101:30 | mn.data_through(...) | main.rs:101:9:101:9 | b | provenance | |
4343
| main.rs:101:29:101:29 | a | main.rs:77:28:77:33 | ...: i64 | provenance | |
4444
| main.rs:101:29:101:29 | a | main.rs:101:13:101:30 | mn.data_through(...) | provenance | |
45+
| main.rs:139:25:139:30 | ...: i64 | main.rs:140:10:140:10 | c | provenance | |
46+
| main.rs:140:6:140:6 | [post] n [&ref] | main.rs:139:12:139:22 | ...: ... [Return] [&ref] | provenance | |
47+
| main.rs:140:10:140:10 | c | main.rs:140:6:140:6 | [post] n [&ref] | provenance | |
48+
| main.rs:148:13:148:13 | [post] m [&ref] | main.rs:149:11:149:11 | m [&ref] | provenance | |
49+
| main.rs:148:16:148:25 | source(...) | main.rs:139:25:139:30 | ...: i64 | provenance | |
50+
| main.rs:148:16:148:25 | source(...) | main.rs:148:13:148:13 | [post] m [&ref] | provenance | |
51+
| main.rs:149:11:149:11 | m [&ref] | main.rs:149:10:149:11 | * ... | provenance | |
4552
nodes
4653
| main.rs:12:28:14:1 | { ... } | semmle.label | { ... } |
4754
| main.rs:13:5:13:13 | source(...) | semmle.label | source(...) |
@@ -92,11 +99,20 @@ nodes
9299
| main.rs:101:13:101:30 | mn.data_through(...) | semmle.label | mn.data_through(...) |
93100
| main.rs:101:29:101:29 | a | semmle.label | a |
94101
| main.rs:102:10:102:10 | b | semmle.label | b |
102+
| main.rs:139:12:139:22 | ...: ... [Return] [&ref] | semmle.label | ...: ... [Return] [&ref] |
103+
| main.rs:139:25:139:30 | ...: i64 | semmle.label | ...: i64 |
104+
| main.rs:140:6:140:6 | [post] n [&ref] | semmle.label | [post] n [&ref] |
105+
| main.rs:140:10:140:10 | c | semmle.label | c |
106+
| main.rs:148:13:148:13 | [post] m [&ref] | semmle.label | [post] m [&ref] |
107+
| main.rs:148:16:148:25 | source(...) | semmle.label | source(...) |
108+
| main.rs:149:10:149:11 | * ... | semmle.label | * ... |
109+
| main.rs:149:11:149:11 | m [&ref] | semmle.label | m [&ref] |
95110
subpaths
96111
| main.rs:36:26:36:26 | a | main.rs:30:17:30:22 | ...: i64 | main.rs:30:32:32:1 | { ... } | main.rs:36:13:36:27 | pass_through(...) |
97112
| main.rs:41:26:44:5 | { ... } | main.rs:30:17:30:22 | ...: i64 | main.rs:30:32:32:1 | { ... } | main.rs:41:13:44:6 | pass_through(...) |
98113
| main.rs:55:26:55:26 | a | main.rs:51:21:51:26 | ...: i64 | main.rs:51:36:53:5 | { ... } | main.rs:55:13:55:27 | pass_through(...) |
99114
| main.rs:101:29:101:29 | a | main.rs:77:28:77:33 | ...: i64 | main.rs:77:43:83:5 | { ... } | main.rs:101:13:101:30 | mn.data_through(...) |
115+
| main.rs:148:16:148:25 | source(...) | main.rs:139:25:139:30 | ...: i64 | main.rs:139:12:139:22 | ...: ... [Return] [&ref] | main.rs:148:13:148:13 | [post] m [&ref] |
100116
testFailures
101117
#select
102118
| main.rs:18:10:18:10 | a | main.rs:13:5:13:13 | source(...) | main.rs:18:10:18:10 | a | $@ | main.rs:13:5:13:13 | source(...) | source(...) |
@@ -107,3 +123,4 @@ testFailures
107123
| main.rs:68:14:68:14 | n | main.rs:94:13:94:21 | source(...) | main.rs:68:14:68:14 | n | $@ | main.rs:94:13:94:21 | source(...) | source(...) |
108124
| main.rs:89:10:89:10 | a | main.rs:74:13:74:21 | source(...) | main.rs:89:10:89:10 | a | $@ | main.rs:74:13:74:21 | source(...) | source(...) |
109125
| main.rs:102:10:102:10 | b | main.rs:100:13:100:21 | source(...) | main.rs:102:10:102:10 | b | $@ | main.rs:100:13:100:21 | source(...) | source(...) |
126+
| main.rs:149:10:149:11 | * ... | main.rs:148:16:148:25 | source(...) | main.rs:149:10:149:11 | * ... | $@ | main.rs:148:16:148:25 | source(...) | source(...) |

rust/ql/test/library-tests/dataflow/global/main.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ fn mutates_argument_1() {
146146
let m = &mut n;
147147
sink(*m);
148148
set_int(m, source(37));
149-
sink(*m); // $ MISSING: hasValueFlow=37
149+
sink(*m); // $ hasValueFlow=37
150150
}
151151

152152
fn mutates_argument_2() {

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ edges
88
| main.rs:15:9:15:9 | c | main.rs:16:10:16:10 | c | provenance | |
99
| main.rs:15:13:15:14 | * ... | main.rs:15:9:15:9 | c | provenance | |
1010
| main.rs:15:14:15:14 | b [&ref] | main.rs:15:13:15:14 | * ... | provenance | |
11+
| main.rs:31:6:31:6 | [post] b [&ref] | main.rs:32:11:32:11 | b [&ref] | provenance | |
12+
| main.rs:31:10:31:19 | source(...) | main.rs:31:6:31:6 | [post] b [&ref] | provenance | |
13+
| main.rs:32:11:32:11 | b [&ref] | main.rs:32:10:32:11 | * ... | provenance | |
1114
| main.rs:37:25:37:26 | &... [&ref] | main.rs:37:26:37:26 | n | provenance | |
1215
| main.rs:37:25:37:32 | ...: ... [&ref] | main.rs:37:25:37:26 | &... [&ref] | provenance | |
1316
| main.rs:37:26:37:26 | n | main.rs:38:10:38:10 | n | provenance | |
@@ -54,6 +57,10 @@ nodes
5457
| main.rs:15:13:15:14 | * ... | semmle.label | * ... |
5558
| main.rs:15:14:15:14 | b [&ref] | semmle.label | b [&ref] |
5659
| main.rs:16:10:16:10 | c | semmle.label | c |
60+
| main.rs:31:6:31:6 | [post] b [&ref] | semmle.label | [post] b [&ref] |
61+
| main.rs:31:10:31:19 | source(...) | semmle.label | source(...) |
62+
| main.rs:32:10:32:11 | * ... | semmle.label | * ... |
63+
| main.rs:32:11:32:11 | b [&ref] | semmle.label | b [&ref] |
5764
| main.rs:37:25:37:26 | &... [&ref] | semmle.label | &... [&ref] |
5865
| main.rs:37:25:37:32 | ...: ... [&ref] | semmle.label | ...: ... [&ref] |
5966
| main.rs:37:26:37:26 | n | semmle.label | n |
@@ -100,6 +107,7 @@ subpaths
100107
testFailures
101108
#select
102109
| main.rs:16:10:16:10 | c | main.rs:13:13:13:22 | source(...) | main.rs:16:10:16:10 | c | $@ | main.rs:13:13:13:22 | source(...) | source(...) |
110+
| main.rs:32:10:32:11 | * ... | main.rs:31:10:31:19 | source(...) | main.rs:32:10:32:11 | * ... | $@ | main.rs:31:10:31:19 | source(...) | source(...) |
103111
| main.rs:38:10:38:10 | n | main.rs:42:15:42:24 | source(...) | main.rs:38:10:38:10 | n | $@ | main.rs:42:15:42:24 | source(...) | source(...) |
104112
| main.rs:70:10:70:30 | my_number.to_number(...) | main.rs:69:40:69:49 | source(...) | main.rs:70:10:70:30 | my_number.to_number(...) | $@ | main.rs:69:40:69:49 | source(...) | source(...) |
105113
| main.rs:80:10:80:31 | my_number.get_number(...) | main.rs:79:41:79:50 | source(...) | main.rs:80:10:80:31 | my_number.get_number(...) | $@ | main.rs:79:41:79:50 | source(...) | source(...) |

rust/ql/test/library-tests/dataflow/pointers/main.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ fn write_and_read_through_borrow() {
2929
let b = &mut a;
3030
sink(*b);
3131
*b = source(37);
32-
sink(*b); // $ MISSING: hasValueFlow=37
32+
sink(*b); // $ hasValueFlow=37
3333
*b = 0;
3434
sink(*b); // now cleared
3535
}

rust/ql/test/utils-tests/modelgenerator/option.rs

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,17 @@ impl<T> MyOption<T> {
4444
}
4545
}
4646

47-
// summary=repo::test;<crate::option::MyOption>::as_ref;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
47+
// NOTE: The returned value inside the variant should be inside a `Reference`, requires handling
48+
// `ref` in patterns.
49+
// summary=repo::test;<crate::option::MyOption>::as_ref;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
4850
pub fn as_ref(&self) -> MyOption<&T> {
4951
match *self {
5052
MySome(ref x) => MySome(x),
5153
MyNone => MyNone,
5254
}
5355
}
5456

55-
// summary=repo::test;<crate::option::MyOption>::as_mut;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
57+
// summary=repo::test;<crate::option::MyOption>::as_mut;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
5658
pub fn as_mut(&mut self) -> MyOption<&mut T> {
5759
match *self {
5860
MySome(ref mut x) => MySome(x),
@@ -285,30 +287,34 @@ impl<T> MyOption<T> {
285287
}
286288
}
287289

288-
// MISSING: summary=repo::test;<crate::option::MyOption>::insert;Argument[0];ReturnValue;value;dfc-generated
289-
// SPURIOUS-summary=repo::test;<crate::option::MyOption>::insert;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
290+
// summary=repo::test;<crate::option::MyOption>::insert;Argument[0];Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
291+
// The below should be `ReturnValue.Reference` and not just `ReturnValue`.
292+
// SPURIOUS-summary=repo::test;<crate::option::MyOption>::insert;Argument[0];ReturnValue;value;dfc-generated
293+
// The content of `self` is overwritten so it does not flow to the return value.
294+
// SPURIOUS-summary=repo::test;<crate::option::MyOption>::insert;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
290295
pub fn insert(&mut self, value: T) -> &mut T {
291296
*self = MySome(value);
292297

293298
// SAFETY: the code above just filled the MyOption
294299
unsafe { self.as_mut().unwrap_unchecked() }
295300
}
296301

297-
// summary=repo::test;<crate::option::MyOption>::get_or_insert;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
298-
// MISSING: repo::test;<crate::option::MyOption>::get_or_insert;Argument[0];ReturnValue;value;dfc-generated
302+
// summary=repo::test;<crate::option::MyOption>::get_or_insert;Argument[0];Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
303+
// summary=repo::test;<crate::option::MyOption>::get_or_insert;Argument[0];ReturnValue;value;dfc-generated
304+
// summary=repo::test;<crate::option::MyOption>::get_or_insert;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
299305
pub fn get_or_insert(&mut self, value: T) -> &mut T {
300306
self.get_or_insert_with(|| value)
301307
}
302308

303-
// summary=repo::test;<crate::option::MyOption>::get_or_insert_default;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
309+
// summary=repo::test;<crate::option::MyOption>::get_or_insert_default;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
304310
pub fn get_or_insert_default(&mut self) -> &mut T
305311
where
306312
T: Default,
307313
{
308314
self.get_or_insert_with(T::default)
309315
}
310316

311-
// summary=repo::test;<crate::option::MyOption>::get_or_insert_with;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
317+
// summary=repo::test;<crate::option::MyOption>::get_or_insert_with;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue;value;dfc-generated
312318
// MISSING: Mutating `self` parameter.
313319
pub fn get_or_insert_with<F>(&mut self, f: F) -> &mut T
314320
where
@@ -329,7 +335,7 @@ impl<T> MyOption<T> {
329335
mem::replace(self, MyNone)
330336
}
331337

332-
// summary=repo::test;<crate::option::MyOption>::take_if;Argument[self].Variant[crate::option::MyOption::MySome(0)];Argument[0].Parameter[0];value;dfc-generated
338+
// summary=repo::test;<crate::option::MyOption>::take_if;Argument[self].Reference.Variant[crate::option::MyOption::MySome(0)];Argument[0].Parameter[0];value;dfc-generated
333339
// MISSING: Uses `take` which doesn't have flow
334340
pub fn take_if<P>(&mut self, predicate: P) -> MyOption<T>
335341
where
@@ -378,7 +384,7 @@ impl<T, U> MyOption<(T, U)> {
378384
}
379385

380386
impl<T> MyOption<&T> {
381-
// summary=repo::test;<crate::option::MyOption>::copied;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
387+
// summary=repo::test;<crate::option::MyOption>::copied;Argument[self].Variant[crate::option::MyOption::MySome(0)].Reference;ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
382388
pub fn copied(self) -> MyOption<T>
383389
where
384390
T: Copy,
@@ -404,7 +410,7 @@ impl<T> MyOption<&T> {
404410
}
405411

406412
impl<T> MyOption<&mut T> {
407-
// summary=repo::test;<crate::option::MyOption>::copied;Argument[self].Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
413+
// summary=repo::test;<crate::option::MyOption>::copied;Argument[self].Variant[crate::option::MyOption::MySome(0)].Reference;ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
408414
pub fn copied(self) -> MyOption<T>
409415
where
410416
T: Copy,
@@ -474,14 +480,14 @@ impl<T> From<T> for MyOption<T> {
474480
}
475481

476482
impl<'a, T> From<&'a MyOption<T>> for MyOption<&'a T> {
477-
// summary=repo::test;<crate::option::MyOption as crate::convert::From>::from;Argument[0].Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
483+
// summary=repo::test;<crate::option::MyOption as crate::convert::From>::from;Argument[0].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
478484
fn from(o: &'a MyOption<T>) -> MyOption<&'a T> {
479485
o.as_ref()
480486
}
481487
}
482488

483489
impl<'a, T> From<&'a mut MyOption<T>> for MyOption<&'a mut T> {
484-
// summary=repo::test;<crate::option::MyOption as crate::convert::From>::from;Argument[0].Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
490+
// summary=repo::test;<crate::option::MyOption as crate::convert::From>::from;Argument[0].Reference.Variant[crate::option::MyOption::MySome(0)];ReturnValue.Variant[crate::option::MyOption::MySome(0)];value;dfc-generated
485491
fn from(o: &'a mut MyOption<T>) -> MyOption<&'a mut T> {
486492
o.as_mut()
487493
}

rust/ql/test/utils-tests/modelgenerator/summaries.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,12 @@ pub fn apply<F>(n: i64, f: F) -> i64 where F : FnOnce(i64) -> i64 {
7979

8080
// Flow out of mutated arguments
8181

82+
// summary=repo::test;crate::summaries::set_int;Argument[1];Argument[0].Reference;value;dfc-generated
8283
pub fn set_int(n: &mut i64, c: i64) {
8384
*n = c;
8485
}
8586

86-
// summary=repo::test;crate::summaries::read_int;Argument[0];ReturnValue;value;dfc-generated
87+
// summary=repo::test;crate::summaries::read_int;Argument[0].Reference;ReturnValue;value;dfc-generated
8788
pub fn read_int(n: &mut i64) -> i64 {
8889
*n
8990
}

0 commit comments

Comments
 (0)