From 615f2a2336fd9edcf3ab5ee097bc914bc92e3be7 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 3 Jul 2024 11:35:42 +0200 Subject: [PATCH] Ruby: Adopt shared SSA data-flow integration --- .../dataflow/internal/DataFlowDispatch.qll | 3 +- .../dataflow/internal/DataFlowPrivate.qll | 407 +++++++++--------- .../ruby/dataflow/internal/DataFlowPublic.qll | 85 +--- .../codeql/ruby/dataflow/internal/SsaImpl.qll | 65 +-- .../internal/TaintTrackingPrivate.qll | 2 +- 5 files changed, 233 insertions(+), 329 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll index 94a6657c0a248..2e2713b3cc213 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll @@ -1093,8 +1093,7 @@ private module TrackSingletonMethodOnInstanceInput implements CallGraphConstruct singletonMethodOnInstance(_, _, nodeFromPreExpr.getExpr()) ) | - nodeFromPreExpr = - LocalFlow::getParameterDefNode(p.getParameter()).getDefinitionExt().getARead() + nodeFromPreExpr = getParameterDef(p.getParameter()).getARead() or nodeFromPreExpr = p.(SelfParameterNodeImpl).getSelfDefinition().getARead() ) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll index c0bc6ac243d7c..62e8ba4e90a53 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll @@ -72,134 +72,155 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) { not exists(getALastEvalNode(result)) } -/** An SSA definition into which another SSA definition may flow. */ -class SsaInputDefinitionExt extends SsaImpl::DefinitionExt { - SsaInputDefinitionExt() { - this instanceof Ssa::PhiNode - or - this instanceof SsaImpl::PhiReadNode - } +/** Gets the SSA definition node corresponding to parameter `p`. */ +pragma[nomagic] +SsaImpl::DefinitionExt getParameterDef(NamedParameter p) { + exists(BasicBlock bb, int i | + bb.getNode(i).getAstNode() = p.getDefiningAccess() and + result.definesAt(_, bb, i, _) + ) +} - predicate hasInputFromBlock(SsaImpl::DefinitionExt def, BasicBlock bb, int i, BasicBlock input) { - SsaImpl::lastRefBeforeRedefExt(def, bb, i, input, this) +class NormalParameter extends Parameter { + NormalParameter() { + this instanceof SimpleParameter or + this instanceof OptionalParameter or + this instanceof KeywordParameter or + this instanceof HashSplatParameter or + this instanceof SplatParameter or + this instanceof BlockParameter } } -/** Provides predicates related to local data flow. */ -module LocalFlow { - /** - * Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`. - */ - pragma[nomagic] - private predicate localFlowSsaInputFromDef( - SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputNode nodeTo - ) { - exists(BasicBlock bb, int i, BasicBlock input, SsaInputDefinitionExt next | - next.hasInputFromBlock(def, bb, i, input) and - def = nodeFrom.getDefinitionExt() and - def.definesAt(_, bb, i, _) and - nodeTo = TSsaInputNode(next, input) - ) - } +/** Provides logic related to SSA. */ +module SsaFlow { + module Input implements SsaImpl::Impl::DataFlowIntegrationInputSig { + class Expr extends CfgNodes::ExprCfgNode { + predicate hasCfgNode(SsaImpl::SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + } - /** - * Holds if `nodeFrom` is a last read of SSA definition `def`, which - * can reach `nodeTo`. - */ - pragma[nomagic] - predicate localFlowSsaInputFromRead(SsaImpl::DefinitionExt def, Node nodeFrom, SsaInputNode nodeTo) { - exists( - BasicBlock bb, int i, CfgNodes::ExprCfgNode exprFrom, BasicBlock input, - SsaInputDefinitionExt next - | - next.hasInputFromBlock(def, bb, i, input) and - exprFrom = bb.getNode(i) and - exprFrom.getExpr() instanceof VariableReadAccess and - exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()] and - nodeTo = TSsaInputNode(next, input) - ) - } + /** + * Holds if SSA definition `def` assigns `value` to the underlying variable. + * + * This is either a direct assignment, `x = value`, or an assignment via + * simple pattern matching + * + * ```rb + * case value + * in Foo => x then ... + * in y => then ... + * end + * ``` + */ + predicate ssaDefAssigns(SsaImpl::WriteDefinition def, Expr value) { + def.(Ssa::WriteDefinition).assigns(value) + or + exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::AstCfgNode pattern | + case.getValue() = value and + pattern = case.getBranch(_).(CfgNodes::ExprNodes::InClauseCfgNode).getPattern() + | + def.(Ssa::WriteDefinition).getWriteAccess() = + [pattern, pattern.(CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess()] + ) + } - /** Gets the SSA definition node corresponding to parameter `p`. */ - pragma[nomagic] - SsaDefinitionExtNode getParameterDefNode(NamedParameter p) { - exists(BasicBlock bb, int i | - bb.getNode(i).getAstNode() = p.getDefiningAccess() and - result.getDefinitionExt().definesAt(_, bb, i, _) - ) - } + private newtype TParameter = + TNormalParameter(NormalParameter p) or + TSelfMethodParameter(MethodBase m) or + TSelfToplevelParameter(Toplevel t) - /** - * Holds if `nodeFrom` is a parameter node, and `nodeTo` is a corresponding SSA node. - */ - pragma[nomagic] - predicate localFlowSsaParamInput(ParameterNodeImpl nodeFrom, SsaDefinitionExtNode nodeTo) { - nodeTo = getParameterDefNode(nodeFrom.getParameter()) - or - nodeTo.getDefinitionExt() = nodeFrom.(SelfParameterNodeImpl).getSelfDefinition() - } + // We could also use `class Parameter = ParameterNodeImpl`, which would be simpler, but + // it makes the `TNode` construction unecessarily recursive. + class Parameter extends TParameter { + NormalParameter asParameter() { this = TNormalParameter(result) } - /** - * Holds if there is a local use-use flow step from `nodeFrom` to `nodeTo` - * involving SSA definition `def`. - */ - predicate localSsaFlowStepUseUse(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { - SsaImpl::adjacentReadPairExt(def, nodeFrom.asExpr(), nodeTo.asExpr()) - } + MethodBase asMethodSelf() { this = TSelfMethodParameter(result) } - /** - * Holds if SSA definition `def` assigns `value` to the underlying variable. - * - * This is either a direct assignment, `x = value`, or an assignment via - * simple pattern matching - * - * ```rb - * case value - * in Foo => x then ... - * in y => then ... - * end - * ``` - */ - predicate ssaDefAssigns(Ssa::WriteDefinition def, CfgNodes::ExprCfgNode value) { - def.assigns(value) - or - exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::AstCfgNode pattern | - case.getValue() = value and - pattern = case.getBranch(_).(CfgNodes::ExprNodes::InClauseCfgNode).getPattern() - | - def.getWriteAccess() = pattern + Toplevel asToplevelSelf() { this = TSelfToplevelParameter(result) } + + ParameterNodeImpl toParameterNode() { + result = TNormalParameterNode(this.asParameter()) + or + result = TSelfMethodParameterNode(this.asMethodSelf()) + or + result = TSelfToplevelParameterNode(this.asToplevelSelf()) + } + + string toString() { + result = + [ + this.asParameter().toString(), this.asMethodSelf().toString(), + this.asToplevelSelf().toString() + ] + } + + Location getLocation() { + result = + [ + this.asParameter().getLocation(), this.asMethodSelf().getLocation(), + this.asToplevelSelf().getLocation() + ] + } + } + + predicate ssaDefInitializesParam(SsaImpl::WriteDefinition def, Parameter p) { + def = getParameterDef(p.asParameter()) or - def.getWriteAccess() = pattern.(CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess() - ) + def.(Ssa::SelfDefinition).getSourceVariable().getDeclaringScope() = + [p.asMethodSelf().(Scope), p.asToplevelSelf()] + } } - /** - * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving - * SSA definition `def`. - */ - pragma[nomagic] - predicate localSsaFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { - // Flow from assignment into SSA definition - ssaDefAssigns(def, nodeFrom.asExpr()) and - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def - or - // Flow from SSA definition to first read - def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and - SsaImpl::firstReadExt(def, nodeTo.asExpr()) - or - // Flow from post-update read to next read - localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode(), nodeTo) + module Impl = SsaImpl::Impl::DataFlowIntegration; + + Impl::Node asNode(Node n) { + n = TSsaNode(result) or - // Flow into phi (read) SSA definition node from def - localFlowSsaInputFromDef(nodeFrom, def, nodeTo) + result.(Impl::ExprNode).getExpr() = n.asExpr() or - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def and - def = nodeFrom.(SsaInputNode).getDefinitionExt() + result.(Impl::ExprPostUpdateNode).getExpr() = n.(PostUpdateNode).getPreUpdateNode().asExpr() or - localFlowSsaParamInput(nodeFrom, nodeTo) and - def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt() + result.(Impl::ParameterNode).getParameter().toParameterNode() = n } + predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { + Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) + } + + predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { + Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) + } + + module BarrierGuardsInput implements Impl::BarrierGuardsInputSig { + class Guard extends CfgNodes::AstCfgNode { + predicate hasCfgNode(SsaImpl::SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + } + + /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ + predicate guardControlsBlock(Guard guard, SsaImpl::SsaInput::BasicBlock bb, boolean branch) { + exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s | + guard = conditionBlock.getLastNode() and + s.getValue() = branch and + conditionBlock.controls(bb, s) + ) + } + + /** Gets an immediate conditional successor of basic block `bb`, if any. */ + SsaImpl::SsaInput::BasicBlock getAConditionalBasicBlockSuccessor( + SsaImpl::SsaInput::BasicBlock bb, boolean branch + ) { + exists(SuccessorTypes::ConditionalSuccessor s | + result = bb.getASuccessor(s) and + s.getValue() = branch + ) + } + } + + module BarrierGuardsImpl = Impl::BarrierGuards; +} + +/** Provides predicates related to local data flow. */ +module LocalFlow { pragma[nomagic] predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) { nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue() @@ -240,7 +261,7 @@ module LocalFlow { p.(KeywordParameter).getDefaultValue() = nodeFrom.asExpr().getExpr() ) or - nodeTo.(BlockArgumentNode).getParameterNode(true) = nodeFrom + nodeTo.(ImplicitBlockArgumentNode).getParameterNode(true) = nodeFrom } predicate flowSummaryLocalStep( @@ -253,21 +274,13 @@ module LocalFlow { } predicate localMustFlowStep(Node node1, Node node2) { - LocalFlow::localFlowSsaParamInput(node1, node2) - or - exists(SsaImpl::Definition def | - def.(Ssa::WriteDefinition).assigns(node1.asExpr()) and - node2.(SsaDefinitionExtNode).getDefinitionExt() = def - or - def = node1.(SsaDefinitionExtNode).getDefinitionExt() and - node2.asExpr() = SsaImpl::getARead(def) - ) + SsaFlow::localMustFlowStep(_, node1, node2) or node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs() or node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue() or - node1 = node2.(BlockArgumentNode).getParameterNode(true) + node1 = node2.(ImplicitBlockArgumentNode).getParameterNode(true) or node1 = unique(FlowSummaryNode n1 | @@ -347,7 +360,7 @@ module VariableCapture { or exists(Ssa::Definition def | def.getARead() = e2 and - LocalFlow::ssaDefAssigns(def.getAnUltimateDefinition(), e1) + SsaFlow::Input::ssaDefAssigns(def.getAnUltimateDefinition(), e1) ) } @@ -538,23 +551,14 @@ private module Cached { newtype TNode = TExprNode(CfgNodes::ExprCfgNode n) or TReturningNode(CfgNodes::ReturningCfgNode n) { exists(n.getReturnedValueNode()) } or - TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) or - TSsaInputNode(SsaInputDefinitionExt def, BasicBlock input) { - def.hasInputFromBlock(_, _, _, input) - } or + TSsaNode(SsaFlow::Impl::SsaNode node) or TCapturedVariableNode(VariableCapture::CapturedVariable v) or - TNormalParameterNode(Parameter p) { - p instanceof SimpleParameter or - p instanceof OptionalParameter or - p instanceof KeywordParameter or - p instanceof HashSplatParameter or - p instanceof SplatParameter - } or + TNormalParameterNode(NormalParameter p) or TSelfMethodParameterNode(MethodBase m) or TSelfToplevelParameterNode(Toplevel t) or TLambdaSelfReferenceNode(Callable c) { lambdaCreationExpr(_, _, c) } or - TBlockParameterNode(MethodBase m) or - TBlockArgumentNode(CfgNodes::ExprNodes::CallCfgNode yield) { + TImplicitBlockParameterNode(MethodBase m) { not m.getAParameter() instanceof BlockParameter } or + TImplicitBlockArgumentNode(CfgNodes::ExprNodes::CallCfgNode yield) { yield = any(BlockParameterNode b).getAYieldCall() } or TSynthHashSplatParameterNode(DataFlowCallable c) { @@ -600,7 +604,7 @@ private module Cached { class TSelfParameterNode = TSelfMethodParameterNode or TSelfToplevelParameterNode; class TSourceParameterNode = - TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or + TNormalParameterNode or TImplicitBlockParameterNode or TSelfParameterNode or TSynthHashSplatParameterNode or TSynthSplatParameterNode; cached @@ -619,15 +623,9 @@ private module Cached { LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or exists(SsaImpl::DefinitionExt def | + SsaFlow::localFlowStep(def, nodeFrom, nodeTo) and // captured variables are handled by the shared `VariableCapture` library - not def instanceof VariableCapture::CapturedSsaDefinitionExt - | - LocalFlow::localSsaFlowStep(def, nodeFrom, nodeTo) - or - LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and - not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) - or - LocalFlow::localFlowSsaInputFromRead(def, nodeFrom, nodeTo) and + not def instanceof VariableCapture::CapturedSsaDefinitionExt and not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) ) or @@ -643,11 +641,7 @@ private module Cached { predicate localFlowStepImpl(Node nodeFrom, Node nodeTo) { LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or - LocalFlow::localSsaFlowStep(_, nodeFrom, nodeTo) - or - LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) - or - LocalFlow::localFlowSsaInputFromRead(_, nodeFrom, nodeTo) + SsaFlow::localFlowStep(_, nodeFrom, nodeTo) or // Simple flow through library code is included in the exposed local // step relation, even though flow is technically inter-procedural @@ -661,11 +655,7 @@ private module Cached { predicate localFlowStepTypeTracker(Node nodeFrom, Node nodeTo) { LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or - LocalFlow::localSsaFlowStep(_, nodeFrom, nodeTo) - or - LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) - or - LocalFlow::localFlowSsaInputFromRead(_, nodeFrom, nodeTo) + SsaFlow::localFlowStep(_, nodeFrom, nodeTo) or VariableCapture::flowInsensitiveStep(nodeFrom, nodeTo) or @@ -675,7 +665,8 @@ private module Cached { /** Holds if `n` wraps an SSA definition without ingoing flow. */ private predicate entrySsaDefinition(SsaDefinitionExtNode n) { - n.getDefinitionExt() = any(SsaImpl::WriteDefinition def | not LocalFlow::ssaDefAssigns(def, _)) + n.getDefinitionExt() = + any(SsaImpl::WriteDefinition def | not SsaFlow::Input::ssaDefAssigns(def, _)) } pragma[nomagic] @@ -715,13 +706,13 @@ private module Cached { // Ensure all entry SSA definitions are local sources, except those that correspond // to parameters (which are themselves local sources) entrySsaDefinition(n) and - not LocalFlow::localFlowSsaParamInput(_, n) + not SsaFlow::Input::ssaDefInitializesParam(n.(SsaDefinitionExtNode).getDefinitionExt(), _) or isStoreTargetNode(n) or TypeTrackingInput::loadStep(_, n, _) or - n instanceof BlockArgumentNode + n instanceof ImplicitBlockArgumentNode } cached @@ -825,11 +816,7 @@ import Cached /** Holds if `n` should be hidden from path explanations. */ predicate nodeIsHidden(Node n) { - n.(SsaDefinitionExtNode).isHidden() - or - n instanceof SsaInputNode - or - n = LocalFlow::getParameterDefNode(_) + n.(SsaNode).isHidden() or exists(AstNode desug | isDesugarNode(desug) and @@ -861,33 +848,55 @@ predicate nodeIsHidden(Node n) { or n instanceof CaptureNode or - n instanceof BlockArgumentNode + n instanceof ImplicitBlockArgumentNode } -/** An SSA definition, viewed as a node in a data flow graph. */ -class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode { +/** An SSA node. */ +abstract class SsaNode extends NodeImpl, TSsaNode { + SsaFlow::Impl::SsaNode node; SsaImpl::DefinitionExt def; - SsaDefinitionExtNode() { this = TSsaDefinitionExtNode(def) } + SsaNode() { + this = TSsaNode(node) and + def = node.getDefinitionExt() + } - /** Gets the underlying SSA definition. */ SsaImpl::DefinitionExt getDefinitionExt() { result = def } + /** Holds if this node should be hidden from path explanations. */ + abstract predicate isHidden(); + + 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 SsaFlow::Impl::SsaDefinitionExtNode node; + /** Gets the underlying variable. */ Variable getVariable() { result = def.getSourceVariable() } - /** Holds if this node should be hidden from path explanations. */ - predicate isHidden() { + override predicate isHidden() { not def instanceof Ssa::WriteDefinition or isDesugarNode(def.(Ssa::WriteDefinition).getWriteAccess().getExpr()) + or + def = getParameterDef(_) } override CfgScope getCfgScope() { result = def.getBasicBlock().getScope() } +} + +class SsaDefinitionNodeImpl extends SsaDefinitionExtNode { + Ssa::Definition ssaDef; - override Location getLocationImpl() { result = def.getLocation() } + SsaDefinitionNodeImpl() { ssaDef = def } - override string toStringImpl() { result = def.toString() } + override Location getLocationImpl() { result = ssaDef.getLocation() } + + override string toStringImpl() { result = ssaDef.toString() } } /** @@ -925,20 +934,15 @@ class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode { * * both inputs into the phi read node after the outer condition are guarded. */ -class SsaInputNode extends NodeImpl, TSsaInputNode { - SsaImpl::DefinitionExt def; +class SsaInputNode extends SsaNode { + override SsaFlow::Impl::SsaInputNode node; BasicBlock input; - SsaInputNode() { this = TSsaInputNode(def, input) } + SsaInputNode() { node.isInputInto(def, input) } - /** Gets the underlying SSA definition. */ - SsaImpl::DefinitionExt getDefinitionExt() { result = def } + override predicate isHidden() { any() } override CfgScope getCfgScope() { result = input.getScope() } - - override Location getLocationImpl() { result = input.getLastNode().getLocation() } - - override string toStringImpl() { result = "[input] " + def } } /** An SSA definition for a `self` variable. */ @@ -1026,7 +1030,7 @@ private module ParameterNodes { * flow graph. */ class NormalParameterNode extends ParameterNodeImpl, TNormalParameterNode { - private Parameter parameter; + Parameter parameter; NormalParameterNode() { this = TNormalParameterNode(parameter) } @@ -1055,6 +1059,9 @@ private module ParameterNodes { // There are no positional parameters after the splat not exists(SimpleParameter p, int m | m > n | p = callable.getParameter(m)) ) + or + parameter = callable.getAParameter().(BlockParameter) and + pos.isBlock() ) } @@ -1162,25 +1169,33 @@ private module ParameterNodes { * The value of a block parameter at function entry, viewed as a node in a data * flow graph. */ - class BlockParameterNode extends ParameterNodeImpl, TBlockParameterNode { + abstract class BlockParameterNode extends ParameterNodeImpl { + abstract MethodBase getMethod(); + + CfgNodes::ExprNodes::CallCfgNode getAYieldCall() { + this.getMethod() = result.getExpr().(YieldCall).getEnclosingMethod() + } + } + + private class ExplicitBlockParameterNode extends BlockParameterNode, NormalParameterNode { + override BlockParameter parameter; + + final override MethodBase getMethod() { result.getAParameter() = parameter } + } + + private class ImplicitBlockParameterNode extends BlockParameterNode, TImplicitBlockParameterNode { private MethodBase method; - BlockParameterNode() { this = TBlockParameterNode(method) } + ImplicitBlockParameterNode() { this = TImplicitBlockParameterNode(method) } - final MethodBase getMethod() { result = method } + final override MethodBase getMethod() { result = method } - override Parameter getParameter() { - result = method.getAParameter() and result instanceof BlockParameter - } + override Parameter getParameter() { none() } override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { c.asCfgScope() = method and pos.isBlock() } - CfgNodes::ExprNodes::CallCfgNode getAYieldCall() { - this.getMethod() = result.getExpr().(YieldCall).getEnclosingMethod() - } - override CfgScope getCfgScope() { result = method } override Location getLocationImpl() { @@ -1452,10 +1467,10 @@ module ArgumentNodes { } } - class BlockArgumentNode extends NodeImpl, ArgumentNode, TBlockArgumentNode { + class ImplicitBlockArgumentNode extends NodeImpl, ArgumentNode, TImplicitBlockArgumentNode { CfgNodes::ExprNodes::CallCfgNode yield; - BlockArgumentNode() { this = TBlockArgumentNode(yield) } + ImplicitBlockArgumentNode() { this = TImplicitBlockArgumentNode(yield) } CfgNodes::ExprNodes::CallCfgNode getYieldCall() { result = yield } @@ -1893,7 +1908,7 @@ predicate jumpStep(Node pred, Node succ) { or any(AdditionalJumpStep s).step(pred, succ) or - succ.(BlockArgumentNode).getParameterNode(false) = pred + succ.(ImplicitBlockArgumentNode).getParameterNode(false) = pred } private ContentSet getArrayContent(int n) { @@ -2241,7 +2256,7 @@ private predicate lambdaCallExpr( */ predicate lambdaSourceCall(CfgNodes::ExprNodes::CallCfgNode call, LambdaCallKind kind, Node receiver) { kind = TYieldCallKind() and - call = receiver.(BlockArgumentNode).getYieldCall() + call = receiver.(ImplicitBlockArgumentNode).getYieldCall() or kind = TLambdaCallKind() and lambdaCallExpr(call, receiver.asExpr()) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index d4e07e0653e10..ceef160f92650 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -360,16 +360,12 @@ class PostUpdateNode extends Node { } /** An SSA definition, viewed as a node in a data flow graph. */ -class SsaDefinitionNode extends Node instanceof SsaDefinitionExtNode { - Ssa::Definition def; - - SsaDefinitionNode() { this = TSsaDefinitionExtNode(def) } - +class SsaDefinitionNode extends Node instanceof SsaDefinitionNodeImpl { /** Gets the underlying SSA definition. */ - Ssa::Definition getDefinition() { result = def } + Ssa::Definition getDefinition() { result = super.getDefinitionExt() } /** Gets the underlying variable. */ - Variable getVariable() { result = def.getSourceVariable() } + Variable getVariable() { result = this.getDefinition().getSourceVariable() } } cached @@ -870,57 +866,6 @@ private predicate sameSourceVariable(Ssa::Definition def1, Ssa::Definition def2) * in data flow and taint tracking. */ module BarrierGuard { - private import SsaImpl as SsaImpl - - pragma[nomagic] - private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) { - guardChecks(g, def.getARead(), branch) - } - - pragma[nomagic] - private predicate guardControlsSsaRead( - CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n - ) { - def.getARead() = n.asExpr() and - guardControlsBlock(g, n.asExpr().getBasicBlock(), branch) - } - - pragma[nomagic] - private predicate guardControlsPhiInput( - CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input, - SsaInputDefinitionExt phi - ) { - phi.hasInputFromBlock(def, _, _, input) and - ( - guardControlsBlock(g, input, branch) - or - exists(SuccessorTypes::ConditionalSuccessor s | - g = input.getLastNode() and - s.getValue() = branch and - input.getASuccessor(s) = phi.getBasicBlock() - ) - ) - } - - /** Gets a node that is safely guarded by the given guard check. */ - Node getABarrierNode() { - exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def | - guardChecksSsaDef(g, branch, def) and - guardControlsSsaRead(g, branch, def, result) - ) - or - exists( - CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input, - SsaInputDefinitionExt phi - | - guardChecksSsaDef(g, branch, def) and - guardControlsPhiInput(g, branch, def, input, phi) and - result = TSsaInputNode(phi, input) - ) - or - result.asExpr() = getAMaybeGuardedCapturedDef().getARead() - } - /** * Gets an implicit entry definition for a captured variable that * may be guarded, because a call to the capturing callable is guarded. @@ -928,6 +873,7 @@ module BarrierGuard { * This is restricted to calls where the variable is captured inside a * block. */ + pragma[nomagic] private Ssa::CapturedEntryDefinition getAMaybeGuardedCapturedDef() { exists( CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode, @@ -935,20 +881,25 @@ module BarrierGuard { | def.getARead() = testedNode and guardChecks(g, testedNode, branch) and - guardControlsBlock(g, call.getBasicBlock(), branch) and + SsaFlow::BarrierGuardsInput::guardControlsBlock(g, call.getBasicBlock(), branch) and result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock() and sameSourceVariable(def, result) ) } -} -/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ -private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) { - exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s | - guard = conditionBlock.getLastNode() and - s.getValue() = branch and - conditionBlock.controls(bb, s) - ) + private predicate guardChecksAdj( + SsaFlow::BarrierGuardsInput::Guard g, SsaFlow::Input::Expr e, boolean branch + ) { + guardChecks(g, e, branch) + } + + /** Gets a node that is safely guarded by the given guard check. */ + Node getABarrierNode() { + SsaFlow::asNode(result) = + SsaFlow::BarrierGuardsImpl::BarrierGuard::getABarrierNode() + or + result.asExpr() = getAMaybeGuardedCapturedDef().getARead() + } } /** diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index 8824d08130847..477110dbe4c6f 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -6,7 +6,7 @@ private import codeql.ruby.dataflow.SSA private import codeql.ruby.ast.Variable private import Cfg::CfgNodes::ExprNodes -private module SsaInput implements SsaImplCommon::InputSig { +module SsaInput implements SsaImplCommon::InputSig { private import codeql.ruby.controlflow.ControlFlowGraph as Cfg private import codeql.ruby.controlflow.BasicBlocks as BasicBlocks @@ -65,7 +65,7 @@ private module SsaInput implements SsaImplCommon::InputSig { } } -private import SsaImplCommon::Make as Impl +import SsaImplCommon::Make as Impl class Definition = Impl::Definition; @@ -283,15 +283,6 @@ private predicate adjacentDefSkipUncertainReads( SsaInput::variableRead(bb2, i2, _, true) } -/** Same as `adjacentDefReadExt`, but skips uncertain reads. */ -pragma[nomagic] -private predicate adjacentDefSkipUncertainReadsExt( - DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 -) { - adjacentDefReachesReadExt(def, bb1, i1, bb2, i2) and - SsaInput::variableRead(bb2, i2, _, true) -} - private predicate adjacentDefReachesUncertainReadExt( DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 ) { @@ -393,19 +384,6 @@ private module Cached { ) } - /** - * Holds if the value defined at SSA definition `def` can reach a read at `read`, - * without passing through any other non-pseudo read. - */ - cached - predicate firstReadExt(DefinitionExt def, VariableReadAccessCfgNode read) { - exists(Cfg::BasicBlock bb1, int i1, Cfg::BasicBlock bb2, int i2 | - def.definesAt(_, bb1, i1, _) and - adjacentDefSkipUncertainReadsExt(def, bb1, i1, bb2, i2) and - read = bb2.getNode(i2) - ) - } - /** * Holds if the read at `read2` is a read of the same SSA definition `def` * as the read at `read1`, and `read2` can be reached from `read1` without @@ -423,23 +401,6 @@ private module Cached { ) } - /** - * Holds if the read at `read2` is a read of the same SSA definition `def` - * as the read at `read1`, and `read2` can be reached from `read1` without - * passing through another non-pseudo read. - */ - cached - predicate adjacentReadPairExt( - DefinitionExt def, VariableReadAccessCfgNode read1, VariableReadAccessCfgNode read2 - ) { - exists(Cfg::BasicBlock bb1, int i1, Cfg::BasicBlock bb2, int i2 | - read1 = bb1.getNode(i1) and - variableReadActual(bb1, i1, _) and - adjacentDefSkipUncertainReadsExt(def, bb1, i1, bb2, i2) and - read2 = bb2.getNode(i2) - ) - } - /** * Holds if the read of `def` at `read` may be a last read. That is, `read` * can either reach another definition of the underlying source variable or @@ -454,28 +415,6 @@ private module Cached { ) } - /** - * Holds if the reference to `def` at index `i` in basic block `bb` can reach - * another definition `next` of the same underlying source variable, without - * passing through another write or non-pseudo read. - * - * The reference is either a read of `def` or `def` itself. - */ - cached - predicate lastRefBeforeRedefExt( - DefinitionExt def, Cfg::BasicBlock bb, int i, Cfg::BasicBlock input, DefinitionExt next - ) { - exists(LocalVariable v | - Impl::lastRefRedefExt(def, v, bb, i, input, next) and - not SsaInput::variableRead(bb, i, v, false) - ) - or - exists(SsaInput::BasicBlock bb0, int i0 | - Impl::lastRefRedefExt(def, _, bb0, i0, input, next) and - adjacentDefReachesUncertainReadExt(def, bb, i, bb0, i0) - ) - } - cached Definition uncertainWriteDefinitionInput(UncertainWriteDefinition def) { Impl::uncertainWriteDefinitionInput(def, result) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll index 903c8b562e763..6a17f8b74a9f4 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll @@ -89,7 +89,7 @@ private module Cached { clause = case.getBranch(_) and def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt() and def.getControlFlowNode() = variablesInPattern(clause.getPattern()) and - not LocalFlow::ssaDefAssigns(def, value) + not SsaFlow::Input::ssaDefAssigns(def, value) ) or // operation involving `nodeFrom`