Skip to content

Swift: Add variable-capture flow #14577

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.

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
merged 32 commits into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
050b8e6
Swift: add failing inline expectation test based on closure AST tests.
d10c Aug 29, 2023
3253c04
Swift: s/getName/getShortName/ in InlineFlowTest.qll
d10c Aug 29, 2023
95a7d65
Swift: initial version of a swift port of most of the java code
d10c Aug 30, 2023
c04654d
Swift: getImmediateBasicBlockDominator/2 should use immediatelyDomina…
d10c Sep 5, 2023
21a369d
Swift: Add closure content read-write steps
d10c Sep 11, 2023
4e1b44a
Swift: port simpleAstFlowStep/hasAliasedAccess
d10c Sep 11, 2023
8115774
Swift: Add the capture flow step as part of the normal data flow rela…
d10c Sep 11, 2023
af49a3a
Swift: accept new results in old tests
d10c Sep 11, 2023
5418d39
Swift: add and accept a few new simple test cases
d10c Sep 14, 2023
9de3cc7
Swift: add CapturePostUpdateNode
d10c Sep 25, 2023
9dbf7e8
Swift: align definition of InputSig slightly closer to Java version
d10c Sep 18, 2023
310ebe4
Swift: Clean up test file.
MathiasVP Oct 24, 2023
1c298e6
Swift: Fix 'parameter' -> 'argument' flow into closures.
MathiasVP Oct 24, 2023
3d5098a
Swift: Add failing test.
MathiasVP Oct 24, 2023
56b49a4
Swift: Add a closure flow step from the right-hand side of variable d…
MathiasVP Oct 24, 2023
6f37d7c
Swift: Accept changes in paths.
MathiasVP Oct 24, 2023
862de15
Swift: Add required qldoc.
MathiasVP Oct 24, 2023
78e08cf
Swift: Remove irrelevant TODO.
MathiasVP Oct 25, 2023
951b6be
Swift: Untangle the confusion between 'getParameter' and 'asParameter'.
MathiasVP Oct 25, 2023
11194e5
Swift: Get rid of the unnecessary parameter/argument position for the…
MathiasVP Oct 25, 2023
2465cc2
Swift: Don't define 'ClosureSelfParameterNode' as the expression node…
MathiasVP Oct 26, 2023
2ad121a
Swift: Simplify test.
MathiasVP Oct 26, 2023
96a37f3
Swift: Simplify more tests.
MathiasVP Oct 26, 2023
784bb72
Swift: Add some more tests.
MathiasVP Oct 26, 2023
63525a9
Swift: Delete one TODO (it has been converted to an internal issue) a…
MathiasVP Oct 26, 2023
9e2dd09
Swift: Accept test regression (caused by no model for 'withVaList').
MathiasVP Oct 27, 2023
93234c0
Swift: Add model for 'withVaList' and accept test changes.
MathiasVP Oct 27, 2023
65e13aa
Swift: Add simple version of the 'captureList' test that works.
MathiasVP Oct 27, 2023
b41ec37
Swift: Remove the code related to constructor capture (and the relate…
MathiasVP Oct 27, 2023
a5a7d27
Swift: Add change note.
MathiasVP Oct 27, 2023
9b150e4
Swift: Add failing test.
MathiasVP Oct 27, 2023
68999f3
Swift: Fix test by including the 'allowParameterReturnInSelf' hook fr…
MathiasVP Oct 27, 2023
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: 1 addition & 1 deletion swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class ApplyExprCfgNode extends ExprCfgNode {

Callable getStaticTarget() { result = e.getStaticTarget() }

Expr getFunction() { result = e.getFunction() }
CfgNode getFunction() { result.getAst() = e.getFunction() }
}

class CallExprCfgNode extends ApplyExprCfgNode {
Expand Down
17 changes: 15 additions & 2 deletions swift/ql/lib/codeql/swift/dataflow/internal/DataFlowDispatch.qll
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,14 @@ private module Cached {
newtype TArgumentPosition =
TThisArgument() or
// we rely on default exprs generated in the caller for ordering
TPositionalArgument(int n) { n = any(Argument arg).getIndex() }
TPositionalArgument(int n) { n = any(Argument arg).getIndex() } or
TClosureSelfArgument()

cached
newtype TParameterPosition =
TThisParameter() or
TPositionalParameter(int n) { n = any(Argument arg).getIndex() }
TPositionalParameter(int n) { n = any(Argument arg).getIndex() } or
TClosureSelfParameter()
}

import Cached
Expand Down Expand Up @@ -331,6 +333,10 @@ class ThisParameterPosition extends ParameterPosition, TThisParameter {
override string toString() { result = "this" }
}

class ClosureSelfParameter extends ParameterPosition, TClosureSelfParameter {
override string toString() { result = "{ ... }" }
}

/** An argument position. */
class ArgumentPosition extends TArgumentPosition {
/** Gets a textual representation of this position. */
Expand All @@ -347,11 +353,18 @@ class ThisArgumentPosition extends ArgumentPosition, TThisArgument {
override string toString() { result = "this" }
}

class ClosureSelfArgument extends ArgumentPosition, TClosureSelfArgument {
override string toString() { result = "{ ... }" }
}

/** Holds if arguments at position `apos` match parameters at position `ppos`. */
pragma[inline]
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
ppos instanceof TThisParameter and
apos instanceof TThisArgument
or
ppos.(PositionalParameterPosition).getIndex() = apos.(PositionalArgumentPosition).getIndex()
or
ppos instanceof TClosureSelfParameter and
apos instanceof TClosureSelfArgument
}
217 changes: 212 additions & 5 deletions swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ private import codeql.swift.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
private import codeql.swift.frameworks.StandardLibrary.PointerTypes
private import codeql.swift.frameworks.StandardLibrary.Array
private import codeql.swift.frameworks.StandardLibrary.Dictionary
private import codeql.dataflow.VariableCapture as VariableCapture

/** Gets the callable in which this node occurs. */
DataFlowCallable nodeGetEnclosingCallable(Node n) { result = n.(NodeImpl).getEnclosingCallable() }
Expand Down Expand Up @@ -98,6 +99,16 @@ private class SsaDefinitionNodeImpl extends SsaDefinitionNode, NodeImpl {
}
}

private class CaptureNodeImpl extends CaptureNode, NodeImpl {
override Location getLocationImpl() { result = this.getSynthesizedCaptureNode().getLocation() }

override string toStringImpl() { result = this.getSynthesizedCaptureNode().toString() }

override DataFlowCallable getEnclosingCallable() {
result.asSourceCallable() = this.getSynthesizedCaptureNode().getEnclosingCallable()
}
}

private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) {
exists(BasicBlock bb, int i | def.lastRefRedef(bb, i, next) |
def.definesAt(_, bb, i) and
Expand Down Expand Up @@ -140,12 +151,14 @@ private module Cached {
[
any(Argument arg | modifiable(arg)).getExpr(), any(MemberRefExpr ref).getBase(),
any(ApplyExpr apply).getQualifier(), any(TupleElementExpr te).getSubExpr(),
any(SubscriptExpr se).getBase()
any(SubscriptExpr se).getBase(),
any(ApplyExpr apply | not exists(apply.getStaticTarget())).getFunction()
])
} or
TDictionarySubscriptNode(SubscriptExpr e) {
e.getBase().getType().getCanonicalType() instanceof CanonicalDictionaryType
}
} or
TCaptureNode(CaptureFlow::SynthesizedCaptureNode cn)

