Merge pull request #19154 from aschackmull/ssa/variablecapture

Ssa: Replace phi-read references in VariableCapture with default use-use flow
2025-12-16 16:53:25 +01:00 · 2025-04-02 10:16:17 +02:00
parent 45b55c05ae 56c46d74f9
commit 47b1c3d3ce
6 changed files with 103 additions and 90 deletions
--- a/java/ql/test/library-tests/dataflow/capture/test.expected
+++ b/java/ql/test/library-tests/dataflow/capture/test.expected
@@ -18,7 +18,6 @@
 | A.java:21:11:21:13 | "B" : String | A.java:15:16:15:22 | get(...) : String |
 | A.java:21:11:21:13 | "B" : String | A.java:21:7:21:13 | ...=... : String |
 | A.java:21:11:21:13 | "B" : String | A.java:25:5:25:26 | SSA phi(s) : String |
-| A.java:21:11:21:13 | "B" : String | A.java:25:5:25:26 | phi(String s) : String |
 | A.java:21:11:21:13 | "B" : String | A.java:28:11:38:5 | String s : String |
 | A.java:21:11:21:13 | "B" : String | A.java:28:11:38:5 | new (...) : new A(...) { ... } [String s] |
 | A.java:21:11:21:13 | "B" : String | A.java:30:14:30:16 | parameter this : new A(...) { ... } [String s] |
@@ -35,7 +34,6 @@
 | A.java:23:11:23:13 | "C" : String | A.java:15:16:15:22 | get(...) : String |
 | A.java:23:11:23:13 | "C" : String | A.java:23:7:23:13 | ...=... : String |
 | A.java:23:11:23:13 | "C" : String | A.java:25:5:25:26 | SSA phi(s) : String |
-| A.java:23:11:23:13 | "C" : String | A.java:25:5:25:26 | phi(String s) : String |
 | A.java:23:11:23:13 | "C" : String | A.java:28:11:38:5 | String s : String |
 | A.java:23:11:23:13 | "C" : String | A.java:28:11:38:5 | new (...) : new A(...) { ... } [String s] |
 | A.java:23:11:23:13 | "C" : String | A.java:30:14:30:16 | parameter this : new A(...) { ... } [String s] |
--- a/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected
+++ b/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected
@@ -1,2 +0,0 @@
-identityLocalStep
-| main.rs:442:9:442:20 | phi(default_name) | Node steps to itself |
--- a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected
+++ b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected
@@ -647,10 +647,9 @@ localStep
 | main.rs:441:24:441:33 | [post] receiver for source(...) | main.rs:441:24:441:33 | [post] source(...) |
 | main.rs:441:24:441:33 | source(...) | main.rs:441:24:441:33 | receiver for source(...) |
 | main.rs:441:24:441:45 | ... .to_string(...) | main.rs:441:9:441:20 | default_name |
-| main.rs:441:24:441:45 | ... .to_string(...) | main.rs:442:9:442:20 | phi(default_name) |
+| main.rs:441:24:441:45 | ... .to_string(...) | main.rs:442:9:442:20 | SSA phi read(default_name) |
 | main.rs:442:5:448:5 | for ... in ... { ... } | main.rs:440:75:449:1 | { ... } |
-| main.rs:442:9:442:20 | phi(default_name) | main.rs:442:9:442:20 | phi(default_name) |
-| main.rs:442:9:442:20 | phi(default_name) | main.rs:444:41:444:67 | default_name |
+| main.rs:442:9:442:20 | SSA phi read(default_name) | main.rs:444:41:444:67 | default_name |
 | main.rs:442:10:442:13 | [SSA] cond | main.rs:443:12:443:15 | cond |
 | main.rs:442:10:442:13 | cond | main.rs:442:10:442:13 | [SSA] cond |
 | main.rs:442:10:442:13 | cond | main.rs:442:10:442:13 | cond |
