Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9acb58e
SSA: Add SsaNode predicates that don't mention DefinitionExt.
aschackmull Feb 21, 2025
4e515bc
JS: Remove reference to isInputInto
aschackmull Feb 21, 2025
09b2aeb
SSA: Replace use-use step implementation in data-flow integration.
aschackmull Feb 24, 2025
88fe4fa
SSA: Remove nodes that are no longer used.
aschackmull Feb 24, 2025
782b6cf
SSA: Fix bug in guards for ssa input nodes.
aschackmull Feb 24, 2025
1af753c
JS: Use shared barrier guard for falsy check.
aschackmull Feb 24, 2025
09454f9
SSA: Remove unused.
aschackmull Feb 24, 2025
db7ec4a
Java: Remove getDefinitionExt reference
aschackmull Feb 24, 2025
0583d85
C#: Remove getDefinitionExt references.
aschackmull Feb 24, 2025
7499df4
Rust: Remove getDefinitionExt reference.
aschackmull Feb 24, 2025
22b3dc8
Ruby: Remove getDefinitionExt references.
aschackmull Feb 24, 2025
57c4fd6
JS: Combine phi reads and ssa input nodes into SynthReadNode class.
aschackmull Feb 25, 2025
95cbd21
Ruby: Accept test change following SSA bugfix.
aschackmull Feb 25, 2025
1f628d0
Ruby: Remove reference to SsaInputNode.
aschackmull Feb 25, 2025
f00f2c6
SSA: Deprecate public SsaDefinitionExtNode and SsaInputNode.
aschackmull Feb 25, 2025
b1b72b7
SSA: Add qldoc.
aschackmull Feb 25, 2025
ae3736b
C#: Accept test changes showing that we skip over useless input nodes.
aschackmull Feb 25, 2025
449150e
JS: Accept fixed FP flow.
aschackmull Feb 25, 2025
b2a5955
JS: Remove irrelevant comment.
aschackmull Feb 25, 2025
28e9644
C#: Address review comment.
aschackmull Feb 25, 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
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ module LocalFlow {
ssaDef.getADefinition() = def and
ssaDef.getControlFlowNode() = cfn and
nodeFrom = TAssignableDefinitionNode(def, cfn) and
nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = ssaDef
nodeTo.(SsaDefinitionNode).getDefinition() = ssaDef
)
}

Expand Down Expand Up @@ -1269,78 +1269,33 @@ predicate nodeIsHidden(Node n) {
}

