Rust: Make `SummarizedCallable` extend `Function` instead of `string` by hvitved · Pull Request #19268 · github/codeql

hvitved · 2025-04-10T07:46:06Z

This PR changes the base class of SummarizedCallable from string to Function, thereby reusing the existing function entities already extracted instead of synthesizing new entities.

This is a big step towards using QL computed canonical paths in models-as-data instead of using extractor provided canonical paths.

A lot of preliminary PRs mean that we mostly retain results, however as witnessed by the tests, we still lose some results. This is largely because:

We do not resolve trait implementations of the form impl<T> Foo for T.
We do not handle implicit Derefs in type inference.
No type inference for patterns.
Rust: Fix type inference for library parameters #19658 not yet merged.

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

rust/ql/lib/codeql/rust/frameworks/stdlib/Clone.qll

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

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

 Type inferType(AstNode n) { result = inferType(n, TypePath::nil()) }
+
+/** Provides predicates for debugging the type inference implementation. */
+private module Debug {


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

+/** Provides predicates for debugging the type inference implementation. */
+private module Debug {
+  private Locatable getRelevantLocatable() {
+    exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |


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

+/** Provides predicates for debugging the type inference implementation. */
+private module Debug {
+  private Locatable getRelevantLocatable() {
+    exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |


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

+/** Provides predicates for debugging the type inference implementation. */
+private module Debug {
+  private Locatable getRelevantLocatable() {
+    exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |


rust/ql/lib/codeql/rust/elements/internal/SelfParamImpl.qll

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

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

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

geoffw0

Test and DCA changes LGTM (I haven't reviewed the QL changes).

There are some losses (in particular: common data flow paths in tests) but they're well understood / documented with plans in place to get them back. 👍

geoffw0 · 2025-06-17T09:01:18Z

rust/ql/test/library-tests/dataflow/sources/test_futures_io.rs

+use futures_rustls::TlsConnector;
+use std::io;
+use std::pin::Pin;
+use std::task::{Context, Poll};


Why are we changing the imports for this test?

If it was done by an auto-formatter can be avoid doing that in future, we do not want to test only auto-formatted target code.

Yes, it was auto-format.

rust/ql/test/library-tests/sensitivedata/test.rs

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

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

paldepind

Looks great! Only one nitpick.

One question, is using string for SummarizedCallable the approach taken for dynamic languages? Or is that some third approach?

paldepind · 2025-06-17T16:00:57Z

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

+      target = result.asCfgScope()
+      or
+      target = result.asSummarizedCallable()
+    )


I guess this is a key change? We now use getStaticTarget target in both cases, whereas we previously used getACall in the later case, which used extractor provided canonical paths internally.

Absolutely correct.

paldepind · 2025-06-17T16:04:39Z

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

-      filepath.matches("%/test.rs") and
-      startline = 74
+      filepath.matches("%/main.rs") and
+      startline = 52


It would be nice not to commit these changes. For instance, when looking at the history of a file it's easier if the commits that show up actually make meaningful changes.

Good point, I'll avoid that going forward.

paldepind · 2025-06-17T16:06:29Z

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

-      filepath.matches("%/main.rs") and
-      startline = 1718
+      filepath.matches("%/sqlx.rs") and
+      startline = [56 .. 60]


Save as above.

hvitved · 2025-06-17T18:51:27Z

One question, is using string for SummarizedCallable the approach taken for dynamic languages? Or is that some third approach?

Yes, that is indeed the approach taken there, where there are no extracted entities to use.

github-actions bot added the Rust Pull requests that update Rust code label Apr 10, 2025

github-advanced-security bot found potential problems Apr 10, 2025

View reviewed changes

hvitved force-pushed the rust/summarized-callable-base branch 3 times, most recently from 86ccc96 to a91cc6b Compare April 29, 2025 08:53

github-advanced-security bot found potential problems Apr 29, 2025

View reviewed changes

hvitved force-pushed the rust/summarized-callable-base branch 4 times, most recently from d8b7085 to 10a72f7 Compare May 2, 2025 09:07

hvitved force-pushed the rust/summarized-callable-base branch 3 times, most recently from 9b1cd94 to 2a2e7d5 Compare May 19, 2025 08:02

hvitved force-pushed the rust/summarized-callable-base branch 3 times, most recently from e0a2c1d to dc9700e Compare May 27, 2025 10:36

hvitved force-pushed the rust/summarized-callable-base branch from dc9700e to 4224947 Compare June 3, 2025 08:42

github-advanced-security bot found potential problems Jun 3, 2025

View reviewed changes

rust/ql/lib/codeql/rust/internal/TypeInference.qll Fixed Show fixed Hide fixed

rust/ql/lib/codeql/rust/internal/TypeInference.qll Fixed Show fixed Hide fixed

rust/ql/lib/codeql/rust/internal/TypeInference.qll Fixed Show fixed Hide fixed

hvitved force-pushed the rust/summarized-callable-base branch 4 times, most recently from b0991ad to c29c5f9 Compare June 10, 2025 09:42

hvitved force-pushed the rust/summarized-callable-base branch 8 times, most recently from c1959ca to 73c4919 Compare June 15, 2025 18:14

hvitved force-pushed the rust/summarized-callable-base branch 5 times, most recently from afa16ea to 58cd7b0 Compare June 16, 2025 11:06

hvitved marked this pull request as ready for review June 16, 2025 11:19

hvitved requested a review from a team as a code owner June 16, 2025 11:19

geoffw0 reviewed Jun 17, 2025

View reviewed changes

hvitved force-pushed the rust/summarized-callable-base branch from 58cd7b0 to b978d35 Compare June 17, 2025 09:36

Rust: Make SummarizedCallable extend Function instead of string

433756d

hvitved force-pushed the rust/summarized-callable-base branch from b978d35 to 433756d Compare June 17, 2025 12:13

geoffw0 mentioned this pull request Jun 17, 2025

Rust: Update SqlxQuery, SqlxExecute to use getCanonicalPath #19802

Merged

paldepind requested changes Jun 17, 2025

View reviewed changes

paldepind approved these changes Jun 18, 2025

View reviewed changes

hvitved merged commit bef07a7 into github:main Jun 18, 2025
18 checks passed

hvitved deleted the rust/summarized-callable-base branch June 18, 2025 06:31

Conversation

hvitved commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check warning

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

geoffw0 Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paldepind left a comment

Choose a reason for hiding this comment

Uh oh!

paldepind Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved commented Jun 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hvitved commented Apr 10, 2025 •

edited

Loading