ruby: refine query for uninitialised local variables

- there are places where uninitialised reads are intentional
- there are also some places where they are impossible

Author: yoff

ruby/ql/src/queries/variables/UninitializedLocal.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In Ruby, raw identifiers like <code>x</code> 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:
+</p>
+
+<sample src="examples/UninitializedLocal.rb" />
+
+<p>
+This will generate an alert on the last access to <code>m</code>, where it is not clear that the programmer intended to read from the local variable.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+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 <code>nil</code>.
+</p>
+
+</recommendation>
+
+<references>
+
+
+<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
+
+
+
+</references>
+</qhelp>

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)
   }
 }
 

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

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 |

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