From 1ca25b2ccb7da1942d5c2c52d905e1dcc8fd8102 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 9 Apr 2025 16:49:36 +0200 Subject: [PATCH 01/12] ruby: add test of `rb/uninitialized-local-variable` --- .../UninitializedLocal.expected | 14 ++++++ .../UninitializedLocal.qlref | 2 + .../UninitializedLocal/UninitializedLocal.rb | 46 +++++++++++++++++++ 3 files changed, 62 insertions(+) create mode 100644 ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected create mode 100644 ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.qlref create mode 100644 ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected new file mode 100644 index 000000000000..206af4ee3b4a --- /dev/null +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected @@ -0,0 +1,14 @@ +| UninitializedLocal.rb:12:3:12:3 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:7:8:7 | m | m | +| UninitializedLocal.rb:17:16:17:16 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:17:7:17:7 | a | a | +| UninitializedLocal.rb:30:3:30:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:31:3:31:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:32:3:32:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:32:8:32:8 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:33:3:33:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:33:14:33:14 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:33:20:33:20 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:34:3:34:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:34:15:34:15 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:34:21:34:21 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | +| UninitializedLocal.rb:44:13:44:13 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:40:11:40:11 | a | a | +| UninitializedLocal.rb:45:3:45:3 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:40:11:40:11 | a | a | diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.qlref b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.qlref new file mode 100644 index 000000000000..e37501dffbff --- /dev/null +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.qlref @@ -0,0 +1,2 @@ +query: queries/variables/UninitializedLocal.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb new file mode 100644 index 000000000000..bf591be50052 --- /dev/null +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb @@ -0,0 +1,46 @@ +def m + puts "m" +end + +def foo + m # calls m above + if false + m = 0 + m # reads local variable m + else + end + m #$ Alert + m2 # undefined local variable or method 'm2' for main (NameError) +end + +def test_guards + if (a = 3 && a) #$ Alert + a + end + if (a = 3) && a # OK - a is assigned in the previous conjunct + a + end + if !(a = 3) or a # OK - a is assigned in the previous conjunct + a + end + if false + b = 0 + end + b.nil? + b || 0 #$ SPURIOUS: Alert + b&.m #$ SPURIOUS: Alert + b if b #$ SPURIOUS: Alert + b.close if b && !b.closed #$ SPURIOUS: Alert + b.blowup if b || !b.blownup #$ Alert +end + +def test_loop + begin + if false + a = 0 + else + set_a + end + end until a #$ SPURIOUS: Alert + a #$ SPURIOUS: Alert +end \ No newline at end of file From 53c88da91ba08a10eefe832648d16db2c5eb5073 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 13:21:54 +0200 Subject: [PATCH 02/12] ruby: refine query for uninitialised local variables - there are places where uninitialised reads are intentional - there are also some places where they are impossible --- .../variables/UninitializedLocal.qhelp | 36 +++++++++ .../queries/variables/UninitializedLocal.ql | 63 +++++++++++++-- .../variables/examples/UninitializedLocal.rb | 14 ++++ .../UninitializedLocal.expected | 15 +--- .../UninitializedLocal/UninitializedLocal.rb | 76 ++++++++++++------- 5 files changed, 159 insertions(+), 45 deletions(-) create mode 100644 ruby/ql/src/queries/variables/UninitializedLocal.qhelp create mode 100644 ruby/ql/src/queries/variables/examples/UninitializedLocal.rb diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.qhelp b/ruby/ql/src/queries/variables/UninitializedLocal.qhelp new file mode 100644 index 000000000000..d06bdc7087f0 --- /dev/null +++ b/ruby/ql/src/queries/variables/UninitializedLocal.qhelp @@ -0,0 +1,36 @@ + + + +

+In Ruby, raw identifiers like x can be both local variable accesses and method calls. It is a local variable access iff it is syntactically preceded by something that binds it (like an assignment). +Consider the following example: +

+ + + +

+This will generate an alert on the last access to m, where it is not clear that the programmer intended to read from the local variable. +

+ + + + +

+Ensure that you check the control and data flow in the method carefully. +Check that the variable reference is spelled correctly, perhaps the variable has been renamed and the reference needs to be updated. +Another possibility is that an exception may be raised before the variable is assigned, in which case the read should be protected by a check for nil. +

