From a02735326ab703e5a833140ccc5ba072e8785454 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 27 Feb 2025 13:47:15 +0100 Subject: [PATCH 1/3] Ruby: Remove some DefinitionExt references and deprecate the rest. --- .../ruby/dataflow/internal/DataFlowPrivate.qll | 16 ++++++++-------- .../codeql/ruby/dataflow/internal/SsaImpl.qll | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll index d477550d1e84..baf36bd96398 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll @@ -74,10 +74,10 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) { /** Gets the SSA definition node corresponding to parameter `p`. */ pragma[nomagic] -SsaImpl::DefinitionExt getParameterDef(NamedParameter p) { +Ssa::Definition getParameterDef(NamedParameter p) { exists(BasicBlock bb, int i | bb.getNode(i).getAstNode() = p.getDefiningAccess() and - result.definesAt(_, bb, i, _) + result.definesAt(_, bb, i) ) } @@ -388,15 +388,15 @@ module VariableCapture { Flow::clearsContent(asClosureNode(node), c.getVariable()) } - class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt { - CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable } + class CapturedSsaDefinition extends SsaImpl::Definition { + CapturedSsaDefinition() { this.getSourceVariable() instanceof CapturedVariable } } // From an assignment or implicit initialization of a captured variable to its flow-insensitive node private predicate flowInsensitiveWriteStep( SsaDefinitionNodeImpl node1, CapturedVariableNode node2, CapturedVariable v ) { - exists(CapturedSsaDefinitionExt def | + exists(CapturedSsaDefinition def | def = node1.getDefinition() and def.getSourceVariable() = v and ( @@ -412,7 +412,7 @@ module VariableCapture { private predicate flowInsensitiveReadStep( CapturedVariableNode node1, SsaDefinitionNodeImpl node2, CapturedVariable v ) { - exists(CapturedSsaDefinitionExt def | + exists(CapturedSsaDefinition def | node1.getVariable() = v and def = node2.getDefinition() and def.getSourceVariable() = v and @@ -772,7 +772,7 @@ class SsaNode extends NodeImpl, TSsaNode { class SsaDefinitionNodeImpl extends SsaNode { override SsaImpl::DataFlowIntegration::SsaDefinitionNode node; - SsaImpl::Definition getDefinition() { result = node.getDefinition() } + Ssa::Definition getDefinition() { result = node.getDefinition() } override predicate isHidden() { exists(SsaImpl::Definition def | def = this.getDefinition() | @@ -2478,7 +2478,7 @@ module TypeInference { n = def or n.asExpr() = any(CfgNodes::ExprCfgNode read | - read = def.getDefinition().(SsaImpl::DefinitionExt).getARead() and + read = def.getDefinition().getARead() and not isTypeCheckedRead(read, _) // could in principle be checked against a new type ) ) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index f08178465b91..8973f40c911c 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -421,7 +421,7 @@ import Cached * * Only intended for internal use. */ -class DefinitionExt extends Impl::DefinitionExt { +deprecated class DefinitionExt extends Impl::DefinitionExt { VariableReadAccessCfgNode getARead() { result = getARead(this) } override string toString() { result = this.(Ssa::Definition).toString() } @@ -434,7 +434,7 @@ class DefinitionExt extends Impl::DefinitionExt { * * Only intended for internal use. */ -class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { +deprecated class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { override string toString() { result = "SSA phi read(" + this.getSourceVariable() + ")" } override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() } @@ -453,10 +453,10 @@ class NormalParameter extends Parameter { /** Gets the SSA definition node corresponding to parameter `p`. */ pragma[nomagic] -DefinitionExt getParameterDef(NamedParameter p) { +Definition getParameterDef(NamedParameter p) { exists(Cfg::BasicBlock bb, int i | bb.getNode(i).getAstNode() = p.getDefiningAccess() and - result.definesAt(_, bb, i, _) + result.definesAt(_, bb, i) ) } From c6761db2fc24080c92ee6cbce011b0c7f1858f0c Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 5 Mar 2025 11:45:10 +0100 Subject: [PATCH 2/3] SSA: Replace the Guards interface in the SSA data flow integration. --- .../code/csharp/dataflow/internal/SsaImpl.qll | 23 +++++++++---------- .../code/java/dataflow/internal/SsaImpl.qll | 16 ++++++------- .../dataflow/internal/sharedlib/Ssa.qll | 22 ++++++++++-------- .../codeql/ruby/dataflow/internal/SsaImpl.qll | 21 +++++++++-------- .../codeql/rust/dataflow/internal/SsaImpl.qll | 21 +++++++++-------- shared/ssa/codeql/ssa/Ssa.qll | 17 ++++++-------- 6 files changed, 62 insertions(+), 58 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll index c891bf45bbcd..f1299d5ad09e 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll @@ -1047,8 +1047,17 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu } class Guard extends Guards::Guard { - predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { - this.getAControlFlowNode() = bb.getNode(i) + /** + * Holds if the control flow branching from `bb1` is dependent on this guard, + * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this + * guard to `branch`. + */ + predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) { + exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s | + this.getAControlFlowNode() = bb1.getLastNode() and + bb2 = bb1.getASuccessorByType(s) and + s.getValue() = branch + ) } } @@ -1060,16 +1069,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu conditionBlock.edgeDominates(bb, s) ) } - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - ControlFlow::BasicBlock getAConditionalBasicBlockSuccessor( - ControlFlow::BasicBlock bb, boolean branch - ) { - exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s | - result = bb.getASuccessorByType(s) and - s.getValue() = branch - ) - } } private module DataFlowIntegrationImpl = Impl::DataFlowIntegration; diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll index d5343834ffe2..bfd8869a7e16 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll @@ -667,10 +667,13 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu } class Guard extends Guards::Guard { - predicate hasCfgNode(BasicBlock bb, int i) { - this = bb.getNode(i).asExpr() - or - this = bb.getNode(i).asStmt() + /** + * Holds if the control flow branching from `bb1` is dependent on this guard, + * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this + * guard to `branch`. + */ + predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) { + super.hasBranchEdge(bb1, bb2, branch) } } @@ -678,11 +681,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch) { guard.controls(bb, branch) } - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch) { - result = bb.(Guards::ConditionBlock).getTestSuccessor(branch) - } } private module DataFlowIntegrationImpl = Impl::DataFlowIntegration; diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll index 4fee3f98b171..c23032b34832 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll @@ -81,7 +81,19 @@ module SsaDataflowInput implements DataFlowIntegrationInputSig { class Guard extends js::ControlFlowNode { Guard() { this = any(js::ConditionGuardNode g).getTest() } - predicate hasCfgNode(js::BasicBlock bb, int i) { this = bb.getNode(i) } + /** + * Holds if the control flow branching from `bb1` is dependent on this guard, + * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this + * guard to `branch`. + */ + predicate controlsBranchEdge(js::BasicBlock bb1, js::BasicBlock bb2, boolean branch) { + exists(js::ConditionGuardNode g | + g.getTest() = this and + bb1 = this.getBasicBlock() and + bb2 = g.getBasicBlock() and + branch = g.getOutcome() + ) + } } pragma[inline] @@ -92,14 +104,6 @@ module SsaDataflowInput implements DataFlowIntegrationInputSig { branch = g.getOutcome() ) } - - js::BasicBlock getAConditionalBasicBlockSuccessor(js::BasicBlock bb, boolean branch) { - exists(js::ConditionGuardNode g | - bb = g.getTest().getBasicBlock() and - result = g.getBasicBlock() and - branch = g.getOutcome() - ) - } } import DataFlowIntegration diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index 8973f40c911c..d03332c62b16 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -515,21 +515,24 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) } class Guard extends Cfg::CfgNodes::AstCfgNode { - predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + /** + * Holds if the control flow branching from `bb1` is dependent on this guard, + * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this + * guard to `branch`. + */ + predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) { + exists(Cfg::SuccessorTypes::ConditionalSuccessor s | + this.getBasicBlock() = bb1 and + bb2 = bb1.getASuccessor(s) and + s.getValue() = branch + ) + } } /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) { Guards::guardControlsBlock(guard, bb, branch) } - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) { - exists(Cfg::SuccessorTypes::ConditionalSuccessor s | - result = bb.getASuccessor(s) and - s.getValue() = branch - ) - } } private module DataFlowIntegrationImpl = Impl::DataFlowIntegration; diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll index 5780c5b79766..60578160d5d3 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll @@ -361,7 +361,18 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu } class Guard extends CfgNodes::AstCfgNode { - predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + /** + * Holds if the control flow branching from `bb1` is dependent on this guard, + * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this + * guard to `branch`. + */ + predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) { + exists(Cfg::ConditionalSuccessor s | + this = bb1.getANode() and + bb2 = bb1.getASuccessor(s) and + s.getValue() = branch + ) + } } /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ @@ -372,14 +383,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu conditionBlock.edgeDominates(bb, s) ) } - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) { - exists(Cfg::ConditionalSuccessor s | - result = bb.getASuccessor(s) and - s.getValue() = branch - ) - } } private module DataFlowIntegrationImpl = Impl::DataFlowIntegration; diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index 5bdc2b8a28ad..300a36ee8151 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -1434,15 +1434,16 @@ module Make Input> { /** Gets a textual representation of this guard. */ string toString(); - /** Holds if the `i`th node of basic block `bb` evaluates this guard. */ - predicate hasCfgNode(BasicBlock bb, int i); + /** + * Holds if the control flow branching from `bb1` is dependent on this guard, + * and that the edge from `bb1` to `bb2` corresponds to the evaluation of this + * guard to `branch`. + */ + predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch); } /** Holds if `guard` controls block `bb` upon evaluating to `branch`. */ predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch); - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch); } /** @@ -1891,11 +1892,7 @@ module Make Input> { | DfInput::guardControlsBlock(g, bb, branch) or - exists(int last | - last = bb.length() - 1 and - g.hasCfgNode(bb, last) and - DfInput::getAConditionalBasicBlockSuccessor(bb, branch) = phi.getBasicBlock() - ) + g.controlsBranchEdge(bb, phi.getBasicBlock(), branch) ) ) } From 5e722eecf7853be6db8457801f69c9373695cd13 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 6 Mar 2025 13:31:31 +0100 Subject: [PATCH 3/3] Ruby: Push in casts to Definition to delete the then unused DefinitionExt. --- .../codeql/ruby/dataflow/internal/SsaImpl.qll | 37 +++---------------- 1 file changed, 5 insertions(+), 32 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index d03332c62b16..3c97fbaed895 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -218,7 +218,7 @@ private predicate hasVariableReadWithCapturedWrite( pragma[noinline] deprecated private predicate adjacentDefReadExt( - DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2, + Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2, SsaInput::SourceVariable v ) { Impl::adjacentDefReadExt(def, _, bb1, i1, bb2, i2) and @@ -226,10 +226,10 @@ deprecated private predicate adjacentDefReadExt( } deprecated private predicate adjacentDefReachesReadExt( - DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 + Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 ) { exists(SsaInput::SourceVariable v | adjacentDefReadExt(def, bb1, i1, bb2, i2, v) | - def.definesAt(v, bb1, i1, _) + def.definesAt(v, bb1, i1) or SsaInput::variableRead(bb1, i1, v, true) ) @@ -242,7 +242,7 @@ deprecated private predicate adjacentDefReachesReadExt( } deprecated private predicate adjacentDefReachesUncertainReadExt( - DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 + Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 ) { adjacentDefReachesReadExt(def, bb1, i1, bb2, i2) and SsaInput::variableRead(bb2, i2, _, false) @@ -251,7 +251,7 @@ deprecated private predicate adjacentDefReachesUncertainReadExt( /** Same as `lastRefRedef`, but skips uncertain reads. */ pragma[nomagic] deprecated private predicate lastRefSkipUncertainReadsExt( - DefinitionExt def, SsaInput::BasicBlock bb, int i + Definition def, SsaInput::BasicBlock bb, int i ) { Impl::lastRef(def, bb, i) and not SsaInput::variableRead(bb, i, def.getSourceVariable(), false) @@ -413,33 +413,6 @@ private module Cached { import Cached -/** - * An extended static single assignment (SSA) definition. - * - * This is either a normal SSA definition (`Definition`) or a - * phi-read node (`PhiReadNode`). - * - * Only intended for internal use. - */ -deprecated class DefinitionExt extends Impl::DefinitionExt { - VariableReadAccessCfgNode getARead() { result = getARead(this) } - - override string toString() { result = this.(Ssa::Definition).toString() } - - override Location getLocation() { result = this.(Ssa::Definition).getLocation() } -} - -/** - * A phi-read node. - * - * Only intended for internal use. - */ -deprecated class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { - override string toString() { result = "SSA phi read(" + this.getSourceVariable() + ")" } - - override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() } -} - class NormalParameter extends Parameter { NormalParameter() { this instanceof SimpleParameter or