C++: Make node.asExpr() instanceof ArrayAggregateLiteral satisfiable
#19511
Conversation
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;
| exists(int rnk | | ||
| this.getSourceValue().getUnconvertedResultExpression() = getRankedElementExpr(aggr, rnk) and | ||
| not exists(getRankedElementExpr(aggr, rnk + 1)) |
There was a problem hiding this comment.
Field aggr is used in the LastArrayAggregateStore constructor but never bound to a specific ArrayAggregateLiteral. As a result, getArrayAggregateLiteral() may return an uninitialized value. Consider capturing the literal in the exists predicate (e.g., exists(ArrayAggregateLiteral lit | ... and this.aggr = lit)) or otherwise assigning aggr explicitly.
| exists(int rnk | | |
| this.getSourceValue().getUnconvertedResultExpression() = getRankedElementExpr(aggr, rnk) and | |
| not exists(getRankedElementExpr(aggr, rnk + 1)) | |
| exists(ArrayAggregateLiteral lit, int rnk | | |
| this.aggr = lit and | |
| this.getSourceValue().getUnconvertedResultExpression() = getRankedElementExpr(lit, rnk) and | |
| not exists(getRankedElementExpr(lit, rnk + 1)) |
Copilot uses AI. Check for mistakes.
| @@ -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.
| /** | |
| * 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`. | |
| */ |
Copilot uses AI. Check for mistakes.
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.getLocation()as 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!