C++: Make node.asExpr() instanceof ArrayAggregateLiteral satisfiable#19511
Merged
MathiasVP merged 4 commits intogithub:mainfrom May 19, 2025
Merged
C++: Make node.asExpr() instanceof ArrayAggregateLiteral satisfiable#19511MathiasVP merged 4 commits intogithub:mainfrom
node.asExpr() instanceof ArrayAggregateLiteral satisfiable#19511MathiasVP merged 4 commits intogithub:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
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;
| @@ -45,6 +45,28 @@ private module Cached { | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[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.
Suggested change
| /** | |
| * 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`. | |
| */ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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!