@@ -664,9 +663,9 @@ localStep
 | main.rs:444:21:444:24 | [post] receiver for name | main.rs:444:21:444:24 | [post] name |
 | main.rs:444:21:444:24 | name | main.rs:444:21:444:24 | receiver for name |
 | main.rs:444:21:444:68 | name.unwrap_or_else(...) | main.rs:444:17:444:17 | n |
-| main.rs:444:41:444:67 | [post] default_name | main.rs:442:9:442:20 | phi(default_name) |
+| main.rs:444:41:444:67 | [post] default_name | main.rs:442:9:442:20 | SSA phi read(default_name) |
 | main.rs:444:41:444:67 | closure self in \|...\| ... | main.rs:444:44:444:55 | this |
-| main.rs:444:41:444:67 | default_name | main.rs:442:9:442:20 | phi(default_name) |
+| main.rs:444:41:444:67 | default_name | main.rs:442:9:442:20 | SSA phi read(default_name) |
 | main.rs:444:44:444:55 | [post] receiver for default_name | main.rs:444:44:444:55 | [post] default_name |
 | main.rs:444:44:444:55 | default_name | main.rs:444:44:444:55 | receiver for default_name |
 | main.rs:445:18:445:18 | [post] receiver for n | main.rs:445:18:445:18 | [post] n |
--- a/shared/dataflow/codeql/dataflow/VariableCapture.qll
+++ b/shared/dataflow/codeql/dataflow/VariableCapture.qll
@@ -699,6 +699,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
    class SourceVariable = CaptureContainer;

    predicate variableWrite(BasicBlock bb, int i, SourceVariable cc, boolean certain) {
+      Cached::ref() and
      (
        exists(CapturedVariable v | cc = TVariable(v) and captureWrite(v, bb, i, true, _))
        or
@@ -721,23 +722,55 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig

  private module CaptureSsa = Ssa::Make<Location, CaptureSsaInput>;

-  private newtype TClosureNode =
-    TSynthRead(CapturedVariable v, BasicBlock bb, int i, Boolean isPost) {
-      synthRead(v, bb, i, _, _)
-    } or
-    TSynthThisQualifier(BasicBlock bb, int i, Boolean isPost) { synthThisQualifier(bb, i) } or
-    TSynthPhi(CaptureSsa::DefinitionExt phi) {
-      phi instanceof CaptureSsa::PhiNode or phi instanceof CaptureSsa::PhiReadNode
-    } or
-    TExprNode(Expr expr, Boolean isPost) {
-      expr instanceof VariableRead
-      or
-      synthRead(_, _, _, _, expr)
-    } or
-    TParamNode(CapturedParameter p) or
-    TThisParamNode(Callable c) { captureAccess(_, c) } or
-    TMallocNode(ClosureExpr ce) { hasConstructorCapture(ce, _) } or
-    TVariableWriteSourceNode(VariableWrite write)
+  private module DataFlowIntegrationInput implements CaptureSsa::DataFlowIntegrationInputSig {
+    private import codeql.util.Void
+
+    class Expr instanceof Input::ControlFlowNode {
+      string toString() { result = super.toString() }
+
+      predicate hasCfgNode(BasicBlock bb, int i) { bb.getNode(i) = this }
+    }
+
+    class Guard extends Void {
+      predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) { none() }
+    }
+
+    predicate guardDirectlyControlsBlock(Guard guard, BasicBlock bb, boolean branch) { none() }
+
+    predicate includeWriteDefsInFlowStep() { none() }
+
+    predicate supportBarrierGuardsOnPhiEdges() { none() }
+  }
+
+  private module SsaFlow = CaptureSsa::DataFlowIntegration<DataFlowIntegrationInput>;
+
+  cached
+  private module Cached {
+    cached
+    predicate ref() { any() }
+
+    cached
+    predicate backref() { localFlowStep(_, _) implies any() }
+
+    cached
+    newtype TClosureNode =
+      TSynthRead(CapturedVariable v, BasicBlock bb, int i, Boolean isPost) {
+        synthRead(v, bb, i, _, _)
+      } or
+      TSynthThisQualifier(BasicBlock bb, int i, Boolean isPost) { synthThisQualifier(bb, i) } or
+      TSynthSsa(SsaFlow::SsaNode n) or
+      TExprNode(Expr expr, Boolean isPost) {
+        expr instanceof VariableRead
+        or
+        synthRead(_, _, _, _, expr)
+      } or
+      TParamNode(CapturedParameter p) or
+      TThisParamNode(Callable c) { captureAccess(_, c) } or
+      TMallocNode(ClosureExpr ce) { hasConstructorCapture(ce, _) } or
+      TVariableWriteSourceNode(VariableWrite write)
+  }
+
+  private import Cached

  class ClosureNode extends TClosureNode {
    /** Gets a textual representation of this node. */
@@ -746,11 +779,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
      or
      result = "this" and this = TSynthThisQualifier(_, _, _)
      or
-      exists(CaptureSsa::DefinitionExt phi, CaptureContainer cc |
-        this = TSynthPhi(phi) and
-        phi.definesAt(cc, _, _, _) and
-        result = "phi(" + cc.toString() + ")"
-      )
+      exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and result = n.toString())
      or
      exists(Expr expr, boolean isPost | this = TExprNode(expr, isPost) |
        isPost = false and result = expr.toString()
@@ -784,9 +813,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
        captureWrite(_, bb, i, false, any(VariableWrite vw | result = vw.getLocation()))
      )
      or
-      exists(CaptureSsa::DefinitionExt phi, BasicBlock bb |
-        this = TSynthPhi(phi) and phi.definesAt(_, bb, _, _) and result = bb.getLocation()
-      )
+      exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and result = n.getLocation())
      or
      exists(Expr expr | this = TExprNode(expr, _) and result = expr.getLocation())
      or