+ + + + + + +
  • Wikipedia: Dead store.
    • + + + +     +     diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index df8a275431a4..5f0fb77872a8 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -5,20 +5,71 @@ * @kind problem * @problem.severity error * @id rb/uninitialized-local-variable - * @tags reliability + * @tags quality + * reliability * correctness - * @precision low + * @precision high */ import codeql.ruby.AST import codeql.ruby.dataflow.SSA +private import codeql.ruby.dataflow.internal.DataFlowPublic +import codeql.ruby.controlflow.internal.Guards as Guards +import codeql.ruby.controlflow.CfgNodes + +predicate isInBooleanContext(Expr e) { + e = any(ConditionalExpr c).getCondition() + or + e = any(ConditionalLoop l).getCondition() + or + e = any(LogicalAndExpr n).getAnOperand() + or + e = any(LogicalOrExpr n).getAnOperand() + or + e = any(NotExpr n).getOperand() +} + +predicate isGuarded(LocalVariableReadAccess read) { + exists(AstCfgNode guard, boolean branch | + Guards::guardControlsBlock(guard, read.getAControlFlowNode().getBasicBlock(), branch) + | + // guard is `var` + guard.getAstNode() = read.getVariable().getAnAccess() and + branch = true + or + // guard is `!var` + guard.getAstNode().(NotExpr).getOperand() = read.getVariable().getAnAccess() and + branch = false + or + // guard is `var.nil?` + exists(MethodCall c | guard.getAstNode() = c | + c.getReceiver() = read.getVariable().getAnAccess() and + c.getMethodName() = "nil?" + ) and + branch = false + or + // guard is `!var.nil?` + exists(MethodCall c | guard.getAstNode().(NotExpr).getOperand() = c | + c.getReceiver() = read.getVariable().getAnAccess() and + c.getMethodName() = "nil?" + ) and + branch = true + ) +} + +predicate isNilChecked(LocalVariableReadAccess read) { + exists(MethodCall c | c.getReceiver() = read | + c.getMethodName() = "nil?" + or + c.isSafeNavigation() + ) +} class RelevantLocalVariableReadAccess extends LocalVariableReadAccess { RelevantLocalVariableReadAccess() { - not exists(MethodCall c | - c.getReceiver() = this and - c.getMethodName() = "nil?" - ) + not isInBooleanContext(this) and + not isNilChecked(this) and + not isGuarded(this) } } diff --git a/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb b/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb new file mode 100644 index 000000000000..f7c79410ffc3 --- /dev/null +++ b/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb @@ -0,0 +1,14 @@ +def m + puts "m" +end + +def foo + m # calls m above + if false + m = 0 + m # reads local variable m + else + end + m # reads uninitialized local variable m, `nil` + m2 # undefined local variable or method 'm2' for main (NameError) +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected index 206af4ee3b4a..e8ab42107602 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected @@ -1,14 +1,3 @@ | UninitializedLocal.rb:12:3:12:3 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:7:8:7 | m | m | -| UninitializedLocal.rb:17:16:17:16 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:17:7:17:7 | a | a | -| UninitializedLocal.rb:30:3:30:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:31:3:31:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:32:3:32:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:32:8:32:8 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:33:3:33:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:33:14:33:14 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:33:20:33:20 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:34:3:34:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:34:15:34:15 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:34:21:34:21 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b | -| UninitializedLocal.rb:44:13:44:13 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:40:11:40:11 | a | a | -| UninitializedLocal.rb:45:3:45:3 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:40:11:40:11 | a | a | +| UninitializedLocal.rb:34:5:34:5 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b | +| UninitializedLocal.rb:34:23:34:23 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b | diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb index bf591be50052..6682a4bc1e03 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb @@ -14,33 +14,57 @@ def foo end def test_guards - if (a = 3 && a) #$ Alert - a - end - if (a = 3) && a # OK - a is assigned in the previous conjunct - a - end - if !(a = 3) or a # OK - a is assigned in the previous conjunct - a - end - if false - b = 0 - end - b.nil? - b || 0 #$ SPURIOUS: Alert - b&.m #$ SPURIOUS: Alert - b if b #$ SPURIOUS: Alert - b.close if b && !b.closed #$ SPURIOUS: Alert - b.blowup if b || !b.blownup #$ Alert + if (a = 3 && a) # OK - a is in a Boolean context + a + end + if (a = 3) && a # OK - a is assigned in the previous conjunct + a + end + if !(a = 3) or a # OK - a is assigned in the previous conjunct + a + end + if false + b = 0 + end + b.nil? + b || 0 # OK + b&.m # OK - safe navigation + b if b # OK + b.close if b && !b.closed # OK + b.blowup if b || !b.blownup #$ Alert + + if false + c = 0 + end + unless c + return + end + c # OK - given above unless + + if false + d = 0 + end + if (d.nil?) + return + end + d # OK - given above check + + if false + e = 0 + end + unless (!e.nil?) + return + end + e # OK - given above unless end def test_loop - begin - if false - a = 0 - else - set_a - end - end until a #$ SPURIOUS: Alert - a #$ SPURIOUS: Alert + begin + if false + a = 0 + else + set_a + end + end until a # OK + a # OK - given previous until end \ No newline at end of file From 8555e8c8c817e97218fb11ffe58c13e6e62cd334 Mon Sep 17 00:00:00 2001 From: yoff Date: Thu, 3 Apr 2025 12:12:44 +0200 Subject: [PATCH 03/12] ruby: add change notes --- .../2025-04-02-adjust-uninitialized-local-alert-message.md | 4 ++++ .../2025-04-02-adjust-uninitialized-local-metadata.md | 4 ++++ ruby/ql/src/queries/variables/UninitializedLocal.ql | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md create mode 100644 ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md diff --git a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md new file mode 100644 index 000000000000..201e6e8928af --- /dev/null +++ b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The query `rb/uninitialized-local-variable` now take various guards into account and should produce fewer false positives. It also now comes with a help file. diff --git a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md new file mode 100644 index 000000000000..d6c5f353a53e --- /dev/null +++ b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md @@ -0,0 +1,4 @@ +--- +category: queryMetadata +--- +* The query `rb/uninitialized-local-variable` now only produces alerts when it can find local flow; we have adjusted the precision to 'medium'. diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index 5f0fb77872a8..aa001ac9bc4d 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -8,7 +8,7 @@ * @tags quality * reliability * correctness - * @precision high + * @precision medium */ import codeql.ruby.AST From f675a143d6f62140318b299c4eecd33fa7f0a290 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 10:48:41 +0200 Subject: [PATCH 04/12] ruby: remove redundant cases The CFG handles the negation --- ruby/ql/src/queries/variables/UninitializedLocal.ql | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index aa001ac9bc4d..1633e20bee01 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -37,23 +37,12 @@ predicate isGuarded(LocalVariableReadAccess read) { guard.getAstNode() = read.getVariable().getAnAccess() and branch = true or - // guard is `!var` - guard.getAstNode().(NotExpr).getOperand() = read.getVariable().getAnAccess() and - branch = false - or // guard is `var.nil?` exists(MethodCall c | guard.getAstNode() = c | c.getReceiver() = read.getVariable().getAnAccess() and c.getMethodName() = "nil?" ) and branch = false - or - // guard is `!var.nil?` - exists(MethodCall c | guard.getAstNode().(NotExpr).getOperand() = c | - c.getReceiver() = read.getVariable().getAnAccess() and - c.getMethodName() = "nil?" - ) and - branch = true ) } From 4167e960580bc9692734451e202143d10ea85722 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 11:00:22 +0200 Subject: [PATCH 05/12] ruby: more complete impleemntation of `isInBooleanContext` Co-authored-by: Tom Hvitved --- .../queries/variables/UninitializedLocal.ql | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index 1633e20bee01..93e5c9b3adb3 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -17,19 +17,37 @@ private import codeql.ruby.dataflow.internal.DataFlowPublic import codeql.ruby.controlflow.internal.Guards as Guards import codeql.ruby.controlflow.CfgNodes -predicate isInBooleanContext(Expr e) { - e = any(ConditionalExpr c).getCondition() +private predicate isInBooleanContext(AstNode n) { + exists(ConditionalExpr i | + n = i.getCondition() + or + isInBooleanContext(i) and + n = i.getBranch(_) + ) + or + n = any(ConditionalLoop parent).getCondition() + or + n = any(InClause parent).getCondition() or - e = any(ConditionalLoop l).getCondition() + n = any(LogicalAndExpr op).getAnOperand() or - e = any(LogicalAndExpr n).getAnOperand() + n = any(LogicalOrExpr op).getAnOperand() or - e = any(LogicalOrExpr n).getAnOperand() + n = any(NotExpr op).getOperand() or - e = any(NotExpr n).getOperand() + n = any(StmtSequence parent | isInBooleanContext(parent)).getLastStmt() + or + exists(CaseExpr c, WhenClause w | + not exists(c.getValue()) and + c.getABranch() = w + | + w.getPattern(_) = n + or + w = n + ) } -predicate isGuarded(LocalVariableReadAccess read) { +private predicate isGuarded(LocalVariableReadAccess read) { exists(AstCfgNode guard, boolean branch | Guards::guardControlsBlock(guard, read.getAControlFlowNode().getBasicBlock(), branch) | @@ -46,7 +64,7 @@ predicate isGuarded(LocalVariableReadAccess read) { ) } -predicate isNilChecked(LocalVariableReadAccess read) { +private predicate isNilChecked(LocalVariableReadAccess read) { exists(MethodCall c | c.getReceiver() = read | c.getMethodName() = "nil?" or From 6e2cfab7b226e0c845ee36eb1deb31215000b4ce Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 12:46:25 +0200 Subject: [PATCH 06/12] ruby: add test for `for` found during triage --- .../UninitializedLocal/UninitializedLocal.expected | 2 ++ .../variables/UninitializedLocal/UninitializedLocal.rb | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected index e8ab42107602..5d0cdddf964f 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected @@ -1,3 +1,5 @@ | UninitializedLocal.rb:12:3:12:3 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:7:8:7 | m | m | | UninitializedLocal.rb:34:5:34:5 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b | | UninitializedLocal.rb:34:23:34:23 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b | +| UninitializedLocal.rb:73:9:73:9 | i | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:73:9:73:9 | i | i | +| UninitializedLocal.rb:76:5:76:5 | i | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:73:9:73:9 | i | i | diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb index 6682a4bc1e03..9761baac1da2 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb @@ -67,4 +67,11 @@ def test_loop end end until a # OK a # OK - given previous until +end + +def test_for + for i in 0..10 #$ SPURIOUS: Alert + i + end + i #$ SPURIOUS: Alert end \ No newline at end of file From b641d5f177637cf4ac6cfbca3a97fb934a7cb5c4 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 13:22:42 +0200 Subject: [PATCH 07/12] ruby: fix FP --- ruby/ql/src/queries/variables/UninitializedLocal.ql | 5 +++-- .../variables/UninitializedLocal/UninitializedLocal.expected | 1 - .../variables/UninitializedLocal/UninitializedLocal.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index 93e5c9b3adb3..0e819cf8a902 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -13,9 +13,9 @@ import codeql.ruby.AST import codeql.ruby.dataflow.SSA -private import codeql.ruby.dataflow.internal.DataFlowPublic import codeql.ruby.controlflow.internal.Guards as Guards import codeql.ruby.controlflow.CfgNodes +import codeql.ruby.ast.internal.Variable private predicate isInBooleanContext(AstNode n) { exists(ConditionalExpr i | @@ -72,7 +72,8 @@ private predicate isNilChecked(LocalVariableReadAccess read) { ) } -class RelevantLocalVariableReadAccess extends LocalVariableReadAccess { +class RelevantLocalVariableReadAccess extends LocalVariableReadAccess instanceof TVariableAccessReal +{ RelevantLocalVariableReadAccess() { not isInBooleanContext(this) and not isNilChecked(this) and diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected index 5d0cdddf964f..174d0d348a2a 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected @@ -1,5 +1,4 @@ | UninitializedLocal.rb:12:3:12:3 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:7:8:7 | m | m | | UninitializedLocal.rb:34:5:34:5 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b | | UninitializedLocal.rb:34:23:34:23 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b | -| UninitializedLocal.rb:73:9:73:9 | i | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:73:9:73:9 | i | i | | UninitializedLocal.rb:76:5:76:5 | i | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:73:9:73:9 | i | i | diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb index 9761baac1da2..9f4dc720ed08 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb @@ -70,7 +70,7 @@ def test_loop end def test_for - for i in 0..10 #$ SPURIOUS: Alert + for i in 0..10 # OK - since 0..10 cannot raise i end i #$ SPURIOUS: Alert From 2477233508ef3db2671182f45cf699ec4097dae3 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 15:01:57 +0200 Subject: [PATCH 08/12] ruby: only report on method calls Interviewing a Ruby developer, I learned that dealing with nil is common practice. So alerts are mostly useful, if we can point to a place where this has gone wrong. --- .../variables/UninitializedLocal.qhelp | 9 ++-- .../queries/variables/UninitializedLocal.ql | 3 +- .../variables/examples/UninitializedLocal.rb | 4 +- .../UninitializedLocal/UninitializedLocal.rb | 42 +++++++++---------- 4 files changed, 30 insertions(+), 28 deletions(-) diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.qhelp b/ruby/ql/src/queries/variables/UninitializedLocal.qhelp index d06bdc7087f0..078775e803fc 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.qhelp +++ b/ruby/ql/src/queries/variables/UninitializedLocal.qhelp @@ -12,6 +12,8 @@ Consider the following example:

    This will generate an alert on the last access to m, where it is not clear that the programmer intended to read from the local variable. +In fact, the last access to m is a method call, and the value of the local variable is nil, +so this will raise a NoMethodError.

    @@ -19,8 +21,7 @@ This will generate an alert on the last access to m, where it is no

    Ensure that you check the control and data flow in the method carefully. -Check that the variable reference is spelled correctly, perhaps the variable has been renamed and the reference needs to be updated. -Another possibility is that an exception may be raised before the variable is assigned, in which case the read should be protected by a check for nil. +Add a check for nil before the read, or rewrite the code to ensure that the variable is always initialized before being read.

    @@ -28,8 +29,8 @@ Another possibility is that an exception may be raised before the variable is as -
  • Wikipedia: Dead store.
    • - +
  • https://www.rubyguides.com/: Nil.
    • +
  • https://ruby-doc.org/: NoMethodError.
    •  diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index 0e819cf8a902..1cd496c86bec 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -77,7 +77,8 @@ class RelevantLocalVariableReadAccess extends LocalVariableReadAccess instanceof RelevantLocalVariableReadAccess() { not isInBooleanContext(this) and not isNilChecked(this) and - not isGuarded(this) + not isGuarded(this) and + this = any(MethodCall m).getReceiver() } } diff --git a/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb b/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb index f7c79410ffc3..56172f4df706 100644 --- a/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb +++ b/ruby/ql/src/queries/variables/examples/UninitializedLocal.rb @@ -5,10 +5,10 @@ def m def foo m # calls m above if false - m = 0 + m = "0" m # reads local variable m else end - m # reads uninitialized local variable m, `nil` + m.strip # reads uninitialized local variable m, `nil`, and crashes m2 # undefined local variable or method 'm2' for main (NameError) end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb index 9f4dc720ed08..7c82c8bf69e0 100644 --- a/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb @@ -5,57 +5,57 @@ def m def foo m # calls m above if false - m = 0 + m = "0" m # reads local variable m else end - m #$ Alert + m.strip #$ Alert m2 # undefined local variable or method 'm2' for main (NameError) end def test_guards - if (a = 3 && a) # OK - a is in a Boolean context - a + if (a = "3" && a) # OK - a is in a Boolean context + a.strip end - if (a = 3) && a # OK - a is assigned in the previous conjunct - a + if (a = "3") && a # OK - a is assigned in the previous conjunct + a.strip end - if !(a = 3) or a # OK - a is assigned in the previous conjunct - a + if !(a = "3") or a # OK - a is assigned in the previous conjunct + a.strip end if false - b = 0 + b = "0" end b.nil? b || 0 # OK - b&.m # OK - safe navigation - b if b # OK + b&.strip # OK - safe navigation + b.strip if b # OK b.close if b && !b.closed # OK b.blowup if b || !b.blownup #$ Alert if false - c = 0 + c = "0" end unless c return end - c # OK - given above unless + c.strip # OK - given above unless if false - d = 0 + d = "0" end if (d.nil?) return end - d # OK - given above check + d.strip # OK - given above check if false - e = 0 + e = "0" end unless (!e.nil?) return end - e # OK - given above unless + e.strip # OK - given above unless end def test_loop @@ -66,12 +66,12 @@ def test_loop set_a end end until a # OK - a # OK - given previous until + a.strip # OK - given previous until end def test_for - for i in 0..10 # OK - since 0..10 cannot raise - i + for i in ["foo", "bar"] # OK - since 0..10 cannot raise + puts i.strip end - i #$ SPURIOUS: Alert + i.strip #$ SPURIOUS: Alert end \ No newline at end of file From 6a76a40cf4385d2cd1b163a918cc16d795bc6c06 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 16:18:03 +0200 Subject: [PATCH 09/12] ruby: adjust change notes --- .../2025-04-02-adjust-uninitialized-local-alert-message.md | 4 ++-- .../2025-04-02-adjust-uninitialized-local-metadata.md | 2 +- ruby/ql/src/queries/variables/UninitializedLocal.ql | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md index 201e6e8928af..87b92ee51ce3 100644 --- a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md +++ b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md @@ -1,4 +1,4 @@ --- -category: minorAnalysis +category: majorAnalysis --- -* The query `rb/uninitialized-local-variable` now take various guards into account and should produce fewer false positives. It also now comes with a help file. +* The query `rb/uninitialized-local-variable` now only produces alerts when the variable is the receiver of a method call and should produce very few false positives. It also now comes with a help file. diff --git a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md index d6c5f353a53e..61c223981d57 100644 --- a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md +++ b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md @@ -1,4 +1,4 @@ --- category: queryMetadata --- -* The query `rb/uninitialized-local-variable` now only produces alerts when it can find local flow; we have adjusted the precision to 'medium'. +* The query `rb/uninitialized-local-variable` now only produces alerts when the variable is the receiver of a method call; we have adjusted the precision to 'high'. diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index 1cd496c86bec..c5b4fa18d62f 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -8,7 +8,7 @@ * @tags quality * reliability * correctness - * @precision medium + * @precision high */ import codeql.ruby.AST From eb0f8e9572cca851f296c9b5cc427a10a4fe18b1 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 16:27:21 +0200 Subject: [PATCH 10/12] ruby: add `rb/uninitialized-local-variable` to quality suite --- ruby/ql/src/codeql-suites/ruby-code-quality.qls | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ruby/ql/src/codeql-suites/ruby-code-quality.qls b/ruby/ql/src/codeql-suites/ruby-code-quality.qls index 33be91082d09..2111c6979ef9 100644 --- a/ruby/ql/src/codeql-suites/ruby-code-quality.qls +++ b/ruby/ql/src/codeql-suites/ruby-code-quality.qls @@ -2,4 +2,5 @@ - include: id: - rb/database-query-in-loop - - rb/useless-assignment-to-local \ No newline at end of file + - rb/useless-assignment-to-local + - rb/uninitialized-local-variable \ No newline at end of file From b988be8ff62ca0cfb544e557fae889c65d41ab98 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 21:29:01 +0200 Subject: [PATCH 11/12] ruby: improve help file This has improved autofixes I hope it also helps humans --- .../queries/variables/UninitializedLocal.md | 41 +++++++++++++++++++ .../variables/UninitializedLocal.qhelp | 37 ----------------- .../queries/variables/UninitializedLocal.ql | 13 +++++- 3 files changed, 53 insertions(+), 38 deletions(-) create mode 100644 ruby/ql/src/queries/variables/UninitializedLocal.md delete mode 100644 ruby/ql/src/queries/variables/UninitializedLocal.qhelp diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.md b/ruby/ql/src/queries/variables/UninitializedLocal.md new file mode 100644 index 000000000000..c99b24b11013 --- /dev/null +++ b/ruby/ql/src/queries/variables/UninitializedLocal.md @@ -0,0 +1,41 @@ +# Method call on `nil` + +## Description +In Ruby, it is not necessary to explicitly initialize variables. +If a local variable has not been explicitly initialized, it will have the value `nil`. If this happens unintended, though, the variable will not represent an object with the expected methods, and a method call on the variable will raise a `NoMethodError`. + +## Recommendation + +Ensure that the variable cannot be `nil` at the point hightligted by the alert. +This can be achieved by using a safe navigation or adding a check for `nil`. + +Note: You do not need to explicitly initialize the variable, if you can make the program deal with the possible `nil` value. In particular, initializing the variable to `nil` will have no effect, as this is already the value of the variable. If `nil` is the only possibly default value, you need to handle the `nil` value instead of initializing the variable. + +## Examples + +In the following code, the call to `create_file` may fail and then the call `f.close` will raise a `NoMethodError` since `f` will be `nil` at that point. + +```ruby +def dump(x) + f = create_file + f.puts(x) +ensure + f.close +end +``` + +We can fix this by using safe navigation: +```ruby +def dump(x) + f = create_file + f.puts(x) +ensure + f&.close +end +``` + +## References + +- https://www.rubyguides.com/: [Nil](https://www.rubyguides.com/2018/01/ruby-nil/) +- https://ruby-doc.org/: [NoMethodError](https://ruby-doc.org/core-2.6.5/NoMethodError.html) + diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.qhelp b/ruby/ql/src/queries/variables/UninitializedLocal.qhelp deleted file mode 100644 index 078775e803fc..000000000000 --- a/ruby/ql/src/queries/variables/UninitializedLocal.qhelp +++ /dev/null @@ -1,37 +0,0 @@ - - - -

    -In Ruby, raw identifiers like x can be both local variable accesses and method calls. It is a local variable access iff it is syntactically preceded by something that binds it (like an assignment). -Consider the following example: -

    - - - -

    -This will generate an alert on the last access to m, where it is not clear that the programmer intended to read from the local variable. -In fact, the last access to m is a method call, and the value of the local variable is nil, -so this will raise a NoMethodError. -

    - -     - - -

    -Ensure that you check the control and data flow in the method carefully. -Add a check for nil before the read, or rewrite the code to ensure that the variable is always initialized before being read. -

    - -     - - - - -
  • https://www.rubyguides.com/: Nil.
    • -
  • https://ruby-doc.org/: NoMethodError.
    • - - -     -     diff --git a/ruby/ql/src/queries/variables/UninitializedLocal.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index c5b4fa18d62f..2f5a4b875aa1 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -72,13 +72,24 @@ private predicate isNilChecked(LocalVariableReadAccess read) { ) } +/** + * Holds if `name` is the name of a method defined on `nil`. + * See https://ruby-doc.org/core-2.5.8/NilClass.html + */ +private predicate isNilMethodName(string name) { + name in [ + "inspect", "instance_of?", "is_a?", "kind_of?", "method", "nil?", "rationalize", "to_a", + "to_c", "to_f", "to_h", "to_i", "to_r", "to_s" + ] +} + class RelevantLocalVariableReadAccess extends LocalVariableReadAccess instanceof TVariableAccessReal { RelevantLocalVariableReadAccess() { not isInBooleanContext(this) and not isNilChecked(this) and not isGuarded(this) and - this = any(MethodCall m).getReceiver() + this = any(MethodCall m | not isNilMethodName(m.getMethodName())).getReceiver() } } From 7517272d34dcd965ba31c75eca363d87d38aabf4 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 11 Apr 2025 23:01:15 +0200 Subject: [PATCH 12/12] ruby: remove repetitive change note --- .../2025-04-02-adjust-uninitialized-local-metadata.md | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md diff --git a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md deleted file mode 100644 index 61c223981d57..000000000000 --- a/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-metadata.md +++ /dev/null @@ -1,4 +0,0 @@ ---- -category: queryMetadata ---- -* The query `rb/uninitialized-local-variable` now only produces alerts when the variable is the receiver of a method call; we have adjusted the precision to 'high'.