From ca6444ce98faae024d48b36915a4661449ff10f6 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 27 Mar 2025 11:02:46 +0100 Subject: [PATCH 1/4] VariableCapture: Replace phi-read reference with SSA data flow integration module. --- .../codeql/dataflow/VariableCapture.qll | 148 ++++++++++-------- 1 file changed, 83 insertions(+), 65 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/shared/dataflow/codeql/dataflow/VariableCapture.qll index 3077739338ec..c76e1320a37e 100644 --- a/shared/dataflow/codeql/dataflow/VariableCapture.qll +++ b/shared/dataflow/codeql/dataflow/VariableCapture.qll @@ -699,6 +699,7 @@ module Flow 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 Input> implements OutputSig private module CaptureSsa = Ssa::Make; - 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; + + 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 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 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 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 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 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 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 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( From 70e53c2f8b3cca70fa272516f709d4b53fe41a8f Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 28 Mar 2025 16:28:12 +0100 Subject: [PATCH 2/4] SSA: Push includeWriteDefsInFlowStep constraint into newtype. --- shared/ssa/codeql/ssa/Ssa.qll | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index 2bbdb6e2a478..fcaa1eb6a9f2 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -1661,7 +1661,16 @@ module Make 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) } /** @@ -1904,14 +1913,7 @@ module Make 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 From b4daba30a5eb7500945740a9647f3956695588dc Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 31 Mar 2025 10:42:50 +0200 Subject: [PATCH 3/4] SSA: Remove dead code. --- shared/ssa/codeql/ssa/Ssa.qll | 2 -- 1 file changed, 2 deletions(-) diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index fcaa1eb6a9f2..7e48feb42dc8 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -1757,8 +1757,6 @@ module Make Input> { this.getExpr().hasCfgNode(bb_, i_) } - SourceVariable getVariable() { result = v_ } - pragma[nomagic] predicate readsAt(BasicBlock bb, int i, SourceVariable v) { bb = bb_ and From 56c46d74f91a17c6f0e25577ab32b46ffb4a85c0 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 31 Mar 2025 11:44:37 +0200 Subject: [PATCH 4/4] Java/Rust/Swift: Accept qltest changes. --- .../test/library-tests/dataflow/capture/test.expected | 2 -- .../local/CONSISTENCY/DataFlowConsistency.expected | 2 -- .../library-tests/dataflow/local/DataFlowStep.expected | 9 ++++----- .../library-tests/dataflow/dataflow/LocalFlow.expected | 10 +++++----- 4 files changed, 9 insertions(+), 14 deletions(-) delete mode 100644 rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected diff --git a/java/ql/test/library-tests/dataflow/capture/test.expected b/java/ql/test/library-tests/dataflow/capture/test.expected index 1e8a2d7d3349..a744f468fbe9 100644 --- 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] | diff --git a/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected b/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected deleted file mode 100644 index e20ac1da53f6..000000000000 --- a/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected +++ /dev/null @@ -1,2 +0,0 @@ -identityLocalStep -| main.rs:442:9:442:20 | phi(default_name) | Node steps to itself | diff --git a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected index 2766d8c0fd8c..7fd8c3fe8a8e 100644 --- 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 | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected index 7ec3f1a5aa48..b3dca8680670 100644 --- 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.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 | &... |