C++: Switch to the shared Guards library #20485

MathiasVP · 2025-09-18T09:23:02Z

This PR switches C/C++ away from our own guards library and over to the shared guards library which was introduced in #19573.

The main motivation for this is two-fold: It reduces the amount of C/C++ specific QL code that needs to be maintained ¹. It also enables C/C++ to instantiate the newly shared "nullness" library which was introduced in #20367. This should hopefully open the doors to better null dereference / use-after-free / double free queries.

On top of that, the improvements to the internal guards logic also means much better query results on existing queries. In particular, the cpp/missing-check-scanf query has lost a tons of FPs 🎉.

Commit-by-commit review extremely recommended. It's a large PR, but I actually think I managed to make it fairly reviewable 🤞.

This PR does bring a syntactic breaking changes which was just decided a couple of weeks ago would be okay for CodeQL going forward. The breaking change is that the AST wrapper around the IR-based guards library now extends Element instead of Expr. This is because the shared library also infers that Parameters (which are not Exprs in the AST) can be guards if the parameter determines a condition.

In fact, whereas the old library only made the condition (and certain subexpressions inside the condition) were Guards, the new library makes every expression a Guard, but only some of those Guards actually controls conditions. This is a fundamental difference that's always been between the C/C++ guards library and the Java/C# guards library, and we're finally getting rid of that difference which brings some more language consistency 🎉.

Although, there is still lots of maintenance required because C/C++ still relies on our own implication of ensuresEq and ensuresLt. Getting rid of those requires switching C/C++ fully over to the shared range analysis library. ↩

Copilot

Pull Request Overview

This PR switches the C++ CodeQL library from its own guards library to the shared guards library that was introduced previously. The main purpose is to reduce C++-specific QL code maintenance and enable C++ to use the shared "nullness" library. This change also improves query results on existing queries, particularly reducing false positives in the cpp/missing-check-scanf query.

Key changes include:

Replacing the old C++-specific guards implementation with the shared guards library
Updating guard value types from AbstractValue to GuardValue
Making guards extend Element instead of Expr to support parameters as guards
Improving the guards logic to provide better query results

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
GuardsEnsure.expected	Updated test expectations showing improved guard inference with more comprehensive results
GuardsControl.ql	Changed parameter type from `AbstractValue` to `GuardValue`
GuardsControl.expected	Expanded test results with additional guard control relationships
GuardsCompare.ql	Updated parameter type from `AbstractValue` to `GuardValue`
GuardsCompare.expected	Extended comparison results with more comprehensive guard comparisons
Guards.ql	Removed simple guard selection query
Guards.expected	Removed corresponding test expectations
tests.ql	Updated parameter types to use `GuardValue`
TOCTOUFilesystemRace.ql	Fixed guard child access by casting to `Expr`
SSLResultConflation.ql	Fixed guard child access by casting to `Expr`
SemanticExprSpecific.qll	Updated method call from `controlsEdge` to `controlsBranchEdge`
Instruction.qll	Added `getAnInput()` method to `BinaryInstruction`
EdgeKind.qll	Enhanced switch edge handling with better value range support
SsaImpl.qll	Major updates to SSA implementation with improved guard integration
IRGuards.qll	Complete rewrite using shared guards library with expanded functionality
RangeAnalysis.qll	Updated method call to use `controlsBranchEdge`

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll

+  }
+
+  private import semmle.code.cpp.dataflow.new.DataFlow::DataFlow as DataFlow
+  private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate as Private


…nIndirectUse'. This will solve a non-monotonic recursion issue later.

…rk after instantiating the shared guards library.

…rary needs.

… a guard condition.

…sion.

aschackmull · 2025-09-18T11:18:14Z

This is because the shared library also infers that Parameters (which are not Exprs in the AST) can be guards if the parameter determines a condition

No. Parameters can't be Guards. But case statements can - and that's what brings Guards beyond mere Exprs.

MathiasVP · 2025-09-18T11:22:42Z

No. Parameters can't be Guards. But case statements can - and that's what brings Guards beyond mere Exprs.

Oh. I guess there's a subtle thing for C/C++ here then: In the IR world a parameter is an Instruction (specifically, it's an InitializeParameter instruction). And since Instructions are Exprs in the new guards library this means the new guards library can infer that InitializeParameter can be a guard ... and the InitializeParameter maps back to the AST-world as a Parameter.

Is that ... bad?

MathiasVP · 2025-09-18T11:41:26Z

Phew! Thanks for sanity checking that! I'll pull this PR out of draft in a second once CI has finished the last check. I chatted with @jketema earlier and he's hoping that you can chime in on the reviewing on this PR. I guess primarily 840097f. Would you mind doing that at some point? 🙏

aschackmull · 2025-09-18T11:43:10Z

Would you mind doing that at some point? 🙏

Sure, I'll take a look (not today, though).

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll

aschackmull · 2025-09-24T09:35:57Z

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll

+
+private module GuardsImpl = SharedGuards::Make<Cpp::Location, IRCfg, GuardsInput>;
+
+private module LogicInput_v1 implements GuardsImpl::LogicInputSig {


If we only have _v1 then the suffix seems somewhat superfluous, but I assume you perhaps intend to add a _v2, which adds the output from Range Analysis? I.e. let's keep the _v1 suffix.

Yeah, eventually I will add a _2 😄 However, I wanted to keep things as semantically identical as I could for this first PR.

aschackmull · 2025-09-24T09:45:56Z

Could you perhaps add a new test with a bunch of complex guard implications to showcase that these are indeed recognized? E.g. something like this and a bunch of other cases:

x = notNull(y) ? 42 : 0;
if (x != 0) {
  // guarded by `y != null`
}

And a test case for data flow that shows that barrier guard wrappers are now supported.

…itionalImpliesStep.

…senting a gcc case range is a bad idea.

…ent 'rangeGuard'.

…n' when detecting return expressions.

MathiasVP · 2025-09-24T19:12:51Z

Could you perhaps add a new test with a bunch of complex guard implications to showcase that these are indeed recognized? E.g. something like this and a bunch of other cases:
x = notNull(y) ? 42 : 0;
if (x != 0) {
  // guarded by `y != null`
}
And a test case for data flow that shows that barrier guard wrappers are now supported.

Thanks a lot for reminding me to add those tests! I've added a bunch in b2269fb. That commit also highlights some missing inferences, and 26a8a4b also demonstrated that something was totally broken with the guard wrapper implementation.

Luckily, the fix was easy: a07d03f. With that change the missing inferences are now fixed, and the barrier guard wrappers also work 🎉.

I'll start another DCA to see if this has any effects on real-world projects.

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll

aschackmull · 2025-09-25T07:53:26Z

cpp/ql/test/library-tests/controlflow/guards/test.cpp

+void test_ternary(void* y) {
+  int x = testNotNull1(y) ? 42 : 0;
+  if (x != 0) {
+    chk(); // $ guarded='... ? ... : ...:not 0' guarded='42:not 0' guarded='call to testNotNull1:true' guarded='x != 0:true' guarded='x:not 0' guarded='y:not null'


Hmm, we shouldn't see guarded='42:not 0' as that's a trivial tautology. But I think perhaps the fix for that is tucked away in one of my local branches...

Yeah, I was wondering whether there was any filtering on such trivial implications as well, but I couldn't find anything in the current library.

aschackmull · 2025-09-25T07:56:06Z

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll

+        // We use the `Store` instruction that writes the return value instead of the
+        // `ReturnValue` instruction since the `ReturnValue` instruction is not always
+        // dominated by certain guards. For example:
+        // ```
+        // if(b) {
+        //   return true;
+        // } else {
+        //   return false;
+        // }
+        // ```
+        // this will be translated into IR like:
+        // ```
+        // if(b) {
+        //   x = true;
+        // } else {
+        //   x = false;
+        // }
+        // return x;
+        // ```


I'm somewhat confused by this. Both code snippets should be equally well supported as wrapper guards, so if the latter doesn't work then something else is missing. Make sure you have test cases for both kinds of wrapper guards.

Ah, wait. I think I know what was wrong with the old approach. Nothing to see here 🙈! I've deleted the incorrect comment and added a bunch of examples in c1c1f60. They all seem to work perfectly fine 🎉

MathiasVP · 2025-09-29T14:06:06Z

@aschackmull thanks for all the review comments so far! I think I've fixed all of them, and DCA still looks fine. Do you have any other comments before I hand it over to the C team?

C++: Make the return type a bit more precise.

f55ace8

MathiasVP requested a review from a team as a code owner September 18, 2025 09:23

MathiasVP requested review from Copilot and removed request for a team September 18, 2025 09:23

github-actions bot added the C++ label Sep 18, 2025

Copilot AI reviewed Sep 18, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll Show resolved Hide resolved

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll Show resolved Hide resolved

github-advanced-security bot found potential problems Sep 18, 2025

View reviewed changes

github-actions bot added the documentation label Sep 18, 2025

MathiasVP marked this pull request as draft September 18, 2025 09:52

MathiasVP added 10 commits September 18, 2025 11:05

C++: Add a 'hasInputFromBlock' predicate.

f47dd2b

C++: Do not rely on dataflow nodes when implement 'getAUse' and 'getA…

be8195a

…nIndirectUse'. This will solve a non-monotonic recursion issue later.

C++: Add a few subclasses to 'EdgeKind'.

19b8e0d

C++: Add another convenience predicate on 'BinaryInstruction'.

70a8364

C++: Sync identical files.

f0f4311

C++: Delete a bunch of predicates. Also set up things so that they wo…

2b0b840

…rk after instantiating the shared guards library.

C++: Qualified import of cpp.

3a8f77d

C++: Instantiate the shared guards library.

840097f

C++: Lots of renamings.

ac5233d

C++: Add a few helper predicates on 'Definition' which the guards lib…

9ed8b75

…rary needs.

MathiasVP force-pushed the use-shared-guards-library branch from c99ab52 to 6fe3e83 Compare September 18, 2025 10:21

MathiasVP added 8 commits September 18, 2025 12:16

C++: Fixup tests.

0b81fbb

C++: Accept test changes.

2dc783d

C++: Delete a test. This is no longer useful when every expression is…

fbd877a

… a guard condition.

C++: Fixup queries which assumes that a guard is always an expression.

5e82eb9

C++: Do not use the deprecated predicate in queries.

d8f34db

C++: Fix barrier guards.

e22d665

C++: Add change note.

a4dbee3

C++: Accept test changes for tests that select all types of an expres…

c481be8

…sion.

MathiasVP marked this pull request as ready for review September 18, 2025 11:59

aschackmull reviewed Sep 24, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 24, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Show resolved Hide resolved

aschackmull reviewed Sep 24, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 24, 2025

View reviewed changes

C++: Add QLDoc to TRange.

b3cbdb5

MathiasVP added 6 commits September 24, 2025 10:51

C++: Remove superfluous inference logic.

2b47ac8

C++: Add a comment to the 'ConditionalBranchInstruction' case in 'add…

99e1a07

…itionalImpliesStep.

C++: Rename in the name of consistency.

d10d5fd

C++: Add testcase demonstrating that a ConstantValue IPA branch repre…

13cde4d

…senting a gcc case range is a bad idea.

C++: Get rid of the case range constant value with and instead implem…

d15e388

…ent 'rangeGuard'.

C++: Add more tests.

b2269fb

MathiasVP force-pushed the use-shared-guards-library branch from dad3413 to b2269fb Compare September 24, 2025 18:06

MathiasVP added 2 commits September 24, 2025 19:54

C++: Add failing test demonstrating broken wrapper barrier guards.

26a8a4b

C++: Use the 'StoreInstruction' instead of the 'ReturnValueInstructio…

a07d03f

…n' when detecting return expressions.

MathiasVP force-pushed the use-shared-guards-library branch from 74799e5 to a07d03f Compare September 24, 2025 19:10

aschackmull reviewed Sep 25, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Show resolved Hide resolved

aschackmull reviewed Sep 25, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 25, 2025

View reviewed changes

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Show resolved Hide resolved

C++: Fully delete TRange.

b169ccf

aschackmull reviewed Sep 25, 2025

View reviewed changes

MathiasVP added 3 commits September 25, 2025 12:33

C++: Fix fan-out.

5b5c1de

C++: Add more comments to describe the constant expression hack.

04ce405

C++: Delete incorrect comment and add a bunch of barrier guard tests.

c1c1f60


		private module GuardsImpl = SharedGuards::Make<Cpp::Location, IRCfg, GuardsInput>;

		private module LogicInput_v1 implements GuardsImpl::LogicInputSig {

C++: Switch to the shared Guards library #20485

Are you sure you want to change the base?

C++: Switch to the shared Guards library #20485

Conversation

MathiasVP commented Sep 18, 2025

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Check warning

aschackmull commented Sep 18, 2025

Uh oh!

MathiasVP commented Sep 18, 2025

Uh oh!

MathiasVP commented Sep 18, 2025

Uh oh!

aschackmull commented Sep 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aschackmull Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

MathiasVP Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Sep 24, 2025

Uh oh!

MathiasVP commented Sep 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aschackmull Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MathiasVP Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

aschackmull Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MathiasVP Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MathiasVP commented Sep 29, 2025 •

edited

Loading