/** An SSA node. */
abstract class SsaNode extends NodeImpl, TSsaNode {
class SsaNode extends NodeImpl, TSsaNode {
SsaImpl::DataFlowIntegration::SsaNode node;
SsaImpl::DefinitionExt def;

SsaNode() {
this = TSsaNode(node) and
def = node.getDefinitionExt()
}

SsaImpl::DefinitionExt getDefinitionExt() { result = def }
SsaNode() { this = TSsaNode(node) }

override DataFlowCallable getEnclosingCallableImpl() {
result.getAControlFlowNode().getBasicBlock() = def.getBasicBlock()
result.getAControlFlowNode().getBasicBlock() = node.getBasicBlock()
}

override Type getTypeImpl() { result = def.getSourceVariable().getType() }
override Type getTypeImpl() { result = node.getSourceVariable().getType() }

override ControlFlow::Node getControlFlowNodeImpl() {
result = def.(Ssa::Definition).getControlFlowNode()
}
override ControlFlow::Node getControlFlowNodeImpl() { none() }

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

override string toStringImpl() { result = node.toString() }
}

/** An (extended) SSA definition, viewed as a node in a data flow graph. */
class SsaDefinitionExtNode extends SsaNode {
override SsaImpl::DataFlowIntegration::SsaDefinitionExtNode node;
}
/** An SSA definition, viewed as a node in a data flow graph. */
class SsaDefinitionNode extends SsaNode {
override SsaImpl::DataFlowIntegration::SsaDefinitionNode node;

/**
* A node that represents an input to an SSA phi (read) definition.
*
* This allows for barrier guards to filter input to phi nodes. For example, in
*
* ```csharp
* var x = taint;
* if (x != "safe")
* {
* x = "safe";
* }
* sink(x);
* ```
*
* the `false` edge out of `x != "safe"` guards the input from `x = taint` into the
* `phi` node after the condition.
*
* It is also relevant to filter input into phi read nodes:
*
* ```csharp
* var x = taint;
* if (b)
* {
* if (x != "safe1")
* {
* return;
* }
* } else {
* if (x != "safe2")
* {
* return;
* }
* }
*
* sink(x);
* ```
*
* both inputs into the phi read node after the outer condition are guarded.
*/
class SsaInputNode extends SsaNode {
override SsaImpl::DataFlowIntegration::SsaInputNode node;
SsaImpl::Definition getDefinition() { result = node.getDefinition() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change the return type to Ssa::Definition and avoid the infix cast below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.


override ControlFlow::Node getControlFlowNodeImpl() {
result = this.getDefinition().(Ssa::Definition).getControlFlowNode()
}
}

/** A definition, viewed as a node in a data flow graph. */
Expand Down Expand Up @@ -1728,12 +1683,12 @@ private module ReturnNodes {
* A data-flow node that represents an assignment to an `out` or a `ref`
* parameter.
*/
class OutRefReturnNode extends ReturnNode, SsaDefinitionExtNode {
class OutRefReturnNode extends ReturnNode, SsaDefinitionNode {
OutRefReturnKind kind;

OutRefReturnNode() {
exists(Parameter p |
this.getDefinitionExt().(Ssa::Definition).isLiveOutRefParameterDefinition(p) and
this.getDefinition().(Ssa::Definition).isLiveOutRefParameterDefinition(p) and
kind.getPosition() = p.getPosition()
|
p.isOut() and kind instanceof OutReturnKind
Expand Down Expand Up @@ -2464,7 +2419,7 @@ private predicate readContentStep(Node node1, Content c, Node node2) {
exists(ForeachStmt fs, Ssa::ExplicitDefinition def |
x.hasDefPath(fs.getIterableExpr(), node1.getControlFlowNode(), def.getADefinition(),
def.getControlFlowNode()) and
node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
node2.(SsaDefinitionNode).getDefinition() = def and
c instanceof ElementContent
)
or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ module Public {
or
result instanceof TypeObject and this instanceof AdditionalNode
or
result = this.(SsaNode).getDefinitionExt().getSourceVariable().getType()
result = this.(SsaNode).getTypeImpl()
}

/** Gets the callable in which this node occurs. */
Expand Down Expand Up @@ -394,7 +394,9 @@ class SsaNode extends Node, TSsaNode {

SsaNode() { this = TSsaNode(node) }

SsaImpl::Impl::DefinitionExt getDefinitionExt() { result = node.getDefinitionExt() }
BasicBlock getBasicBlock() { result = node.getBasicBlock() }

Type getTypeImpl() { result = node.getSourceVariable().getType() }

override Location getLocation() { result = node.getLocation() }

Expand Down Expand Up @@ -442,7 +444,7 @@ module Private {
result.asCallable() = n.(CaptureNode).getSynthesizedCaptureNode().getEnclosingCallable() or
result.asFieldScope() = n.(FieldValueNode).getField() or
result.asCallable() = any(Expr e | n.(AdditionalNode).nodeAt(e, _)).getEnclosingCallable() or
result.asCallable() = n.(SsaNode).getDefinitionExt().getBasicBlock().getEnclosingCallable()
result.asCallable() = n.(SsaNode).getBasicBlock().getEnclosingCallable()
}

/** Holds if `p` is a `ParameterNode` of `c` with position `pos`. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,8 @@ private module Cached {
TSsaDefNode(SsaDefinition d) or
/** Use of a variable or 'this', with flow from a post-update node (from an earlier use) */
TSsaUseNode(ControlFlowNode use) { use = any(Ssa2::SsaConfig::SourceVariable v).getAUse() } or
/** Phi-read node (new SSA library). Ordinary phi nodes are represented by TSsaDefNode. */
TSsaPhiReadNode(Ssa2::PhiReadNode phi) or
/** Input to a phi node (new SSA library) */
TSsaInputNode(Ssa2::SsaInputNode input) or
/** A synthesized variable read (new SSA library). Definitions are represented by TSsaDefNode. */
TSsaSynthReadNode(Ssa2::SsaSynthReadNode read) or
TCapturedVariableNode(LocalVariable v) { v.isCaptured() } or
TPropNode(@property p) or
TRestPatternNode(DestructuringPattern dp, Expr rest) { rest = dp.getRest() } or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,38 +35,21 @@ class SsaUseNode extends DataFlow::Node, TSsaUseNode {
override Location getLocation() { result = expr.getLocation() }
}

class SsaPhiReadNode extends DataFlow::Node, TSsaPhiReadNode {
private Ssa2::PhiReadNode phi;
class SsaSynthReadNode extends DataFlow::Node, TSsaSynthReadNode {
private Ssa2::SsaSynthReadNode read;

SsaPhiReadNode() { this = TSsaPhiReadNode(phi) }
SsaSynthReadNode() { this = TSsaSynthReadNode(read) }

cached
override string toString() { result = "[ssa-phi-read] " + phi.getSourceVariable().getName() }

cached
override StmtContainer getContainer() { result = phi.getSourceVariable().getDeclaringContainer() }

cached
override Location getLocation() { result = phi.getLocation() }
}

class SsaInputNode extends DataFlow::Node, TSsaInputNode {
private Ssa2::SsaInputNode input;

SsaInputNode() { this = TSsaInputNode(input) }

cached
override string toString() {
result = "[ssa-input] " + input.getDefinitionExt().getSourceVariable().getName()
}
override string toString() { result = "[ssa-synth-read] " + read.getSourceVariable().getName() }

cached
override StmtContainer getContainer() {
result = input.getDefinitionExt().getSourceVariable().getDeclaringContainer()
result = read.getSourceVariable().getDeclaringContainer()
}

cached
override Location getLocation() { result = input.getLocation() }
override Location getLocation() { result = read.getLocation() }
}

class FlowSummaryNode extends DataFlow::Node, TFlowSummaryNode {
Expand Down Expand Up @@ -675,9 +658,7 @@ predicate nodeIsHidden(Node node) {
or
node instanceof SsaUseNode
or
node instanceof SsaPhiReadNode
or
node instanceof SsaInputNode
node instanceof SsaSynthReadNode
}

predicate neverSkipInPathGraph(Node node) {
Expand Down Expand Up @@ -1258,20 +1239,18 @@ Node getNodeFromSsa2(Ssa2::Node node) {
result = TImplicitThisUse(use, true)
)
or
result = TSsaPhiReadNode(node.(Ssa2::SsaDefinitionExtNode).getDefinitionExt())
or
result = TSsaInputNode(node.(Ssa2::SsaInputNode))
result = TSsaSynthReadNode(node)
or
exists(SsaPhiNode legacyPhi, Ssa2::PhiNode ssaPhi |
node.(Ssa2::SsaDefinitionExtNode).getDefinitionExt() = ssaPhi and
node.(Ssa2::SsaDefinitionNode).getDefinition() = ssaPhi and
samePhi(legacyPhi, ssaPhi) and
result = TSsaDefNode(legacyPhi)
)
}

private predicate useUseFlow(Node node1, Node node2) {
exists(Ssa2::DefinitionExt def, Ssa2::Node ssa1, Ssa2::Node ssa2 |
Ssa2::localFlowStep(def, ssa1, ssa2, _) and
exists(Ssa2::Node ssa1, Ssa2::Node ssa2 |
Ssa2::localFlowStep(_, ssa1, ssa2, _) and
node1 = getNodeFromSsa2(ssa1) and
node2 = getNodeFromSsa2(ssa2) and
not node1.getTopLevel().isExterns()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@ predicate defaultAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2,
defaultAdditionalTaintStep(node1, node2) and model = "" // TODO: set model
}

bindingset[node]
pragma[inline_late]
private BasicBlock getBasicBlockFromSsa2(Ssa2::Node node) {
result = node.(Ssa2::ExprNode).getExpr().getBasicBlock()
or
node.(Ssa2::SsaInputNode).isInputInto(_, result)
private predicate guardChecksFalsy(
Ssa2::SsaDataflowInput::Guard g, Ssa2::SsaDataflowInput::Expr e, boolean outcome
) {
exists(ConditionGuardNode guard |
guard.getTest() = g and
guard.getOutcome() = outcome and
e = g and
e instanceof VarAccess and
outcome = false
)
}

/**
Expand All @@ -64,13 +68,7 @@ private BasicBlock getBasicBlockFromSsa2(Ssa2::Node node) {
* ```
*/
private predicate varAccessBarrier(DataFlow::Node node) {
exists(ConditionGuardNode guard, Ssa2::ExprNode nodeFrom, Ssa2::Node nodeTo |
guard.getOutcome() = false and
guard.getTest().(VarAccess) = nodeFrom.getExpr() and
Ssa2::localFlowStep(_, nodeFrom, nodeTo, true) and
guard.dominates(getBasicBlockFromSsa2(nodeTo)) and
node = getNodeFromSsa2(nodeTo)
)
getNodeFromSsa2(Ssa2::BarrierGuard<guardChecksFalsy/3>::getABarrierNode()) = node
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ private module TrackInstanceInput implements CallGraphConstruction::InputSig {
// type being checked against
localFlowStep(nodeFrom, nodeTo, summary) and
not hasAdjacentTypeCheckedRead(nodeTo) and
not TypeInference::asModulePattern(nodeTo.(SsaDefinitionExtNode).getDefinitionExt(), _)
not TypeInference::asModulePattern(nodeTo.(SsaDefinitionNodeImpl).getDefinition(), _)
}

predicate stepCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {
Expand Down
Loading
Loading