Rust: Allow SSA and some data flow for mutable borrows#18872
Rust: Allow SSA and some data flow for mutable borrows#18872paldepind merged 6 commits intogithub:mainfrom
Conversation
paldepind
force-pushed
the
rust-ref-mut
branch
from
ada5c5a to
476fef4
Compare
There was a problem hiding this comment.
PR Overview
This PR expands the data flow testing in the Rust queries to support SSA and mutable borrows while refining the expected test annotations. Key changes include:
- Splitting intraprocedural tests into immutable and mutable borrow modules with updated function signatures.
- Modifying methods on types such as MyFlag and MyNumber to take ownership for more precise flow tracking.
- Updating expected flow and SQL injection annotation markers in several test files.
Reviewed Changes
| File | Description |
|---|---|
| rust/ql/test/library-tests/dataflow/pointers/main.rs | Added new intraprocedural modules and updated test annotations for pointer and borrow flows. |
| rust/ql/test/library-tests/dataflow/global/main.rs | Changed method receivers from &self to self and updated related test call sites. |
| rust/ql/test/library-tests/dataflow/local/main.rs | Corrected expected flow annotation on iterator tests. |
| rust/ql/test/library-tests/dataflow/sources/test.rs | Adjusted taint flow annotation to the correct expected value. |
| rust/ql/test/query-tests/security/CWE-089/sqlx.rs | Fixed SQL injection alert annotations to correctly reflect expected test markers. |
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (11)
rust/ql/test/library-tests/dataflow/pointers/main.rs:47
- The expected value flow annotation for sink(c) in ref_nested_pattern_match is missing; please add '$ hasValueFlow=23' to align the test.
sink(c); // $ MISSING: hasValueFlow=23
rust/ql/test/library-tests/dataflow/pointers/main.rs:84
- Missing expected value flow annotation for sink(a) in write_through_borrow; add '$ hasValueFlow=39' to meet the test expectations.
sink(a); // $ MISSING: hasValueFlow=39
rust/ql/test/library-tests/dataflow/pointers/main.rs:107
- The sink(a) call in write_through_borrow_in_match is missing its value flow annotation; please add '$ hasValueFlow=24'.
sink(a); // $ MISSING: hasValueFlow=24
rust/ql/test/library-tests/dataflow/pointers/main.rs:108
- The sink(b) call in write_through_borrow_in_match is missing its expected annotation; please add '$ hasValueFlow=24'.
sink(b); // $ MISSING: hasValueFlow=24
rust/ql/test/library-tests/dataflow/pointers/main.rs:123
- The expected annotation for sink(t.1) in mutate_tuple is missing; add '$ hasValueFlow=48' to satisfy the test conditions.
sink(t.1); // $ MISSING: hasValueFlow=48
rust/ql/test/library-tests/dataflow/pointers/main.rs:139
- The call to sink(a.0) in tuple_match_mut is missing its expected flow annotation; please add '$ hasValueFlow=71'.
sink(a.0); // $ MISSING: hasValueFlow=71
rust/ql/test/library-tests/dataflow/global/main.rs:68
- [nitpick] Multiple value flow annotations are present on sink(n); please use a single, consistent annotation to avoid confusion.
sink(n); // $ hasValueFlow=1 hasValueFlow=8
rust/ql/test/query-tests/security/CWE-089/sqlx.rs:63
- The SQL injection alert annotation for unsafe_query_1 is missing; please add 'Alert[rust/sql-injection]=args1'.
let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink MISSING: Alert[rust/sql-injection]=args1
rust/ql/test/query-tests/security/CWE-089/sqlx.rs:66
- Missing SQL injection alert annotation for unsafe_query_3; please add 'Alert[rust/sql-injection]=remote1'.
let _ = conn.execute(unsafe_query_3.as_str()).await?; // $ sql-sink MISSING: Alert[rust/sql-injection]=remote1
rust/ql/test/query-tests/security/CWE-089/sqlx.rs:75
- The SQL query for unsafe_query_1 is missing the expected alert annotation; please update it with 'Alert[rust/sql-injection]=args1'.
let _ = sqlx::query(unsafe_query_1.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[rust/sql-injection][rust/sql-injection]=args1
rust/ql/test/query-tests/security/CWE-089/sqlx.rs:77
- Please add the missing SQL injection alert annotation 'Alert[rust/sql-injection]=remote1' for unsafe_query_3 in this SQL query.
let _ = sqlx::query(unsafe_query_3.as_str()).execute(&pool).await?; // $ sql-sink MISSING: Alert[rust/sql-injection]=remote1
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
| | 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 | | ||
| | 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 | | ||
| | Unexpected summary found: repo::test;<crate::option::MyOption>::insert;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated | | ||
| | Unexpected summary found: repo::test;<crate::option::MyOption>::take_if;Argument[self].Field[crate::option::MyOption::MySome(0)];Argument[0].Parameter[0].Reference;value;dfc-generated | |
There was a problem hiding this comment.
These summaries are incorrect and show up since we now insert implicit borrow/deref in some places where there actually isn't any. Once we can place them more precisely using type information, these should disappear again.
|
CI is failing, but I think it's only in the way in which it seems to fail for everything right now. |
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Really nice work, great to see that it worked out in the end. A few comments.
| ) | ||
| } | ||
| } | ||
| class SourceVariable = Variable; |
There was a problem hiding this comment.
The QL doc needs to be updated now (or perhaps simply be removed).
| sink(to_number(my_number)); // $ hasValueFlow=99 | ||
| (&mut my_number).set(0); | ||
| sink(to_number(my_number)); // now cleared | ||
| sink(to_number(my_number)); // SPURIOUS: hasValueFlow=99 |
| predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { | ||
| exists(Variable v, BasicBlock bb, int i | | ||
| def.definesAt(v, bb, i) and mutablyBorrows(bb.getNode(i).getAstNode(), v) | ||
| ) | ||
| } |
There was a problem hiding this comment.
As discussed offline, perhaps this should be restricted to borrows passed into calls, i.e.
| predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { | |
| exists(Variable v, BasicBlock bb, int i | | |
| def.definesAt(v, bb, i) and mutablyBorrows(bb.getNode(i).getAstNode(), v) | |
| ) | |
| } | |
| predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { | |
| exists(CfgNodes::CallExprBaseCfgNode call, Variable v, BasicBlock bb, int i | | |
| def.definesAt(v, bb, i) and | |
| mutablyBorrows(bb.getNode(i).getAstNode(), v) | |
| | | |
| call.getArgument(_) = bb.getNode(i) | |
| or | |
| call.(CfgNodes::MethodCallExprCfgNode).getReceiver() = bb.getNode(i) | |
| ) | |
| } |
| ExprArgumentNode() { | ||
| isArgumentForCall(n, call_, pos_) and | ||
| // For receivers in method calls the `ReceiverNode` is the argument. | ||
| not call_.(MethodCallExprCfgNode).getReceiver() = n |
There was a problem hiding this comment.
Perhaps instead remove the arg = call.(MethodCallExprCfgNode).getReceiver() and pos.isSelf() case inside isArgumentForCall, and then remove this restriction and add a e = any(MethodCallExprCfgNode mc).getReceiver() case to TExprPostUpdateNode.
| } | ||
|
|
||
| /** | ||
| * The receiver of a method call _after_ any implicit borrow or dereferences |
There was a problem hiding this comment.
| * The receiver of a method call _after_ any implicit borrow or dereferences | |
| * The receiver of a method call _after_ any implicit borrow or dereferencing |
|
|
||
| override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() } | ||
|
|
||
| override Location getLocation() { result = n.getLocation() } |
There was a problem hiding this comment.
I think it would be better to use this.getReceiver().getLocation().
|
|
||
| override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() } | ||
|
|
||
| override Location getLocation() { result = n.getLocation() } |
| } | ||
|
|
||
| /** Holds if `mc` implicitly borrows its receiver. */ | ||
| predicate implicitBorrow(MethodCallExpr mc) { |
| } | ||
|
|
||
| /** Holds if `mc` implicitly dereferences its receiver. */ | ||
| predicate implicitDeref(MethodCallExpr mc) { |
paldepind
force-pushed
the
rust-ref-mut
branch
2 times, most recently
from
14b1114 to
cc2e13e
Compare
paldepind
force-pushed
the
rust-ref-mut
branch
from
cc2e13e to
bc651af
Compare
|
I've addressed the comments :) |
|
I've merged |
2 participants
As seen in the tests this gets us some true flow but also some false flow. If there's an "easy" fix to the false flow (without loosing true flow) then that would be great, otherwise I think it's ok as is.