-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Add more barriers to cpp/overrun-write
#20107
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
Conversation
…s on pointers in the 'cpp/overrun-write' query.
…few more columns that we'll need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extracts and consolidates barrier logic from the cpp/invalid-pointer-deref
query into a reusable module for the cpp/overrun-write
query. The purpose is to improve performance and reduce false positives by applying proven barriers that detect whether a pointer is within bounds before dereferencing.
Key changes:
- Creates a new shared
ProductFlowUtils
module containing theSizeBarrier
parameterized module - Updates
cpp/overrun-write
query to use the extracted barriers, removing custom flow step logic - Refactors
cpp/invalid-pointer-deref
to use the shared barrier module
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
ProductFlowUtils.qll |
New module providing shared SizeBarrier implementation with hasSize predicate and barrier logic |
OverrunWriteProductFlow.ql |
Refactored to use shared barriers, simplified flow configuration, removed custom flow steps |
AllocationToInvalidPointer.qll |
Updated to use shared SizeBarrier module instead of local implementation |
test.cpp |
Added test case for barrier effectiveness |
OverrunWriteProductFlow.expected |
Updated test expectations showing removal of false positive results |
cpp/ql/lib/semmle/code/cpp/security/ProductFlowUtils/ProductFlowUtils.qll
Outdated
Show resolved
Hide resolved
…owUtils.qll Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
cpp/ql/lib/semmle/code/cpp/security/ProductFlowUtils/ProductFlowUtils.qll
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
The changes seem reasonable, and it's great to see a reduction in false positives.
Also, DCA is happy.
…owUtils.qll Co-authored-by: Idriss Riouak <idrissrio@github.com>
Thanks a lot, Idriss! ❤️ I've accepted your suggestion which removed the approval 😭 |
This PR extracts the barriers from
cpp/invalid-pointer-deref
(which detect whether a pointer is within bounds before a dereference) into its own small module so that it can be reused by thecpp/overrun-write
query.I had to remove the additional logic to flow through pointer arithmetic while keeping track of the constant offset in the flow state. However, I don't think we've ever seen a case where this produced a real-world difference. It was mostly added to see how far we could push these queries in terms of the complexity of the findings.
The motivation for this is twofold:
cpp/overrun-write
query perform really poorly on a DCA project, and the work in this PR fixes that poor performancecpp/invalid-pointer-deref
to the point where it could almost go into the Code Scanning (but not quite 😭), and we should make use of those barriers in this query as well to remove the same kind of FPs.Commit-by-commit review recommended.
The 6 results removed from DCA are all FPs 🎉