-
Notifications
You must be signed in to change notification settings - Fork 1.9k
C++: Make node.asExpr() instanceof ArrayAggregateLiteral satisfiable
#19511
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
C++: Make node.asExpr() instanceof ArrayAggregateLiteral satisfiable
#19511
Conversation
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
Adds support for treating the final store of an array aggregate literal as the expression node for dataflow, updates tests to expect the new {...} placeholder, and records the change in the release notes.
- Introduce
getRankedElementExprandLastArrayAggregateStoreinExprNodes.qllto map the last store instruction back to itsArrayAggregateLiteral. - Update expected test outputs in several
.expectedfiles to include*{...}orasExpr={...}where the full aggregate literal is now recognized. - Add a change note documenting the fix to
asExpr()for array aggregates.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ExprNodes.qll | Add helper to pick the last element store and a class to link it back to the array literal |
| cpp/ql/test/library-tests/dataflow/asExpr/test.cpp | Update test cases to expect asExpr={...} for array aggregate literals |
| cpp/ql/test/query-tests/Security/CWE/CWE-319/UseOfHttp/UseOfHttp.expected | Replace literal values with {...} in flows for array writes |
| cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/consts/NonConstantFormat.expected | Replace intermediate nodes with {...} for array writes |
| cpp/ql/lib/change-notes/2025-05-16-array-aggregate-literals.md | Document the fix for asExpr() on ArrayAggregateLiteral |
Comments suppressed due to low confidence (1)
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ExprNodes.qll:58
- [nitpick] The field name
aggris abbreviated and may be unclear. Consider renaming it toarrayLiteraloraggregateLiteralto improve readability.
ArrayAggregateLiteral aggr;
| result = getConvertedResultExpression(operand.getDef(), n - 1) | ||
| ) | ||
| } | ||
|
|
Copilot
AI
May 16, 2025
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.
[nitpick] The purpose and behavior of getRankedElementExpr aren't documented. Adding a brief comment explaining how it selects and orders elements by index and position will aid future maintainers.
| /** | |
| * Retrieves the expression corresponding to the `rnk`-th ranked element | |
| * in the given array aggregate literal `aggr`. | |
| * | |
| * The function selects elements using the `rank` construct, which ranks | |
| * elements based on their index and position, and orders them by | |
| * `elementIndex` and `position`. | |
| */ |
jketema
left a comment
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.
LGTM
This PR fixes the same problem as #19501, but for
ArrayAggregateLiterals. Unlike in theArrayAggregateLiteralscase we don't have a post-update node to use to represent the "fully initialized literal" since array writes aren't handled using post-update nodes. So we can't do quite the same in this case.In #19501 I said that we should fix the
ArrayAggregateLiteralsby making use of post-update nodes for array writes. However, that's a really hard task that I don't actually feel like tackling right now 😅However, we can do something that's almost as good: We can use the last
Storeinstruction as the dataflow node for whichnode.asExpr() instanceof ArrayAggregateLiteralshould hold. This is because, at this point, the literal has been fully initialized and the memory pointed to by the array will contain (among other things) the last expression that was written to it.This gives us exactly the flow we want. Sadly, the location isn't quite right. In the below example I track flow from:
to
and as you can see the location of the source is the location of the last expression in the literal:
I plan on fixing this as a follow-up. The problem is that we don't have the same logic in place to customize the behavior of
Node.getLocationas we do forNode.asExpr. Ideally, the result ofNode.getLocation()should be location ofNode.asExpr(), if any (as we do forNode.toString). But I'll leave that for a subsequent PR!