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..87b92ee51ce3 --- /dev/null +++ b/ruby/ql/src/change-notes/2025-04-02-adjust-uninitialized-local-alert-message.md @@ -0,0 +1,4 @@ +--- +category: majorAnalysis +--- +* 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/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 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.ql b/ruby/ql/src/queries/variables/UninitializedLocal.ql index df8a275431a4..2f5a4b875aa1 100644 --- a/ruby/ql/src/queries/variables/UninitializedLocal.ql +++ b/ruby/ql/src/queries/variables/UninitializedLocal.ql @@ -5,20 +5,91 @@ * @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 +import codeql.ruby.controlflow.internal.Guards as Guards +import codeql.ruby.controlflow.CfgNodes +import codeql.ruby.ast.internal.Variable -class RelevantLocalVariableReadAccess extends LocalVariableReadAccess { - RelevantLocalVariableReadAccess() { - not exists(MethodCall c | - c.getReceiver() = this and +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 + n = any(LogicalAndExpr op).getAnOperand() + or + n = any(LogicalOrExpr op).getAnOperand() + or + n = any(NotExpr op).getOperand() + or + 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 + ) +} + +private 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.nil?` + exists(MethodCall c | guard.getAstNode() = c | + c.getReceiver() = read.getVariable().getAnAccess() and c.getMethodName() = "nil?" - ) + ) and + branch = false + ) +} + +private predicate isNilChecked(LocalVariableReadAccess read) { + exists(MethodCall c | c.getReceiver() = read | + c.getMethodName() = "nil?" + or + c.isSafeNavigation() + ) +} + +/** + * 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 | not isNilMethodName(m.getMethodName())).getReceiver() } } 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..56172f4df706 --- /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.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.expected b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected new file mode 100644 index 000000000000..174d0d348a2a --- /dev/null +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected @@ -0,0 +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: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.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..7c82c8bf69e0 --- /dev/null +++ b/ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.rb @@ -0,0 +1,77 @@ +def m + puts "m" +end + +def foo + m # calls m above + if false + m = "0" + m # reads local variable m + else + end + 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.strip + end + 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.strip + end + if false + b = "0" + end + b.nil? + b || 0 # 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" + end + unless c + return + end + c.strip # OK - given above unless + + if false + d = "0" + end + if (d.nil?) + return + end + d.strip # OK - given above check + + if false + e = "0" + end + unless (!e.nil?) + return + end + e.strip # OK - given above unless +end + +def test_loop + begin + if false + a = 0 + else + set_a + end + end until a # OK + a.strip # OK - given previous until +end + +def test_for + for i in ["foo", "bar"] # OK - since 0..10 cannot raise + puts i.strip + end + i.strip #$ SPURIOUS: Alert +end \ No newline at end of file