Skip to content

Ssa: Replace phi-read references in VariableCapture with default use-use flow #19154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aschackmull merged 4 commits into from
Apr 2, 2025
Merged

Ssa: Replace phi-read references in VariableCapture with default use-use flow #19154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca6444c
VariableCapture: Replace phi-read reference with SSA data flow integr…
aschackmull Mar 27, 2025
70e53c2
SSA: Push includeWriteDefsInFlowStep constraint into newtype.
aschackmull Mar 28, 2025
b4daba3
SSA: Remove dead code.
aschackmull Mar 31, 2025
56c46d7
Java/Rust/Swift: Accept qltest changes.
aschackmull Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions java/ql/test/library-tests/dataflow/capture/test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] |
Expand All @@ -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] |
Expand Down
2 changes: 0 additions & 2 deletions rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected
View file
Open in desktop

This file was deleted.

9 changes: 4 additions & 5 deletions rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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 |
Expand Down
148 changes: 83 additions & 65 deletions shared/dataflow/codeql/dataflow/VariableCapture.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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. */
Expand All @@ -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()
Expand Down Expand Up @@ -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
Expand All @@ -802,35 +829,29 @@ 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() {
this = TSynthRead(_, result, _, _)
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() }

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(_))
}
}

Expand Down Expand Up @@ -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 |
Expand All @@ -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(
Expand Down
22 changes: 11 additions & 11 deletions shared/ssa/codeql/ssa/Ssa.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 | &... |
Expand Down
Loading