Skip to content

Rust: regenerate models #19748

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 2 commits into
base: main
Choose a base branch
from
Open

Rust: regenerate models #19748

wants to merge 2 commits into from

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Jun 13, 2025

Models are regenerated with the fix from #19744 which corrects the order of generation.

Notice the one new FP in the tests though.

Models are regenerated with the fix from #19744
which corrects the order of generation.
@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 13, 2025
@redsun82 redsun82 marked this pull request as ready for review June 13, 2025 10:21
@Copilot Copilot AI review requested due to automatic review settings June 13, 2025 10:21
@redsun82 redsun82 requested a review from a team as a code owner June 13, 2025 10:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Regenerates Rust models after correcting the order of generation (CodeQL PR 19744) and updates the CWE-770 test suite to account for one newly introduced false positive.

  • Annotates the shrink call in main.rs as a spurious alert.
  • Inserts corresponding expected entries in the test’s .expected file.

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

File Description
rust/ql/test/query-tests/security/CWE-770/main.rs Added // $ SPURIOUS comment on the shrink call to mark a FP.
rust/ql/test/query-tests/security/CWE-770/UncontrolledAllocationSize.expected Added new expected lines for the spurious shrink alert and re-numbered model sinks/summaries.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM (not that I've manually checked all the models!)

@@ -210,7 +210,7 @@ unsafe fn test_system_alloc(v: usize) {
let _ = std::alloc::System.grow_zeroed(m4, l4, l2).unwrap(); // $ Alert[rust/uncontrolled-allocation-size]=arg1
}
} else {
let _ = std::alloc::System.shrink(m4, l4, l2).unwrap();
let _ = std::alloc::System.shrink(m4, l4, l2).unwrap(); // $ SPURIOUS: Alert[rust/uncontrolled-allocation-size]=arg1 - FP
Copy link
Contributor

@geoffw0 geoffw0 Jun 13, 2025

Choose a reason for hiding this comment

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

I think the model generator is right to generate flow for this, shrink is causing an allocation to happen and the new size l2 is user controlled. The reason this is a FP is because we know a shrink can only reduce the size of the allocation so the allocation is bounded - this can't be the source of a problem.

I think the right answer is to introduce a barrier. I can do this as follow-up to your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants