From ce2d959b7ecc472b77e0cfe2c4b38a499f961ed4 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 25 Sep 2024 15:23:51 +0200 Subject: [PATCH 1/5] Shared: Add CFG consistency check for scopes with missing entry points --- shared/controlflow/codeql/controlflow/Cfg.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shared/controlflow/codeql/controlflow/Cfg.qll b/shared/controlflow/codeql/controlflow/Cfg.qll index aa7a072e4cbb..7f1eb54b2b02 100644 --- a/shared/controlflow/codeql/controlflow/Cfg.qll +++ b/shared/controlflow/codeql/controlflow/Cfg.qll @@ -1387,9 +1387,13 @@ module Make Input> { strictcount(sk.getListOrder()) > 1 } + /** Holds if `n` has multiple textual representations. */ query predicate multipleToString(Node n, string s) { s = strictconcat(n.toString(), ",") and strictcount(n.toString()) > 1 } + + /** Holds if CFG scope `scope` lacks an initial AST node. */ + query predicate scopeNoFirst(CfgScope scope) { not scopeFirst(scope, _) } } } From 1bd504bf61d19139b5917ff2cf96e1b3a002eb0e Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 25 Sep 2024 16:43:15 +0200 Subject: [PATCH 2/5] C#: Restrict `CfgScope` --- .../controlflow/internal/ControlFlowGraphImpl.qll | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll index b6efc78e55cf..74383240f5e3 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll @@ -15,11 +15,19 @@ class CfgScope extends Element, @top_level_exprorstmt_parent { CfgScope() { this.getFile().fromSource() and ( - this instanceof Callable + this = + any(Callable c | + c.(Constructor).hasInitializer() + or + InitializerSplitting::constructorInitializes(c, _) + or + c.hasBody() + ) or // For now, static initializer values have their own scope. Eventually, they // should be treated like instance initializers. - this.(Assignable).(Modifiable).isStatic() + this.(Assignable).(Modifiable).isStatic() and + expr_parent_top_level_adjusted2(_, _, this) ) } } From a3ad6f56975918fe9df5fec02bf0179b2a813aad Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 25 Sep 2024 16:43:33 +0200 Subject: [PATCH 3/5] Ruby: Weaken `scopeNoFirst` check --- ruby/ql/consistency-queries/CfgConsistency.ql | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ruby/ql/consistency-queries/CfgConsistency.ql b/ruby/ql/consistency-queries/CfgConsistency.ql index a57cb03ebb2b..c8d797b71f4c 100644 --- a/ruby/ql/consistency-queries/CfgConsistency.ql +++ b/ruby/ql/consistency-queries/CfgConsistency.ql @@ -1,4 +1,5 @@ -import codeql.ruby.controlflow.internal.ControlFlowGraphImpl::Consistency +import codeql.ruby.controlflow.internal.ControlFlowGraphImpl::Consistency as Consistency +import Consistency import codeql.ruby.AST import codeql.ruby.CFG import codeql.ruby.controlflow.internal.Completion @@ -19,3 +20,14 @@ query predicate nonPostOrderExpr(Expr e, string cls) { c instanceof NormalCompletion ) } + +query predicate scopeNoFirst(CfgScope scope) { + Consistency::scopeNoFirst(scope) and + not scope = any(StmtSequence seq | not exists(seq.getAStmt())) and + not scope = + any(Callable c | + not exists(c.getAParameter()) and + not c.(BodyStmt).hasEnsure() and + not exists(c.(BodyStmt).getARescue()) + ) +} From 24f39ccae2e503100c7f8809e31287ceae4e7f7d Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 26 Sep 2024 11:09:31 +0200 Subject: [PATCH 4/5] Rust: Weaken `scopeNoFirst` check --- rust/ql/consistency-queries/CfgConsistency.ql | 10 +++++++++- rust/ql/test/library-tests/controlflow/Cfg.ql | 4 ---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/rust/ql/consistency-queries/CfgConsistency.ql b/rust/ql/consistency-queries/CfgConsistency.ql index 6bcf8d88201c..ef7f9b6f2551 100644 --- a/rust/ql/consistency-queries/CfgConsistency.ql +++ b/rust/ql/consistency-queries/CfgConsistency.ql @@ -1,5 +1,7 @@ import rust -import codeql.rust.controlflow.internal.ControlFlowGraphImpl::Consistency +import codeql.rust.controlflow.internal.ControlFlowGraphImpl::Consistency as Consistency +import Consistency +import codeql.rust.controlflow.ControlFlowGraph import codeql.rust.controlflow.internal.ControlFlowGraphImpl as CfgImpl import codeql.rust.controlflow.internal.Completion @@ -17,3 +19,9 @@ query predicate nonPostOrderExpr(Expr e, string cls) { c instanceof NormalCompletion ) } + +query predicate scopeNoFirst(CfgScope scope) { + Consistency::scopeNoFirst(scope) and + not scope = any(Function f | not exists(f.getBody())) and + not scope = any(ClosureExpr c | not exists(c.getBody())) +} diff --git a/rust/ql/test/library-tests/controlflow/Cfg.ql b/rust/ql/test/library-tests/controlflow/Cfg.ql index dbafdaa6caa3..23b047b5b2ec 100644 --- a/rust/ql/test/library-tests/controlflow/Cfg.ql +++ b/rust/ql/test/library-tests/controlflow/Cfg.ql @@ -1,7 +1,3 @@ -/** - * @id rust/controlflow/cfg - */ - import rust import codeql.rust.controlflow.ControlFlowGraph import TestUtils From f389a889adaccfa6a98ed0e5fdee20eb917662b7 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 26 Sep 2024 11:05:16 +0200 Subject: [PATCH 5/5] Exclude consistency output from `.gitignore` files --- ql/.gitignore | 1 - ruby/.gitignore | 1 - 2 files changed, 2 deletions(-) diff --git a/ql/.gitignore b/ql/.gitignore index b8df693e8270..2e6fbbf1cc6e 100644 --- a/ql/.gitignore +++ b/ql/.gitignore @@ -3,5 +3,4 @@ target .cache ql/test/**/*.testproj ql/test/**/*.actual -ql/test/**/CONSISTENCY work diff --git a/ruby/.gitignore b/ruby/.gitignore index f0b40d44e7a3..2ad57b363153 100644 --- a/ruby/.gitignore +++ b/ruby/.gitignore @@ -3,5 +3,4 @@ extractor/target .cache ql/test/**/*.testproj ql/test/**/*.actual -ql/test/**/CONSISTENCY .codeql