@@ -802,7 +829,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
    }
  }

-  private class TSynthesizedCaptureNode = TSynthRead or TSynthThisQualifier or TSynthPhi;
+  private class TSynthesizedCaptureNode = TSynthRead or TSynthThisQualifier or TSynthSsa;

  class SynthesizedCaptureNode extends ClosureNode, TSynthesizedCaptureNode {
    BasicBlock getBasicBlock() {
@@ -810,9 +837,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
      or
      this = TSynthThisQualifier(result, _, _)
      or
-      exists(CaptureSsa::DefinitionExt phi |
-        this = TSynthPhi(phi) and phi.definesAt(_, result, _, _)
-      )
+      exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and n.getBasicBlock() = result)
    }

    Callable getEnclosingCallable() { result = this.getBasicBlock().getEnclosingCallable() }
@@ -820,17 +845,13 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
    predicate isVariableAccess(CapturedVariable v) {
      this = TSynthRead(v, _, _, _)
      or
-      exists(CaptureSsa::DefinitionExt phi |
-        this = TSynthPhi(phi) and phi.definesAt(TVariable(v), _, _, _)
-      )
+      exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and n.getSourceVariable() = TVariable(v))
    }

    predicate isInstanceAccess() {
      this instanceof TSynthThisQualifier
      or
-      exists(CaptureSsa::DefinitionExt phi |
-        this = TSynthPhi(phi) and phi.definesAt(TThis(_), _, _, _)
-      )
+      exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and n.getSourceVariable() = TThis(_))
    }
  }

@@ -872,18 +893,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
    )
  }