private predicate localSsaFlowStepUseUse(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
def.adjacentReadPair(nodeFrom.getCfgNode(), nodeTo.getCfgNode()) and
Expand Down Expand Up @@ -279,6 +292,9 @@ private module Cached {
// flow through a flow summary (extension of `SummaryModelCsv`)
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom.(FlowSummaryNode).getSummaryNode(),
nodeTo.(FlowSummaryNode).getSummaryNode(), true)
or
// flow step according to the closure capture library
captureValueStep(nodeFrom, nodeTo)
}

/**
Expand All @@ -305,7 +321,8 @@ private module Cached {
TFieldContent(FieldDecl f) or
TTupleContent(int index) { exists(any(TupleExpr te).getElement(index)) } or
TEnumContent(ParamDecl f) { exists(EnumElementDecl d | d.getAParam() = f) } or
TCollectionContent()
TCollectionContent() or
TCapturedVariableContent(CapturedVariable v)
}

/**
Expand Down Expand Up @@ -366,12 +383,18 @@ private class DictionarySubscriptNode extends NodeImpl, TDictionarySubscriptNode
SubscriptExpr getExpr() { result = expr }
}

private class ClosureSelfReferenceNode extends ExprNodeImpl {
override ClosureExpr expr;

ClosureExpr getClosure() { result = expr }
}

private module ParameterNodes {
abstract class ParameterNodeImpl extends NodeImpl {
predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { none() }

/** Gets the parameter associated with this node, if any. */
ParamDecl getParameter() { none() }
override ParamDecl getParameter() { none() }
}

