Skip to content

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.

Sign up for GitHub

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

Merged
MathiasVP merged 4 commits into from
May 19, 2025
Merged

C++: Make node.asExpr() instanceof ArrayAggregateLiteral satisfiable #19511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e005a6
C++: Fix missing 'asExpr' for array aggregate literals.
MathiasVP May 16, 2025
ced1d58
C++: Accept test changes.
MathiasVP May 16, 2025
0eb5577
C++: Add change note.
MathiasVP May 16, 2025
ff11aaf
C++: Accept query test 'toString' improvements.
MathiasVP May 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cpp/ql/lib/change-notes/2025-05-16-array-aggregate-literals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
---
* Fixed a problem where `asExpr()` on `DataFlow::Node` would never return `ArrayAggregateLiteral`s.
32 changes: 32 additions & 0 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ExprNodes.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,28 @@ private module Cached {
)
}

Copy link

Copilot AI May 16, 2025

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.

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`.
*/

Copilot uses AI. Check for mistakes.
private Expr getRankedElementExpr(ArrayAggregateLiteral aggr, int rnk) {
result =
rank[rnk + 1](Expr e, int elementIndex, int position |
e = aggr.getElementExpr(elementIndex, position)
|
e order by elementIndex, position
)
}

private class LastArrayAggregateStore extends StoreInstruction {
ArrayAggregateLiteral aggr;

LastArrayAggregateStore() {
exists(int rnk |
this.getSourceValue().getUnconvertedResultExpression() = getRankedElementExpr(aggr, rnk) and
not exists(getRankedElementExpr(aggr, rnk + 1))
)
}

ArrayAggregateLiteral getArrayAggregateLiteral() { result = aggr }
}

private Expr getConvertedResultExpressionImpl0(Instruction instr) {
// IR construction inserts an additional cast to a `size_t` on the extent
// of a `new[]` expression. The resulting `ConvertInstruction` doesn't have
Expand Down Expand Up @@ -95,6 +117,16 @@ private module Cached {
tco.producesExprResult() and
result = asDefinitionImpl0(instr)
)
or
// IR construction breaks an array aggregate literal `{1, 2, 3}` into a
// sequence of `StoreInstruction`s. So there's no instruction `i` for which
// `i.getUnconvertedResultExpression() instanceof ArrayAggregateLiteral`.
// So we map the instruction node corresponding to the last `Store`
// instruction of the sequence to the result of the array aggregate
// literal. This makes sense since this store will immediately flow into
// the indirect node representing the array. So this node does represent
// the array after it has been fully initialized.
result = instr.(LastArrayAggregateStore).getArrayAggregateLiteral()
}

private Expr getConvertedResultExpressionImpl(Instruction instr) {
Expand Down
4 changes: 2 additions & 2 deletions cpp/ql/test/library-tests/dataflow/asExpr/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@ void test_aggregate_literal() {

S s5 = {.a = 1, .b = 2}; // $ asExpr=1 asExpr=2 asExpr={...}

int xs[] = {1, 2, 3}; // $ asExpr=1 asExpr=2 asExpr=3 MISSING: asExpr={...}
const int ys[] = {[0] = 4, [1] = 5, [0] = 6}; // $ asExpr=4 asExpr=5 asExpr=6 MISSING: asExpr={...}
int xs[] = {1, 2, 3}; // $ asExpr=1 asExpr=2 asExpr=3 asExpr={...}
const int ys[] = {[0] = 4, [1] = 5, [0] = 6}; // $ asExpr=4 asExpr=5 asExpr=6 asExpr={...}
}
16 changes: 8 additions & 8 deletions cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/consts/NonConstantFormat.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ edges
| consts.cpp:24:7:24:9 | **gv1 | consts.cpp:25:2:25:4 | *a | provenance | |
| consts.cpp:24:7:24:9 | **gv1 | consts.cpp:30:9:30:14 | *access to array | provenance | |
| consts.cpp:24:7:24:9 | **gv1 | consts.cpp:123:2:123:12 | *... = ... | provenance | |
| consts.cpp:25:2:25:4 | *a | consts.cpp:26:2:26:4 | *b | provenance | |
| consts.cpp:26:2:26:4 | *b | consts.cpp:24:7:24:9 | **gv1 | provenance | |
| consts.cpp:25:2:25:4 | *a | consts.cpp:26:2:26:4 | *{...} | provenance | |
| consts.cpp:26:2:26:4 | *{...} | consts.cpp:24:7:24:9 | **gv1 | provenance | |
| consts.cpp:29:7:29:25 | **nonConstFuncToArray | consts.cpp:126:9:126:30 | *call to nonConstFuncToArray | provenance | |
| consts.cpp:30:9:30:14 | *access to array | consts.cpp:29:7:29:25 | **nonConstFuncToArray | provenance | |
| consts.cpp:85:7:85:8 | gets output argument | consts.cpp:86:9:86:10 | *v1 | provenance | |
Expand All @@ -14,7 +14,7 @@ edges
| consts.cpp:85:7:85:8 | gets output argument | consts.cpp:129:19:129:20 | *v1 | provenance | |
| consts.cpp:85:7:85:8 | gets output argument | consts.cpp:135:9:135:11 | *v10 | provenance | TaintFunction |
| consts.cpp:90:2:90:14 | *... = ... | consts.cpp:91:9:91:10 | *v2 | provenance | |
| consts.cpp:90:2:90:14 | *... = ... | consts.cpp:115:21:115:22 | *v2 | provenance | |
| consts.cpp:90:2:90:14 | *... = ... | consts.cpp:115:21:115:22 | *{...} | provenance | |
| consts.cpp:90:7:90:10 | *call to gets | consts.cpp:90:2:90:14 | *... = ... | provenance | |
| consts.cpp:90:12:90:13 | gets output argument | consts.cpp:94:13:94:14 | *v1 | provenance | |
| consts.cpp:90:12:90:13 | gets output argument | consts.cpp:99:2:99:8 | *... = ... | provenance | |
Expand All @@ -28,9 +28,9 @@ edges
| consts.cpp:106:13:106:19 | *call to varFunc | consts.cpp:107:9:107:10 | *v5 | provenance | |
| consts.cpp:111:2:111:15 | *... = ... | consts.cpp:112:9:112:10 | *v6 | provenance | |
| consts.cpp:111:7:111:13 | *call to varFunc | consts.cpp:111:2:111:15 | *... = ... | provenance | |
| consts.cpp:115:17:115:18 | *v1 | consts.cpp:115:21:115:22 | *v2 | provenance | |
| consts.cpp:115:21:115:22 | *v2 | consts.cpp:116:9:116:13 | *access to array | provenance | |
| consts.cpp:115:21:115:22 | *v2 | consts.cpp:120:2:120:11 | *... = ... | provenance | |
| consts.cpp:115:17:115:18 | *v1 | consts.cpp:115:21:115:22 | *{...} | provenance | |
| consts.cpp:115:21:115:22 | *{...} | consts.cpp:116:9:116:13 | *access to array | provenance | |
| consts.cpp:115:21:115:22 | *{...} | consts.cpp:120:2:120:11 | *... = ... | provenance | |
| consts.cpp:120:2:120:11 | *... = ... | consts.cpp:121:9:121:10 | *v8 | provenance | |
| consts.cpp:123:2:123:12 | *... = ... | consts.cpp:24:7:24:9 | **gv1 | provenance | |
| consts.cpp:129:19:129:20 | *v1 | consts.cpp:130:9:130:10 | *v9 | provenance | |
Expand All @@ -39,7 +39,7 @@ edges
nodes
| consts.cpp:24:7:24:9 | **gv1 | semmle.label | **gv1 |
| consts.cpp:25:2:25:4 | *a | semmle.label | *a |
| consts.cpp:26:2:26:4 | *b | semmle.label | *b |
| consts.cpp:26:2:26:4 | *{...} | semmle.label | *{...} |
| consts.cpp:29:7:29:25 | **nonConstFuncToArray | semmle.label | **nonConstFuncToArray |
| consts.cpp:30:9:30:14 | *access to array | semmle.label | *access to array |
| consts.cpp:85:7:85:8 | gets output argument | semmle.label | gets output argument |
Expand All @@ -60,7 +60,7 @@ nodes
| consts.cpp:111:7:111:13 | *call to varFunc | semmle.label | *call to varFunc |
| consts.cpp:112:9:112:10 | *v6 | semmle.label | *v6 |
| consts.cpp:115:17:115:18 | *v1 | semmle.label | *v1 |
| consts.cpp:115:21:115:22 | *v2 | semmle.label | *v2 |
| consts.cpp:115:21:115:22 | *{...} | semmle.label | *{...} |
| consts.cpp:116:9:116:13 | *access to array | semmle.label | *access to array |
| consts.cpp:120:2:120:11 | *... = ... | semmle.label | *... = ... |
| consts.cpp:121:9:121:10 | *v8 | semmle.label | *v8 |
Expand Down
6 changes: 3 additions & 3 deletions cpp/ql/test/query-tests/Security/CWE/CWE-319/UseOfHttp/UseOfHttp.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ edges
| test.cpp:28:10:28:29 | *http://example.com | test.cpp:11:26:11:28 | *url | provenance | |
| test.cpp:35:23:35:42 | *http://example.com | test.cpp:35:23:35:42 | *http://example.com | provenance | |
| test.cpp:35:23:35:42 | *http://example.com | test.cpp:39:11:39:15 | *url_l | provenance | |
| test.cpp:36:26:36:45 | *http://example.com | test.cpp:36:26:36:45 | *http://example.com | provenance | |
| test.cpp:36:26:36:45 | *http://example.com | test.cpp:40:11:40:17 | *access to array | provenance | |
| test.cpp:36:26:36:45 | *http://example.com | test.cpp:36:26:36:45 | *{...} | provenance | |
| test.cpp:36:26:36:45 | *{...} | test.cpp:40:11:40:17 | *access to array | provenance | |
| test.cpp:38:11:38:15 | *url_g | test.cpp:11:26:11:28 | *url | provenance | |
| test.cpp:39:11:39:15 | *url_l | test.cpp:11:26:11:28 | *url | provenance | |
| test.cpp:40:11:40:17 | *access to array | test.cpp:11:26:11:28 | *url | provenance | |
Expand All @@ -29,7 +29,7 @@ nodes
| test.cpp:35:23:35:42 | *http://example.com | semmle.label | *http://example.com |
| test.cpp:35:23:35:42 | *http://example.com | semmle.label | *http://example.com |
| test.cpp:36:26:36:45 | *http://example.com | semmle.label | *http://example.com |
| test.cpp:36:26:36:45 | *http://example.com | semmle.label | *http://example.com |
| test.cpp:36:26:36:45 | *{...} | semmle.label | *{...} |
| test.cpp:38:11:38:15 | *url_g | semmle.label | *url_g |
| test.cpp:39:11:39:15 | *url_l | semmle.label | *url_l |
| test.cpp:40:11:40:17 | *access to array | semmle.label | *access to array |
Expand Down