-
Notifications
You must be signed in to change notification settings - Fork 0
Unified: Add control flow graph. #7
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
base: qa/agent-github-codeql/pr-07-22479/base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| import unified | ||
| import ControlFlow::Consistency |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| name: codeql/unified-consistency-queries | ||
| groups: [unified, test, consistency-queries] | ||
| dependencies: | ||
| codeql/unified-all: ${workspace} | ||
| warnOnImplicitThis: true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,277 @@ | ||
| /** | ||
| * Provides classes representing the control flow graph within callables. | ||
| */ | ||
| overlay[local?] | ||
| module; | ||
|
|
||
| private import unified | ||
| private import codeql.controlflow.ControlFlowGraph | ||
| private import codeql.controlflow.SuccessorType | ||
|
|
||
| private module Cfg0 = Make0<Location, Ast>; | ||
|
|
||
| private module Cfg1 = Make1<Input>; | ||
|
|
||
| private module Cfg2 = Make2<Input>; | ||
|
|
||
| private import Cfg0 | ||
| private import Cfg1 | ||
| private import Cfg2 | ||
| import Public | ||
|
|
||
| /** Provides an implementation of the AST signature for Unified. */ | ||
| private module Ast implements AstSig<Location> { | ||
| private import unified as U | ||
|
|
||
| class AstNode = U::AstNode; | ||
|
|
||
| private predicate skipControlFlow(AstNode e) { e instanceof Modifier or e instanceof Identifier } | ||
|
|
||
| AstNode getChild(AstNode n, int index) { | ||
| result.getParent() = n and | ||
| result.getParentIndex() = index and | ||
| not n instanceof Callable and | ||
| not skipControlFlow(n) and | ||
| not skipControlFlow(result) | ||
| } | ||
|
|
||
| Callable getEnclosingCallable(AstNode node) { | ||
| exists(AstNode parent | parent = node.getParent() | | ||
| result = parent | ||
| or | ||
| not parent instanceof Callable and | ||
| result = getEnclosingCallable(parent) | ||
| ) | ||
| } | ||
|
|
||
| class Callable = U::Callable; | ||
|
|
||
| AstNode callableGetBody(Callable c) { | ||
| result = c.(AccessorDeclaration).getBody() or | ||
| result = c.(ConstructorDeclaration).getBody() or | ||
| result = c.(DestructorDeclaration).getBody() or | ||
| result = c.(FunctionDeclaration).getBody() or | ||
| result = c.(FunctionExpr).getBody() or | ||
| result = c.(InitializerDeclaration).getBody() or | ||
| result = c.(TopLevel).getBody() | ||
| } | ||
|
|
||
| class Parameter extends U::Parameter { | ||
| Expr getDefaultValue() { result = super.getDefault() } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · CRITICAL The 'Ast' module implementing 'AstSig<Location>' contains numerous stub classes with 'none()' bodies (e.g., 'ExprStmt', 'IfStmt', 'ForStmt', 'GotoStmt', 'Assignment', 'UntilStmt'). Impact: The 'Ast' module implementing 'AstSig<Location>' contains numerous stub classes with 'none()' bodies (e.g., 'ExprStmt', 'IfStmt', 'ForStmt', 'GotoStmt', 'Assignment', 'UntilStmt'). Any CFG construction that encounters these node types will silently produce no control-flow edges, yielding an incomplete or incorrect CFG. This can cause downstream analyses (e.g., taint, dataflow, dead-code) to miss paths and prod… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
|
|
||
| AstNode getPattern() { result = super.getPattern() } | ||
| } | ||
|
|
||
| Parameter callableGetParameter(Callable c, int index) { | ||
| result = c.(AccessorDeclaration).getParameter(index) or | ||
| result = c.(ConstructorDeclaration).getParameter(index) or | ||
| result = c.(FunctionDeclaration).getParameter(index) or | ||
| result = c.(FunctionExpr).getParameter(index) | ||
| } | ||
|
|
||
| class Stmt = U::Stmt; | ||
|
|
||
| class Expr = U::Expr; | ||
|
|
||
| class BlockStmt = U::Block; | ||
|
|
||
| class ExprStmt extends Stmt { | ||
| ExprStmt() { none() } | ||
|
|
||
| Expr getExpr() { none() } | ||
| } | ||
|
|
||
| class IfStmt extends Stmt { | ||
| IfStmt() { none() } | ||
|
|
||
| Expr getCondition() { none() } | ||
|
|
||
| Stmt getThen() { none() } | ||
|
|
||
| Stmt getElse() { none() } | ||
| } | ||
|
|
||
| abstract class LoopStmt extends Stmt { | ||
| Stmt getBody() { none() } | ||
| } | ||
|
|
||
| class WhileStmt extends LoopStmt instanceof U::WhileStmt { | ||
| override Stmt getBody() { result = U::WhileStmt.super.getBody() } | ||
|
|
||
| Expr getCondition() { result = super.getCondition() } | ||
| } | ||
|
|
||
| class DoStmt extends LoopStmt instanceof U::DoWhileStmt { | ||
| override Stmt getBody() { result = U::DoWhileStmt.super.getBody() } | ||
|
|
||
| Expr getCondition() { result = super.getCondition() } | ||
| } | ||
|
|
||
| class UntilStmt extends LoopStmt { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The 'Ast' module is a large hand-written adapter with many 'none()' stubs and TODO comments (e.g., '// TODO support foreach guard', '// TODO: sort out the relationship between Bina Impact: The 'Ast' module is a large hand-written adapter with many 'none()' stubs and TODO comments (e.g., '// TODO support foreach guard', '// TODO: sort out the relationship between BinaryExpr and Assignment'). A new maintainer cannot determine which language constructs are actually supported by the CFG without exhaustively reading every stub. This is a maintainability hazard and likely to cause incorrect assumptions abou… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| UntilStmt() { none() } | ||
|
|
||
| Expr getCondition() { none() } | ||
| } | ||
|
|
||
| class ForStmt extends LoopStmt { | ||
| ForStmt() { none() } | ||
|
|
||
| AstNode getInit(int index) { none() } | ||
|
|
||
| Expr getCondition() { none() } | ||
|
|
||
| AstNode getUpdate(int index) { none() } | ||
| } | ||
|
|
||
| class ForeachStmt extends LoopStmt instanceof U::ForEachStmt { | ||
| override Stmt getBody() { result = U::ForEachStmt.super.getBody() } | ||
|
|
||
| // TODO support foreach guard | ||
| // | ||
| // TODO: Expr != Pattern | ||
| Expr getVariable() { result = super.getPattern() } | ||
|
|
||
| Expr getCollection() { result = super.getIterable() } | ||
| } | ||
|
|
||
| class BreakStmt = U::BreakExpr; | ||
|
|
||
| class ContinueStmt = U::ContinueExpr; | ||
|
|
||
| class GotoStmt extends Stmt { | ||
| GotoStmt() { none() } | ||
| } | ||
|
|
||
| class ReturnStmt extends U::ReturnExpr { | ||
| Expr getExpr() { result = super.getValue() } | ||
| } | ||
|
|
||
| class Throw extends U::ThrowExpr { | ||
| Expr getExpr() { result = super.getValue() } | ||
| } | ||
|
|
||
| class TryStmt extends U::TryExpr { | ||
| AstNode getBody(int index) { index = 0 and result = super.getBody() } | ||
|
|
||
| CatchClause getCatch(int index) { result = super.getCatchClause(index) } | ||
|
|
||
| Stmt getFinally() { none() } | ||
| } | ||
|
|
||
| class CatchClause extends U::CatchClause { | ||
| AstNode getPattern() { result = super.getPattern() } | ||
|
|
||
| AstNode getVariable() { none() } | ||
|
|
||
| Expr getCondition() { none() } | ||
|
|
||
| Stmt getBody() { result = super.getBody() } | ||
| } | ||
|
|
||
| class Switch extends U::SwitchExpr { | ||
| Expr getExpr() { result = super.getValue() } | ||
|
|
||
| Case getCase(int index) { result = super.getCase(index) } | ||
|
|
||
| Stmt getStmt(int index) { none() } | ||
| } | ||
|
|
||
| class Case extends U::SwitchCase { | ||
| AstNode getPattern(int index) { result = super.getPattern() and index = 0 } | ||
|
|
||
| Expr getGuard() { none() } | ||
|
|
||
| AstNode getBody() { result = super.getBody() } | ||
| } | ||
|
|
||
| class DefaultCase extends Case { | ||
| DefaultCase() { not exists(super.getPattern()) } | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · CRITICAL 'BooleanLiteral.getValue()' is implemented as 'result.toString() = super.getValue()'. Impact: 'BooleanLiteral.getValue()' is implemented as 'result.toString() = super.getValue()'. This compares a string representation of a boolean result to the raw value, which is type-incompatible or always false depending on QL type resolution. If this compiles, it will never produce a true value, breaking CFG handling of boolean literals and any conditional logic depending on them. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| class ConditionalExpr = U::IfExpr; | ||
|
|
||
| // TODO: sort out the relationship between BinaryExpr and Assignment | ||
| class BinaryExpr extends U::BinaryExpr { | ||
| Expr getLeftOperand() { result = super.getLeft() } | ||
|
|
||
| Expr getRightOperand() { result = super.getRight() } | ||
| } | ||
|
|
||
| class LogicalAndExpr extends BinaryExpr, U::LogicalAndExpr { } | ||
|
|
||
| class LogicalOrExpr extends BinaryExpr, U::LogicalOrExpr { } | ||
|
|
||
| class NullCoalescingExpr extends BinaryExpr, U::NullCoalescingExpr { } | ||
|
|
||
| class UnaryExpr = U::UnaryExpr; | ||
|
|
||
| class LogicalNotExpr = U::LogicalNotExpr; | ||
|
|
||
| // TODO | ||
| class Assignment extends BinaryExpr { | ||
| Assignment() { none() } | ||
| } | ||
|
|
||
| class AssignExpr extends Assignment { } | ||
|
|
||
| class CompoundAssignment extends Assignment { } | ||
|
|
||
| class AssignLogicalAndExpr extends CompoundAssignment { } | ||
|
|
||
| class AssignLogicalOrExpr extends CompoundAssignment { } | ||
|
|
||
| class AssignNullCoalescingExpr extends CompoundAssignment { } | ||
|
|
||
| class BooleanLiteral extends U::BooleanLiteral { | ||
| boolean getValue() { result.toString() = super.getValue() } | ||
| } | ||
|
|
||
| class PatternMatchExpr extends U::PatternGuardExpr { | ||
| Expr getExpr() { result = super.getValue() } | ||
|
|
||
| AstNode getPattern() { result = super.getPattern() } | ||
| } | ||
| } | ||
|
|
||
| private module Input implements InputSig1, InputSig2 { | ||
| private import codeql.util.Void | ||
|
|
||
| predicate cfgCachedStageRef() { CfgCachedStage::ref() } | ||
|
|
||
| class Label extends string { | ||
| Label() { | ||
| any(LabeledStmt l).getLabel().getValue() = this or | ||
| any(BreakExpr b).getLabel().getValue() = this or | ||
| any(ContinueExpr c).getLabel().getValue() = this | ||
| } | ||
|
|
||
| string toString() { result = this } | ||
| } | ||
|
|
||
| private Label getLabelOfStmt(Stmt s) { | ||
| exists(LabeledStmt l | s = l.getStmt() | | ||
| result = l.getLabel().getValue() or | ||
| result = getLabelOfStmt(l) | ||
| ) | ||
| } | ||
|
|
||
| predicate hasLabel(Ast::AstNode n, Label l) { | ||
| l = getLabelOfStmt(n) | ||
| or | ||
| l = n.(BreakExpr).getLabel().getValue() | ||
| or | ||
| l = n.(ContinueExpr).getLabel().getValue() | ||
| } | ||
|
|
||
| class CallableContext = Void; | ||
|
|
||
| predicate beginAbruptCompletion( | ||
| AstNode ast, PreControlFlowNode n, AbruptCompletion c, boolean always | ||
| ) { | ||
| none() | ||
| } | ||
|
|
||
| predicate endAbruptCompletion(AstNode ast, PreControlFlowNode n, AbruptCompletion c) { none() } | ||
|
|
||
| predicate step(PreControlFlowNode n1, PreControlFlowNode n2) { none() } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,14 @@ module Unified { | |
| } | ||
| } | ||
|
|
||
| /** A block statement. */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH 'Block.getLastStmt()' uses 'not exists(this.getStmt(i + 1))' to find the last statement. Impact: 'Block.getLastStmt()' uses 'not exists(this.getStmt(i + 1))' to find the last statement. This relies on 'getStmt' indices being contiguous and starting at 0. If the underlying AST representation has gaps or non-zero-based indexing, this predicate will return no result or the wrong statement, and the intent is not documented. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| class Block extends G::Block { | ||
| /** Gets the last statement in this block. */ | ||
| Stmt getLastStmt() { | ||
| exists(int i | result = this.getStmt(i) and not exists(this.getStmt(i + 1))) | ||
| } | ||
| } | ||
|
|
||
| /** An expression */ | ||
| class Expr extends G::Expr { | ||
| /** Gets the string value of this expression, if it is a known string constant. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The new 'printCfg.ql' query is tagged 'ide-contextual-queries/print-cfg' and exposes a graph representation of a file's CFG via external predicates ('selectedSourceFile', 'selected Impact: The new 'printCfg.ql' query is tagged 'ide-contextual-queries/print-cfg' and exposes a graph representation of a file's CFG via external predicates ('selectedSourceFile', 'selectedSourceLine', 'selectedSourceColumn'). If this query is runnable in an environment where source file paths or line/column inputs are attacker-influenced, it could be used to exfiltrate code structure or probe internal source layout. The que… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| * @name Print CFG | ||
| * @description Produces a representation of a file's Control Flow Graph. | ||
| * This query is used by the VS Code extension. | ||
| * @id unified/print-cfg | ||
| * @kind graph | ||
| * @tags ide-contextual-queries/print-cfg | ||
| */ | ||
|
|
||
| private import unified | ||
| private import codeql.Locations | ||
|
|
||
| external string selectedSourceFile(); | ||
|
|
||
| private predicate selectedSourceFileAlias = selectedSourceFile/0; | ||
|
|
||
| external int selectedSourceLine(); | ||
|
|
||
| private predicate selectedSourceLineAlias = selectedSourceLine/0; | ||
|
|
||
| external int selectedSourceColumn(); | ||
|
|
||
| private predicate selectedSourceColumnAlias = selectedSourceColumn/0; | ||
|
|
||
| module ViewCfgQueryInput implements ControlFlow::ViewCfgQueryInputSig<File> { | ||
| predicate selectedSourceFile = selectedSourceFileAlias/0; | ||
|
|
||
| predicate selectedSourceLine = selectedSourceLineAlias/0; | ||
|
|
||
| predicate selectedSourceColumn = selectedSourceColumnAlias/0; | ||
|
|
||
| predicate cfgScopeSpan( | ||
| Callable scope, File file, int startLine, int startColumn, int endLine, int endColumn | ||
| ) { | ||
| file = scope.getFile() and | ||
| scope.getLocation().hasLocationInfo(_, startLine, startColumn, endLine, endColumn) | ||
| } | ||
| } | ||
|
|
||
| import ControlFlow::ViewCfgQuery<File, ViewCfgQueryInput> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shipwright · HIGH
'getEnclosingCallable' recurses through parents but stops only when the parent is a 'Callable'.
Impact: 'getEnclosingCallable' recurses through parents but stops only when the parent is a 'Callable'. If an AST node is not enclosed by any callable (e.g., top-level declarations outside a 'TopLevel' body), the recursion can fail to produce a result or traverse unexpectedly, leaving CFG nodes without an enclosing callable and breaking scope-based queries.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.