class SourceParameterNode extends ParameterNodeImpl, TSourceParameterNode {
Expand All @@ -396,6 +419,13 @@ private module ParameterNodes {
override ParamDecl getParameter() { result = param }
}

class ClosureSelfParameterNode extends ParameterNodeImpl, ClosureSelfReferenceNode {
override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
c.asSourceCallable() = this.getClosure() and
pos instanceof ClosureSelfParameter
}
}

class SummaryParameterNode extends ParameterNodeImpl, FlowSummaryNode {
SummaryParameterNode() {
FlowSummaryImpl::Private::summaryParameterNode(this.getSummaryNode(), _)
Expand Down Expand Up @@ -610,6 +640,18 @@ private module ArgumentNodes {
pos = TPositionalArgument(0)
}
}

class SelfClosureArgumentNode extends ExprNode, ArgumentNode {
ApplyExprCfgNode apply;

SelfClosureArgumentNode() { n = apply.getFunction() }

override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
apply = call.asCall() and
not exists(apply.getStaticTarget()) and
pos instanceof ClosureSelfArgument
}
}
}

import ArgumentNodes
Expand Down Expand Up @@ -658,7 +700,7 @@ private module ReturnNodes {
result = TDataFlowFunc(param.getDeclaringFunction())
}

ParamDecl getParameter() { result = param }
override ParamDecl getParameter() { result = param }

override Location getLocationImpl() { result = exit.getLocation() }

Expand Down Expand Up @@ -785,6 +827,156 @@ private module OutNodes {

import OutNodes

/**
* Holds if there is a data flow step from `e1` to `e2` that only steps from
* child to parent in the AST.
*/
private predicate simpleAstFlowStep(Expr e1, Expr e2) {
e2.(IfExpr).getBranch(_) = e1
or
e2.(AssignExpr).getSource() = e1
or
e2.(ArrayExpr).getAnElement() = e1
}

private predicate closureFlowStep(CaptureInput::Expr e1, CaptureInput::Expr e2) {
simpleAstFlowStep(e1, e2)
or
exists(Ssa::WriteDefinition def |
def.getARead().getNode().asAstNode() = e2 and
def.assigns(any(CfgNode cfg | cfg.getNode().asAstNode() = e1))
)
or
e2.(Pattern).getImmediateMatchingExpr() = e1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the declared type of e2 being Expr, does this even work? IIRC this was a problem when we debugged this together and this disjunct should perhaps just be folded into the case above so we can step from Expr to Expr.

Copy link
Contributor Author

@MathiasVP MathiasVP Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that e2 is of type CaptureInput::Expr, and CaptureInput::Expr is actually AstNode (which includes Pattern).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's why compilation doesn't fail. I'm asking whether Expr and Pattern might be sufficiently disjoint (even though both are subtypes of AstNode) that this will block flow?

Copy link
Contributor Author

@MathiasVP MathiasVP Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll leave this to another PR. I've discovered that the WriteDefinition::assigns predicate doesn't really do the right thing at all. For a variable declaration such as:

var x = 0

the WriteDefinition::assigns(value) predicate holds where value is the left-hand side of the declaration (i.e., the lvalue of x) instead of the right-hand side (i.e., 0). Once that's fixed (which I'll do in a later PR) we can remove this disjunct and have CaptureInput::Expr be Expr.

}

private module CaptureInput implements VariableCapture::InputSig {
private import swift as S
private import codeql.swift.controlflow.BasicBlocks as B

class Location = S::Location;

class BasicBlock instanceof B::BasicBlock {
string toString() { result = super.toString() }

Callable getEnclosingCallable() { result = super.getScope() }

Location getLocation() { result = super.getLocation() }
}

BasicBlock getImmediateBasicBlockDominator(BasicBlock bb) {
result.(B::BasicBlock).immediatelyDominates(bb)
}

BasicBlock getABasicBlockSuccessor(BasicBlock bb) { result = bb.(B::BasicBlock).getASuccessor() }

//TODO: support capture of `this` in lambdas
class CapturedVariable instanceof S::VarDecl {
CapturedVariable() {
any(S::CapturedDecl capturedDecl).getDecl() = this and
exists(this.getEnclosingCallable())
}

string toString() { result = super.toString() }

Callable getCallable() { result = super.getEnclosingCallable() }

Location getLocation() { result = super.getLocation() }
}

class CapturedParameter extends CapturedVariable instanceof S::ParamDecl { }

class Expr instanceof S::AstNode {
string toString() { result = super.toString() }

Location getLocation() { result = super.getLocation() }

predicate hasCfgNode(BasicBlock bb, int i) {
this = bb.(B::BasicBlock).getNode(i).getNode().asAstNode()
}
}

class VariableWrite extends Expr {
CapturedVariable variable;
Expr source;

VariableWrite() {
exists(S::Assignment a | this = a |
a.getDest().(DeclRefExpr).getDecl() = variable and
source = a.getSource()
)
or
exists(S::NamedPattern np | this = np |
variable = np.getVarDecl() and
source = np.getMatchingExpr()
)
// TODO: support multiple variables in LHS of =, in both of above cases.
}

CapturedVariable getVariable() { result = variable }

Expr getSource() { result = source }
}

class VariableRead extends Expr instanceof S::DeclRefExpr {
CapturedVariable v;

VariableRead() { this.getDecl() = v /* TODO: this should be an R-value only. */ }

CapturedVariable getVariable() { result = v }
}

class ClosureExpr extends Expr instanceof S::Callable {
ClosureExpr() { any(S::CapturedDecl c).getScope() = this }

predicate hasBody(Callable body) { this = body }

predicate hasAliasedAccess(Expr f) { closureFlowStep+(this, f) and not closureFlowStep(f, _) }
}

class Callable extends S::Callable {
predicate isConstructor() { this instanceof S::Initializer }
}
}

class CapturedVariable = CaptureInput::CapturedVariable;

class CapturedParameter = CaptureInput::CapturedParameter;

module CaptureFlow = VariableCapture::Flow<CaptureInput>;

private CaptureFlow::ClosureNode asClosureNode(Node n) {
result = n.(CaptureNode).getSynthesizedCaptureNode()
or
result.(CaptureFlow::ExprNode).getExpr() = n.asExpr()
or
result.(CaptureFlow::ExprPostUpdateNode).getExpr() =
n.(PostUpdateNode).getPreUpdateNode().asExpr()
or
result.(CaptureFlow::ParameterNode).getParameter() = n.getParameter()
or
result.(CaptureFlow::ThisParameterNode).getCallable() = n.(ClosureSelfParameterNode).getClosure()
or
result.(CaptureFlow::MallocNode).getClosureExpr() = n.getCfgNode().getNode().asAstNode() // TODO: figure out why the java version had PostUpdateNode logic here
or
exists(CaptureInput::VariableWrite write |
result.(CaptureFlow::VariableWriteSourceNode).getVariableWrite() = write and
n.asExpr() = write.getSource()
)
}

private predicate captureStoreStep(Node node1, Content::CapturedVariableContent c, Node node2) {
CaptureFlow::storeStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2))
}

private predicate captureReadStep(Node node1, Content::CapturedVariableContent c, Node node2) {
CaptureFlow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2))
}

predicate captureValueStep(Node node1, Node node2) {
CaptureFlow::localFlowStep(asClosureNode(node1), asClosureNode(node2))
}

predicate jumpStep(Node pred, Node succ) {
FlowSummaryImpl::Private::Steps::summaryJumpStep(pred.(FlowSummaryNode).getSummaryNode(),
succ.(FlowSummaryNode).getSummaryNode())
Expand Down Expand Up @@ -897,6 +1089,8 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
or
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), c,
node2.(FlowSummaryNode).getSummaryNode())
or
captureStoreStep(node1, any(Content::CapturedVariableContent cvc | c.isSingleton(cvc)), node2)
}

predicate isLValue(Expr e) { any(AssignExpr assign).getDest() = e }
Expand Down Expand Up @@ -1001,6 +1195,8 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
or
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), c,
node2.(FlowSummaryNode).getSummaryNode())
or
captureReadStep(node1, any(Content::CapturedVariableContent cvc | c.isSingleton(cvc)), node2)
}

/**
Expand Down Expand Up @@ -1094,6 +1290,17 @@ private module PostUpdateNodes {
result.(FlowSummaryNode).getSummaryNode())
}
}

class CapturePostUpdateNode extends PostUpdateNodeImpl, CaptureNode {
private CaptureNode pre;

CapturePostUpdateNode() {
CaptureFlow::capturePostUpdateNode(this.getSynthesizedCaptureNode(),
pre.getSynthesizedCaptureNode())
}

override Node getPreUpdateNode() { result = pre }
}
}

private import PostUpdateNodes
Expand Down
Loading