Skip to content

Rust: Update legacy MaD models 3 #19946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Rust: Update legacy MaD models 3 #19946

wants to merge 13 commits into from

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 1, 2025

Update even more legacy MaD models to the new model format (continues from #19942 and should be independent of that).

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Jul 1, 2025
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 2, 2025

There's currently a large number of test changes caused by this PR (highlighted by CI). I think it would be prudent to get the first two PRs merged before attempting to address this. --- no longer true.

@aibaars
Copy link
Contributor

aibaars commented Jul 10, 2025

At a glance the test changes are due to re-ordering of lines, so that looks fine to me.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

The lost sources in the frameworks/rusqlite test I've added to the issue logging regressions. The rest of the CI failures were just location and model changes. I think this is good to merge now.

@geoffw0 geoffw0 marked this pull request as ready for review July 10, 2025 16:36
@geoffw0 geoffw0 requested a review from a team as a code owner July 10, 2025 16:36
id: row.get(0)?, // $ database-read
name: row.get(1)?, // $ database-read
age: row.get(2)?, // $ database-read
id: row.get(0)?, // $ MISSING: database-read
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the problem is a missing canonical path, or is the problem that getStaticTarget fails (due to type inferencer problem)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the canonical path exists, the problem is the type inferencer, most likely the closure argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep you're right, the getStaticTarget fails (and thus getStaticTarget().(Addressable).getCanonicalPath() fails too) so it's likely a type inference problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's why I use a concat in my test predicate:

predicate test(MethodCallExpr e, Function target, string path) {
  target = e.getStaticTarget() and
  path = concat(target.getCanonicalPath())
}
  • no result means getStaticTarget() failed
  • empty string means: a target was found, but getCanonicalPath() failed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most likely the closure argument.

Correct; type inference for closures is a known gap.

Yeah that's why I use a concat in my test predicate:

I can also recommend the various debug predicates in TypeInference.qll; change getRelevantLocatable to the relevant file+line, and then quick-eval debugInferType et al.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try the TypeInference.qll debug predicates, thanks for reminding me those exist.

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly fine, the rusqlite test failures can be fixed though.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 11, 2025

Fixed issues and merged in main. Ready for approval.

@aibaars
Copy link
Contributor

aibaars commented Jul 11, 2025

Have you checked the hyper project? DCA reports we are losing a fair number of sources. Just to make sure the hyper models are good

@aibaars
Copy link
Contributor

aibaars commented Jul 11, 2025

There is still a test failure:

+| test.rs:483:22:483:50 | file.read_to_end(...) |
+| test.rs:489:22:489:53 | file.read_to_string(...) |
 | test.rs:610:18:610:38 | ...::_print(...) |
 | test.rs:615:18:615:45 | ...::_print(...) |
 | test.rs:619:25:619:49 | address.to_socket_addrs() |
Error: [2/2] [984/989 comp 23.4s eval 41.3s] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/rust/ql/test/library-tests/dataflow/sources/CONSISTENCY/PathResolutionConsistency.ql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants