C++: Modify tests for an upcoming PR by MathiasVP · Pull Request #22990 · github/codeql

MathiasVP · 2026-11-03T09:32:41Z

This PR moves the test-related changes from #22939 into its own PR (as requested by @jketema).

Since no production code changes I don't think a DCA run is necessary.

… using 'char*' instead of 'const char*' which meant it would not type check when attempting to use string literals as keys (if we had been using a real implementation of those containers).

…nce it in models. Ideally, we should have a synthetic fields in dataflow for this instead. But this PR is large enough as it is, and this won't be a problem in real code as the member will always exist.

``` std::pair<std::pair<const char *, const char *>, const char *> m; ... sink(m); ``` and we only had a sink type for `std::pair<const char *, const char *>`.

… that templated constructors are instantiated. So this commit ensures that all the constructors that were not previously instantiated are now instantiated.

In `map.cpp` we had: ``` char *source(); ... std::unordered_map<const char *, std::pair<int, int>> m31; ... sink(m31.try_emplace("abc", source(), 2)); ``` which was incorrect because `source()` returns a `char*` and we're trying to stick it into a constructor for `std::pair<int, int>`.

…licated std::string declarations in 'azure.cpp' and 'bslx.cpp'.

…k flow on all integral types (which includes char-like types). Also add some MaD pretty-printing.

…ccept test changes.

Copilot

🟡 Changes recommended

A critical finding remains because the expected output file is not updated for the changed postprocessor output order, causing the query test to fail.

1 open issue

Regenerate expected output for the postprocessor order · New

What changed in this PR

This PR isolates C++ test updates for the upcoming STL dataflow-model migration. It expands taint and security-test coverage, consolidates shared test stubs, and refreshes generated expectations.

Changes:

Adds coverage for std::string_view, string flows, maps, pairs, unordered maps, and cleartext storage.
Replaces duplicated STL and type-traits test stubs with shared definitions.
Updates query references and generated model, flow, source, sink, and validation expectations.

The review identified one critical issue (2 votes) involving stale ExecTainted.expected output ordering, plus three moderate issues (1 vote each) involving assignment-result coverage, a synthetic string_view::subview API, and missing indirect-pointer string-view coverage.

File	Reviewed changes and findings
`cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/test2.cpp`	Adds cleartext-flow coverage for strings, character iteration, and references.
`cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextFileWrite.qlref`	Adds model pretty-printing to the query test.
`cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextFileWrite.expected`	Updates generated cleartext-flow expectations.
`cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/ExecTainted.qlref`	Adds model pretty-printing; critical (2 votes): the corresponding expected output must be regenerated to reflect the changed output order.
`cpp/ql/test/library-tests/dataflow/taint-tests/type_traits.h`	Removes the obsolete local type-traits stub.
`cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected`	Updates generated signature and type-resolution expectations.
`cpp/ql/test/library-tests/dataflow/taint-tests/string.cpp`	Adds string-view and reverse-flow coverage; moderate (1 vote): replace the synthetic `basic_string_view::subview` case with a real API or remove it; add an `indirect_source()` iterator-constructor case.
`cpp/ql/test/library-tests/dataflow/taint-tests/stl.h`	Expands shared STL declarations for strings, string views, containers, and related operations.
`cpp/ql/test/library-tests/dataflow/taint-tests/map.cpp`	Updates map and pair tests; moderate (1 vote): exercise the map and unordered-map assignment expressions themselves, including their returned container/reference access paths.
`cpp/ql/test/library-tests/dataflow/external-models/validatemodels.expected`	Updates generated model-validation diagnostics.
`cpp/ql/test/library-tests/dataflow/external-models/steps.expected`	Updates generated external-model flow steps.
`cpp/ql/test/library-tests/dataflow/external-models/std_string.h`	Removes the obsolete external-model string stub.
`cpp/ql/test/library-tests/dataflow/external-models/sources.expected`	Updates generated external-source expectations.
`cpp/ql/test/library-tests/dataflow/external-models/sinks.expected`	Updates generated external-sink expectations.
`cpp/ql/test/library-tests/dataflow/external-models/protobuf.cpp`	Reuses the shared STL stub for protobuf tests.
`cpp/ql/test/library-tests/dataflow/external-models/bslx.cpp`	Reuses the shared STL stub for BSLX tests.
`cpp/ql/test/library-tests/dataflow/external-models/azure.cpp`	Reuses shared STL declarations and adds query-parameter coverage.
`cpp/ql/test/library-tests/dataflow/external-models/asio_streams.cpp`	Reuses shared STL and string-view declarations for Asio tests.
`cpp/ql/test/include/type_traits.h`	Provides shared global and `std::size_t` definitions for test stubs.

_{Review effort: Lite (auto)}

jketema · 2026-11-03T10:37:19Z

-	sink(resolver.resolve(host_view, service_view, error)); // $ ir
-	sink(resolver.resolve(host_view, service_view, flags)); // $ ir
-	sink(resolver.resolve(host_view, service_view, flags, error)); // $ ir
+	sink(resolver.resolve(host_view, service_view)); // $ MISSING: ir


Why do these swap over to missing?

Oh that's a good question. It took me some time to figure out what's going on here. I apologize for not making that clear in the commit why that is.

On main the string_view class declaration that's imported by asio_streams is very simplistic:

codeql/cpp/ql/test/library-tests/dataflow/external-models/asio_streams.cpp

Lines 8 to 13 in 81a1deb

namespace std {

class string_view {

public:

string_view(const char* s);

};

};

