Conversation
Models are regenerated with the fix from #19744 which corrects the order of generation.
There was a problem hiding this comment.
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
shrinkcall inmain.rsas a spurious alert. - Inserts corresponding expected entries in the test’s
.expectedfile.
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. |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
LGTM (not that I've manually checked all the models!)
| } | ||
| } 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
well, reading https://doc.rust-lang.org/beta/std/alloc/trait.Allocator.html#safety-4 I can't decide whether we can count on shrink actually shrinking, considering that the constraint of l2's size being smaller that l4's one is actually a safety contract to be guaranteed by the user.
However apart from that, we should know that l2 has size v, and that v < 10 here, which means we aren't really unconstrained in any case here. Or is this too much for the current implementation of the analysis? Are we doing some kind of range analysis?
There was a problem hiding this comment.
We don't have range analysis, we do have barrier guards and in fact the query is using them. However in this case the check on v is after the std::alloc::Layout has been created from it, so we don't notice the guard applies to the layout. I'll see if I can easily address this...
2 participants
Models are regenerated with the fix from #19744 which corrects the order of generation.
Notice the one new FP in the tests though.