From 23ec574dc22d0557d3426a8770a01c5eac923b0d Mon Sep 17 00:00:00 2001 From: yoff Date: Mon, 17 Mar 2025 11:41:39 +0100 Subject: [PATCH 1/7] ruby: ad 'quality' tag to 'rb/unused-parameter' --- ruby/ql/src/queries/variables/UnusedParameter.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/ruby/ql/src/queries/variables/UnusedParameter.ql b/ruby/ql/src/queries/variables/UnusedParameter.ql index d212ee883f74..1d008f136b88 100644 --- a/ruby/ql/src/queries/variables/UnusedParameter.ql +++ b/ruby/ql/src/queries/variables/UnusedParameter.ql @@ -7,6 +7,7 @@ * @id rb/unused-parameter * @tags maintainability * external/cwe/cwe-563 + * quality * @precision low */ From 49f383a3fe616c11949bafb338ddf56f459f2c62 Mon Sep 17 00:00:00 2001 From: yoff Date: Mon, 17 Mar 2025 20:59:26 +0100 Subject: [PATCH 2/7] Ruby: exclude methods with calls to 'super' when the call has no arguments --- ruby/ql/src/queries/variables/UnusedParameter.ql | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ruby/ql/src/queries/variables/UnusedParameter.ql b/ruby/ql/src/queries/variables/UnusedParameter.ql index 1d008f136b88..0a1ddf2a5302 100644 --- a/ruby/ql/src/queries/variables/UnusedParameter.ql +++ b/ruby/ql/src/queries/variables/UnusedParameter.ql @@ -25,5 +25,9 @@ class RelevantParameterVariable extends LocalVariable { from RelevantParameterVariable v where - not exists(Ssa::WriteDefinition def | def.getWriteAccess().getAstNode() = v.getDefiningAccess()) + not exists(Ssa::WriteDefinition def | def.getWriteAccess().getAstNode() = v.getDefiningAccess()) and + not exists(SuperCall s | s.getEnclosingCallable().getAParameter().getAVariable() = v | + // a call to 'super' without any arguments will pass on the parameter. + not exists(s.getAnArgument()) + ) select v, "The parameter '" + v.getName() + "' is never used." From 1867b366a01d56d5ff588636dd3d35f49de7f166 Mon Sep 17 00:00:00 2001 From: yoff Date: Mon, 17 Mar 2025 21:00:13 +0100 Subject: [PATCH 3/7] ruby: exclude uses that are part of || these can safely be uninitialised --- ruby/ql/src/queries/variables/UninitializedLocal.ql | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index df8a275431a4..2292221240d3 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -18,6 +18,10 @@ class RelevantLocalVariableReadAccess extends LocalVariableReadAccess { not exists(MethodCall c | c.getReceiver() = this and c.getMethodName() = "nil?" + ) and + not exists(BinaryOperation b | + b.getLeftOperand() = this and + b.getOperator() = "||" ) } } From c6fd5e56b128fd2f4c3d33add2525fde2970ca3e Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 25 Mar 2025 11:31:22 +0100 Subject: [PATCH 4/7] ruby: add tests for dead store --- .../library-tests/controlflow/graph/raise.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ruby/ql/test/library-tests/controlflow/graph/raise.rb b/ruby/ql/test/library-tests/controlflow/graph/raise.rb index 3caf234ab14c..f73303e504c9 100644 --- a/ruby/ql/test/library-tests/controlflow/graph/raise.rb +++ b/ruby/ql/test/library-tests/controlflow/graph/raise.rb @@ -180,3 +180,23 @@ def m16(b1, b2) return 3 end end + +def m17(b1, b2) + begin + raise ExceptionA if b1 + rescue ExceptionA + if b2 + b1 = false + retry + end +end + +def m18(b2) + b1 = true + begin + raise ExceptionA if b1 + rescue ExceptionA + if b2 + b1 = false + end +end From ffb1a74615ea404c50f3736a844acccbe873ee42 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 25 Mar 2025 11:31:53 +0100 Subject: [PATCH 5/7] ruby: start enabling retry stmt we should convince ourselves, that it is not enabled already.. --- ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll index 83ea11e9d230..b7340473dbe8 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll @@ -69,6 +69,9 @@ private predicate completionIsValidForStmt(AstNode n, Completion c) { or n instanceof ReturnStmt and c = TReturnCompletion() + or + n instanceof RetryStmt and + c = TRetryCompletion() } private AstNode getARescuableBodyChild() { From ed9640a8c432e58dc8f67cd150dbe33ad162af16 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 26 Mar 2025 00:47:46 +0100 Subject: [PATCH 6/7] ruby: fix test syntax --- ruby/ql/test/library-tests/controlflow/graph/raise.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ruby/ql/test/library-tests/controlflow/graph/raise.rb b/ruby/ql/test/library-tests/controlflow/graph/raise.rb index f73303e504c9..0b3b3a16eac5 100644 --- a/ruby/ql/test/library-tests/controlflow/graph/raise.rb +++ b/ruby/ql/test/library-tests/controlflow/graph/raise.rb @@ -188,6 +188,7 @@ def m17(b1, b2) if b2 b1 = false retry + end end end @@ -198,5 +199,6 @@ def m18(b2) rescue ExceptionA if b2 b1 = false + end end end From 87d3a8800528203c312dd6e9c3201ff6c6b7e856 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 26 Mar 2025 00:48:14 +0100 Subject: [PATCH 7/7] ruby: add control flow edge for retry --- .../ruby/controlflow/internal/ControlFlowGraphImpl.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll index dd672ba982d7..fc4cc7916bbd 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll @@ -1319,6 +1319,11 @@ module Trees { last(super.getBody(), pred, c) and c instanceof NormalCompletion and succ = this + or + pred = super.getBody().getAStmt().getAChild*() and + pred instanceof RetryStmt and + c instanceof RetryCompletion and + exists(BodyStmtTree stmt | this = stmt.getARescue() | first(stmt, succ)) } }