-  private predicate step(CaptureContainer cc, BasicBlock bb1, int i1, BasicBlock bb2, int i2) {
-    CaptureSsa::adjacentDefReadExt(_, cc, bb1, i1, bb2, i2)
-  }
-
-  private predicate stepToPhi(CaptureContainer cc, BasicBlock bb, int i, TSynthPhi phi) {
-    exists(CaptureSsa::DefinitionExt next |
-      CaptureSsa::lastRefRedefExt(_, cc, bb, i, next) and
-      phi = TSynthPhi(next)
-    )
-  }
-
-  private predicate ssaAccessAt(
+  private predicate ssaReadAt(
    ClosureNode n, CaptureContainer cc, boolean isPost, BasicBlock bb, int i
  ) {
    exists(CapturedVariable v |
@@ -894,49 +904,57 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
    or
    n = TSynthThisQualifier(bb, i, isPost) and cc = TThis(bb.getEnclosingCallable())
    or
-    exists(CaptureSsa::DefinitionExt phi |
-      n = TSynthPhi(phi) and phi.definesAt(cc, bb, i, _) and isPost = false
-    )
-    or
    exists(VariableRead vr, CapturedVariable v |
      captureRead(v, bb, i, true, vr) and
      n = TExprNode(vr, isPost) and
      cc = TVariable(v)
    )
-    or
+  }
+
+  private predicate ssaWriteAt(ClosureNode n, CaptureContainer cc, BasicBlock bb, int i) {
    exists(VariableWrite vw, CapturedVariable v |
      captureWrite(v, bb, i, true, vw) and
      n = TVariableWriteSourceNode(vw) and
-      isPost = false and
      cc = TVariable(v)
    )
    or
    exists(CapturedParameter p |
      entryDef(cc, bb, i) and
      cc = TVariable(p) and
-      n = TParamNode(p) and
-      isPost = false
+      n = TParamNode(p)
    )
    or
    exists(Callable c |
      entryDef(cc, bb, i) and
      cc = TThis(c) and
-      n = TThisParamNode(c) and
-      isPost = false
+      n = TThisParamNode(c)
    )
  }

-  predicate localFlowStep(ClosureNode node1, ClosureNode node2) {
-    exists(CaptureContainer cc, BasicBlock bb1, int i1, BasicBlock bb2, int i2 |
-      step(cc, bb1, i1, bb2, i2) and
-      ssaAccessAt(node1, pragma[only_bind_into](cc), _, bb1, i1) and
-      ssaAccessAt(node2, pragma[only_bind_into](cc), false, bb2, i2)
+  bindingset[result, cc]
+  pragma[inline_late]
+  private SsaFlow::Node asNode(CaptureContainer cc, ClosureNode n) {
+    n = TSynthSsa(result)
+    or
+    exists(BasicBlock bb, int i |
+      result.(SsaFlow::ExprNode).getExpr().hasCfgNode(bb, i) and
+      ssaReadAt(n, cc, false, bb, i)
    )
    or
-    exists(CaptureContainer cc, BasicBlock bb, int i |
-      stepToPhi(cc, bb, i, node2) and
-      ssaAccessAt(node1, cc, _, bb, i)
+    exists(BasicBlock bb, int i |
+      result.(SsaFlow::ExprPostUpdateNode).getExpr().hasCfgNode(bb, i) and
+      ssaReadAt(n, cc, true, bb, i)
    )
+    or
+    exists(BasicBlock bb, int i |
+      result.(SsaFlow::WriteDefSourceNode).getDefinition().definesAt(cc, bb, i) and
+      ssaWriteAt(n, cc, bb, i)
+    )
+  }
+
+  cached
+  predicate localFlowStep(ClosureNode n1, ClosureNode n2) {
+    exists(CaptureContainer cc | SsaFlow::localFlowStep(cc, asNode(cc, n1), asNode(cc, n2), _))
  }

  private predicate storeStepClosure(
--- a/shared/ssa/codeql/ssa/Ssa.qll
+++ b/shared/ssa/codeql/ssa/Ssa.qll
@@ -1661,7 +1661,16 @@ module Make<LocationSig Location, InputSig<Location> Input> {
    private newtype TNode =
      TWriteDefSource(WriteDefinition def) { DfInput::ssaDefHasSource(def) } or
      TExprNode(DfInput::Expr e, Boolean isPost) { e = DfInput::getARead(_) } or
-      TSsaDefinitionNode(DefinitionExt def) { not phiHasUniqNextNode(def) } or
+      TSsaDefinitionNode(DefinitionExt def) {
+        not phiHasUniqNextNode(def) and
+        if DfInput::includeWriteDefsInFlowStep()
+        then any()
+        else (
+          def instanceof PhiNode or
+          def instanceof PhiReadNode or
+          DfInput::allowFlowIntoUncertainDef(def)
+        )
+      } or
      TSsaInputNode(SsaPhiExt phi, BasicBlock input) { relevantPhiInputNode(phi, input) }

    /**
@@ -1748,8 +1757,6 @@ module Make<LocationSig Location, InputSig<Location> Input> {
        this.getExpr().hasCfgNode(bb_, i_)
      }

-      SourceVariable getVariable() { result = v_ }
-
      pragma[nomagic]
      predicate readsAt(BasicBlock bb, int i, SourceVariable v) {
        bb = bb_ and
@@ -1904,14 +1911,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
      exists(DefinitionExt def |
        nodeFrom.(SsaDefinitionExtNodeImpl).getDefExt() = def and
        def.definesAt(v, bb, i, _) and
-        isUseStep = false and
-        if DfInput::includeWriteDefsInFlowStep()
-        then any()
-        else (
-          def instanceof PhiNode or
-          def instanceof PhiReadNode or
-          DfInput::allowFlowIntoUncertainDef(def)
-        )
+        isUseStep = false
      )
      or
      [nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()].(ReadNode).readsAt(bb, i, v) and
--- a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected
+++ b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected
@@ -1378,17 +1378,17 @@
 | test.swift:888:9:888:9 | stream | test.swift:888:9:888:9 | SSA def(stream) |
 | test.swift:888:18:896:6 | call to AsyncStream<Element>.init(_:bufferingPolicy:_:) | test.swift:888:9:888:9 | stream |
 | test.swift:889:9:889:9 | continuation | test.swift:890:27:895:13 | continuation |
-| test.swift:890:27:895:13 | closure self parameter | test.swift:891:17:891:17 | phi(this) |
+| test.swift:890:27:895:13 | closure self parameter | test.swift:891:17:891:17 | SSA phi read(this) |
 | test.swift:891:17:891:17 | $generator | test.swift:891:17:891:17 | &... |
 | test.swift:891:17:891:17 | &... | test.swift:891:17:891:17 | $generator |
+| test.swift:891:17:891:17 | SSA phi read(this) | test.swift:892:21:892:21 | this |
+| test.swift:891:17:891:17 | SSA phi read(this) | test.swift:894:17:894:17 | this |
 | test.swift:891:17:891:17 | [post] $generator | test.swift:891:17:891:17 | &... |
-| test.swift:891:17:891:17 | phi(this) | test.swift:892:21:892:21 | this |
-| test.swift:891:17:891:17 | phi(this) | test.swift:894:17:894:17 | this |
 | test.swift:891:26:891:26 | $generator | test.swift:891:26:891:26 | SSA def($generator) |
 | test.swift:891:26:891:26 | SSA def($generator) | test.swift:891:17:891:17 | $generator |
 | test.swift:891:26:891:30 | call to makeIterator() | test.swift:891:26:891:26 | $generator |
-| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) |
-| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) |
+| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | SSA phi read(this) |
+| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | SSA phi read(this) |
 | test.swift:898:5:898:5 | $i$generator | test.swift:898:5:898:5 | &... |
 | test.swift:898:5:898:5 | &... | test.swift:898:5:898:5 | $i$generator |
 | test.swift:898:5:898:5 | [post] $i$generator | test.swift:898:5:898:5 | &... |