In 18200b7 we make the string_view much more realistic. In particular, we make the 6'th constructor from cppreference explicit. This has the effect of removing the generic taint-model we have for this constructor. So that's why we're losing the result.

jketema · 2026-11-03T10:41:06Z

 }

 void test_GetHeader() {
+  std::pair<std::string, std::string>();


This looks weird, as it just adds a constructor call.

See the commit message here: 5f676c6

Does that explain it?

So it uses that somewhere under the hood, but doesn't instantiate it there?

In this case we need the class to be instantiated since we need a specific FieldContent to be present (for this piece of code).

There are two related problems that are being solved by instantiating the std::map<std::string, std::string> constructor here.

The main problem is that we've stubbed the body of the Azure::Core::Http::Request class. In a real implementation there would be a std::map<std::string, std::string> somewhere in that class (since such a value is returned by GetHeaders). So somewhere in that class where would be an instantiation which would make the members appear in the DB. However, as we only have declarations in our tests these members aren't instantiated.

So this commit adds the instantiations that were missing from the test database.

In this case we need it because #22939 will model std::map flow using a pair of Contents (like C# models them): a std::pair::second field content, and an ElementContent. The problem is that the std::pair::second field content doesn't exist in the database unless a the templated std::pair class is actually instantiated somewhere in the database.

There are other places where we need the constructor to be present since #22939 will model certain insertion-like STL member functions by forwarding the container insertion arguments to the corresponding constructor, and if the constructor isn't in the DB we cannot do that. Again, this would not be a problem in a real DB since those constructors will always exist, but with our tests stubs that won't happen since we only have declarations of the insertion-like functions and not the actual bodies.

Does that make sense?

Yeah. Makes sense. I see that in cpp/ql/test/library-tests/dataflow/taint-tests/map.cpp you added a comment when you did something similar, could you do that here too?

Added in 0854cc0

jketema · 2026-11-03T10:44:21Z

 	sink(make_pair("123", "456").first);
 	sink(make_pair("123", "456").second);
-	sink(make_pair(source(), "456")); // $ MISSING: ast,ir
+	sink(make_pair(source(), "456")); // $ ir MISSING: ast


Why this change?

This happens because of dc46def. Before that commit, the sink had to be of type std::pair<const char*, const char*> so this converting constructor would be invoked before reaching the sink. And we don't have models for std::pair's constructors.

After dc46def we allow any pair<T, U> as a sink argument and thus no conversion takes place.

jketema

Few small questions above, otherwise this LGTM.

MathiasVP · 2026-11-03T14:54:02Z

@jketema I believe I've addressed your questions! Thanks for the great comments 🙇

jketema

LGTM. Thanks for the clarifications!

Agreed that this doesn't need DCA.

MathiasVP added 15 commits October 22, 2026 17:43

C++: Fix const'ness in the map tests. The containers were incorrectly…

e6b9042

… using 'char*' instead of 'const char*' which meant it would not type check when attempting to use string literals as keys (if we had been using a real implementation of those containers).

C++: Make map in the azure test more realistic.

b63ce1d

C++: Make strings in the azure test more realistic.

f9e4a41

C++: Make sure 'Nullable' actually has a member so that we can refere…

dd2c720

…nce it in models. Ideally, we should have a synthetic fields in dataflow for this instead. But this PR is large enough as it is, and this won't be a problem in real code as the member will always exist.

C++: Add missing map-related APIs.

e00b975

C++: Add more string tests.

18200b7

C++: Accept test changes.

77f9f05

C++: Add a test that uses 'SetQueryParameters'.

13a069f

C++: Fix sink argument types. In the map.cpp tests we have:

dc46def

``` std::pair<std::pair<const char *, const char *>, const char *> m; ... sink(m); ``` and we only had a sink type for `std::pair<const char *, const char *>`.

C++: Ensure instantiation. In an upcoming PR it's important to ensure…

5f676c6

… that templated constructors are instantiated. So this commit ensures that all the constructors that were not previously instantiated are now instantiated.

C++: Big cleanup of declarations in the taint-tests to get rid of dup…

60f268c

…licated std::string declarations in 'azure.cpp' and 'bslx.cpp'.

C++: Accept test changes.

eb60024

C++: Add some tests. The missing result is from the fact that we bloc…

620bdf1

…k flow on all integral types (which includes char-like types). Also add some MaD pretty-printing.

C++: Pretty print MaD summaries in 'cpp/command-line-injection' and a…

e20cd3f

…ccept test changes.

Copilot AI auto review requested due to automatic review settings November 3, 2026 09:32

MathiasVP requested a review from a team as a code owner November 3, 2026 09:32

Copilot started reviewing on behalf of MathiasVP November 3, 2026 09:33

View session

MathiasVP added no-change-note-required This PR does not need a change note C++ labels Nov 3, 2026

Copilot AI reviewed Nov 3, 2026

View reviewed changes

Comment thread cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/ExecTainted.qlref Resolved

jketema reviewed Nov 3, 2026

View reviewed changes

MathiasVP added 2 commits November 3, 2026 13:27

C++: Add a comment to a test file.

0854cc0

C++: Accept test changes.

559f91e

jketema approved these changes Nov 3, 2026

View reviewed changes

MathiasVP merged commit 042fea6 into github:main Nov 3, 2026
11 checks passed

	namespace std {
	class string_view {
	public:
	string_view(const char* s);
	};
	};

Conversation

MathiasVP commented Nov 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jketema left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Nov 3, 2026

Uh oh!

jketema left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants