SSA: Tolerate multiple variables being read at the same CFG node by asgerf · Pull Request #22744 · github/codeql

asgerf · 2026-10-02T11:51:59Z

Fixes an issue with the SSA data-flow integration library that occurs when multiple variables are read at the same CFG node.

This situation happens when it is instantiated from the VariableCapture library. It also happens in unified due to how we use synthetic reads to mark post-update positions.

The assumption has a subtle presence in this bit of code:

    private class ReadNodeImpl extends ExprNodeImpl {
      private BasicBlock bb_;
      private int i_;

      ReadNodeImpl() {
        variableRead(bb_, i_, v_, true) and
        this.getExpr().hasCfgNode(bb_, i_)
      }

Since this is only bound to the expression, the variableRead join can fan out to multiple variables when not uniquely determined by the CFG node. Even if Expr is constructed to be unique to the variable, the correlation is forgotten in this join and they get mixed up anyway. The fix was to make TExprNode unique to a specific variable.

The real culprit is the Expr class from the data-flow integration input. I propose we remove Expr entirely and replace ExprNode with ReadNode: a canonical representative for a (bb,i,v) triple from the variableRead input, and likewise for PostUpdateNode. But such a change is too large for this PR as it requires language-specific refactorings. But the fix is essentially a precursor to this solution: when Expr is a CFG node, ReadNode is effectively a (bb,i,v) triple as it rightfully should be.

+      /**
+       * The given `read` reads multiple variables at once. `var` is bound to one of them.
+       */


These consistency violations originate from the VariableCapture instantation in C#, JS, Python, and Ruby.

The previously-added consistency errors are gone.

Unified also had consistency errors from its LocalSSA instantiation, due to its use of synthetic read nodes to represent post-update positions. Many variables can have a post-update at the same CFG node.

Many tests passed for the wrong reasons, due to the SSA bug. We need more library/operator modelling to actually find these flows.

Switched to TaintTracking and adds some very ad-hoc steps to recover most of the results. Some more tests pass and others fail; these are now consistent with what we actually model.

The C++ instantiation of DataFlowIntegration generated a bad join order

Copilot

Copilot review overview

🟡 Changes recommended

Variable correlation remains incomplete in post-update, must-flow, and barrier joins, and the new Swift URL rule overmatches user-defined calls.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Must-flow joins ignore source variable identity · New
Taint analysis trusts shadowable URL initializer names · New

What changed in this PR

This PR makes SSA data-flow nodes variable-specific when multiple variables share one CFG node and adds consistency coverage.

Changes:

Adds variable identity to SSA expression nodes and ambiguity checks.
Adds Swift regression tests and updates path-injection taint flow.
Enables the new consistency check across supported languages.

File	Description
`shared/ssa/codeql/ssa/Ssa.qll`	Qualifies expression nodes by variable and adds consistency checks.
`shared/dataflow/codeql/dataflow/VariableCapture.qll`	Exposes SSA ambiguity checks for variable capture.
`unified/ql/consistency-queries/LocalSsaConsistency.ql`	Enables the new unified consistency check.
`unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll`	Adds Swift-specific taint steps.
`unified/ql/src/queries/security/CWE-022/PathInjection.ql`	Switches path injection to taint tracking.
`unified/ql/test/library-tests/dataflow/test.swift`	Adds a shared-CFG-node regression case.
`unified/ql/test/library-tests/dataflow/test.expected`	Updates generated data-flow expectations.
`unified/ql/test/query-tests/security/CWE-022/PathInjection/testPathInjection.swift`	Updates path-injection annotations.
`unified/ql/test/query-tests/security/CWE-022/PathInjection/PathInjectionTest.expected`	Updates generated path-injection results.
`rust/ql/consistency-queries/SsaConsistency.ql`	Enables SSA ambiguity checking for Rust.
`ruby/ql/consistency-queries/SsaConsistency.ql`	Enables SSA ambiguity checking for Ruby.
`java/ql/consistency-queries/SsaConsistency.ql`	Enables SSA ambiguity checking for Java.
`csharp/ql/consistency-queries/SsaConsistency.ql`	Enables SSA ambiguity checking for C#.
`python/ql/test/library-tests/dataflow/variable-capture/dataflow-capture-consistency.expected`	Updates generated consistency expectations.
`javascript/ql/test/library-tests/FlowSummary/CaptureConsistency.expected`	Updates generated consistency expectations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+      TExprNode(DfInput::Expr e, SourceVariable v, Boolean isPost) {
+        exists(Definition def |
+          def.getSourceVariable() = v and
+          e = DfInput::getARead(def)
+        )


+    exists(CallExpr call |
+      call.getCallee().(Identifier).getValue() = ["URL", "NSURL"] and
+      node1.isResultValue(call.getNamedArgument("string")) and
+      step.taint() and
+      node2.isResultValue(call)


hvitved · 2026-10-05T16:52:55Z

      /** Gets the underlying expression. */
      DfInput::Expr getExpr() { result = e }

+      /** Holds if represents the access to `var` performed at `expr`. */


aschackmull

LGTM! Thanks for fixing this.

C++ has a subtle dependency on this behavior. Previously, when a single TExprNode corresponded to multiple variables, it would be materialised if just one of those variables had a reaching definition. Now the node has been split up, which generally works fine for C++, but in some cases one of the split-off nodes is missing its reaching def, but C++ still needs to find the flow to its next use, and therefore needs the node to be materialised.

asgerf · 2026-10-08T07:58:25Z

I decided to investigate the rare loss of a source-sink pair reported by the C++ analysis, and tracked the discrepancy down to the fix described in fc2d9f3. Will run another rounds of DCA.

I also plan a small follow-up PR with some simplifications to the C++ SSA library that are made possible by this change (it currently goes a long way to work around the bug fixed in this PR).

asgerf · 2026-10-08T08:30:07Z

Taking back into draft until unit tests and DCA runs are happy

+          variableRead(bb, i, v, true) and
+          // Only materialise if 'expr' has a reaching definition.
+          // Note that the read may correspond to a different variable than 'v', but the C++
+          // instantiation currently expects this particular behaviour.


asgerf · 2026-10-08T09:03:33Z

Rerun has been triggered: 2 restarted 🚀

asgerf · 2026-10-08T12:40:12Z

PTAL. I believe I've reduced the impact for C++ without negatively affecting any other languages.

asgerf added 3 commits October 2, 2026 12:25

unified: Add test showing spurious SSA flow

3137897

Ssa: Add consistency check for ambiguous read

f906f8d

unified: Add consistency check

9d8f5e6

github-advanced-security Bot found potential problems Oct 2, 2026

View reviewed changes

Comment thread shared/ssa/codeql/ssa/Ssa.qll

Comment on lines +2150 to +2152

/**

* The given `read` reads multiple variables at once. `var` is bound to one of them.

*/

github-actions Bot added C# Java Ruby Rust Pull requests that update Rust code DataFlow Library Unified labels Oct 2, 2026

asgerf force-pushed the ssa/multiple-reads-v2 branch from f0d12fb to bfd363b Compare October 2, 2026 13:36

github-actions Bot added JS Python labels Oct 2, 2026

asgerf added 5 commits October 2, 2026 16:22

ssa: Expose new consistency check from VariableCapture.qll

b1b2b20

ssa: Record ambiguousReadNode errors from VariableCapture

b2f9e09

These consistency violations originate from the VariableCapture instantation in C#, JS, Python, and Ruby.

ssa: Associate ExprNode with a variable

8957cda

ssa: Record absense of ambiguousReadNode errors

c084707

The previously-added consistency errors are gone.

unified: Update local SSA consistency output

649e8cc

Unified also had consistency errors from its LocalSSA instantiation, due to its use of synthetic read nodes to represent post-update positions. Many variables can have a post-update at the same CFG node.

asgerf force-pushed the ssa/multiple-reads-v2 branch from bfd363b to 1bbd5d5 Compare October 2, 2026 14:23

asgerf added 3 commits October 2, 2026 16:47

unified: Update unit test output

0164aff

unified: Update path-injection output

ee7fc86

Many tests passed for the wrong reasons, due to the SSA bug. We need more library/operator modelling to actually find these flows.

unified: Mostly restore path-injection results

2c637ab

Switched to TaintTracking and adds some very ad-hoc steps to recover most of the results. Some more tests pass and others fail; these are now consistent with what we actually model.

asgerf force-pushed the ssa/multiple-reads-v2 branch 2 times, most recently from 1bbd5d5 to 2c637ab Compare October 2, 2026 14:55

asgerf changed the title ~~Ssa/multiple reads v2~~ SSA: Tolerate multiple variables being read at the same CFG node Oct 2, 2026

ssa: Fix bad join order

cbd6d35

The C++ instantiation of DataFlowIntegration generated a bad join order

asgerf force-pushed the ssa/multiple-reads-v2 branch from 4048c51 to cbd6d35 Compare October 5, 2026 07:39

asgerf requested a balanced review from Copilot October 5, 2026 12:07

Copilot started reviewing on behalf of asgerf October 5, 2026 12:08 View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

asgerf marked this pull request as ready for review October 5, 2026 12:52

asgerf requested review from a team as code owners October 5, 2026 12:52

asgerf requested a review from aschackmull October 5, 2026 12:52

asgerf added the no-change-note-required This PR does not need a change note label Oct 5, 2026

ssa: Update some uses of ExprNode to bind the variable

32e5525

asgerf force-pushed the ssa/multiple-reads-v2 branch from fab449a to 32e5525 Compare October 5, 2026 13:02

hvitved reviewed Oct 5, 2026

View reviewed changes

aschackmull previously approved these changes Oct 6, 2026

View reviewed changes

asgerf dismissed aschackmull’s stale review via fc2d9f3 October 8, 2026 07:55

ssa: Grammar fix

b34d384

asgerf added 2 commits October 8, 2026 10:16

cpp: Simplify SsaImpl

82c2e5b

ssa: Only materialise if Expr is reachable (for some variable)

9ea99f8

asgerf requested a review from a team as a code owner October 8, 2026 08:29

asgerf marked this pull request as draft October 8, 2026 08:29

github-advanced-security Bot found potential problems Oct 8, 2026

View reviewed changes

Comment thread shared/ssa/codeql/ssa/Ssa.qll

variableRead(bb, i, v, true) and

// Only materialise if 'expr' has a reaching definition.

// Note that the read may correspond to a different variable than 'v', but the C++

// instantiation currently expects this particular behaviour.

github-actions Bot added the C++ label Oct 8, 2026

asgerf marked this pull request as ready for review October 8, 2026 12:39

Navigation Menu

Repository navigation

SSA: Tolerate multiple variables being read at the same CFG node - #22744

SSA: Tolerate multiple variables being read at the same CFG node#22744
asgerf wants to merge 17 commits into
github:mainfrom
asgerf:ssa/multiple-reads-v2

asgerf commented Oct 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

hvitved Oct 5, 2026 •

edited

Loading

Uh oh!

aschackmull left a comment

Uh oh!

asgerf commented Oct 8, 2026

Uh oh!

asgerf commented Oct 8, 2026

Uh oh!

asgerf commented Oct 8, 2026 •

edited by codeql-ci-app Bot

Loading

Uh oh!

asgerf commented Oct 8, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

asgerf commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

hvitved Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

asgerf commented Oct 8, 2026

Uh oh!

asgerf commented Oct 8, 2026

Uh oh!

asgerf commented Oct 8, 2026 • edited by codeql-ci-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asgerf commented Oct 8, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asgerf commented Oct 2, 2026 •

edited

Loading

hvitved Oct 5, 2026 •

edited

Loading

asgerf commented Oct 8, 2026 •

edited by codeql-ci-app Bot

Loading