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,19 +5,81 @@
  * @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
+
+predicate factor(Expr e, Expr factor) {
+  factor = e
+  or
+  e.(BinaryOperation).getOperator() = ["||", "&&"] and
+  factor = e.(BinaryOperation).getAnOperand()
+  or
+  e.(BinaryOperation).getOperator() = ["||", "&&"] and
+  factor(e.(BinaryOperation).getAnOperand(), factor)
+}
+
+predicate previousConjunct(Expr e, Expr prev) {
+  exists(BinaryOperation b |
+    b.getOperator() = "&&" and
+    b.getRightOperand() = e
+  |
+    // 'prev' && 'e'
+    prev = b.getLeftOperand()
+    or
+    // (... && 'prev') && 'e'
+    b.getLeftOperand().(BinaryOperation).getOperator() = "&&" and
+    prev = b.getLeftOperand().(BinaryOperation).getRightOperand()
+    or
+    // (subtree['prev'] && _) && 'e'
+    b.getLeftOperand().(BinaryOperation).getOperator() = "&&" and
+    previousConjunct(b.getLeftOperand().(BinaryOperation).getRightOperand(), prev)
+  )
+}
+
+Expr evaluatingMention(LocalVariableReadAccess read) {
+  result = read
+  or
+  result.(AssignExpr).getLeftOperand() = read
+  or
+  result.(NotExpr).getOperand() = read
+}
 
 class RelevantLocalVariableReadAccess extends LocalVariableReadAccess {
   RelevantLocalVariableReadAccess() {
     not exists(MethodCall c |
       c.getReceiver() = this and
       c.getMethodName() = "nil?"
+    ) and
+    not exists(MethodCall c |
+      c.getReceiver() = this and
+      c.isSafeNavigation()
+    ) and
+    // 'a' is fine to be uninitialised in 'a || ...'
+    not exists(BinaryOperation b |
+      b.getLeftOperand() = this and
+      b.getOperator() = "||"
+    ) and
+    // The second 'a' cannot be uninitialised in 'a && (...a...)'
+    not exists(Expr parent |
+      parent.getAChild*() = this and
+      previousConjunct(parent, this.getVariable().getAnAccess())
+    ) and
+    // Various guards
+    not exists(ConditionalExpr c | factor(c.getCondition(), evaluatingMention(this))) and
+    not exists(ConditionalExpr c | factor(c.getCondition(), this.getVariable().getAnAccess()) |
+      this = c.getBranch(true).getAChild*()
+    ) and
+    not exists(ConditionalLoop l |
+      l.getCondition() = this.getVariable().getAnAccess() and
+      l.entersLoopWhenConditionIs(false) and
+      l.getAControlFlowNode().getBasicBlock().dominates(this.getAControlFlowNode().getBasicBlock())
     )
   }
 }

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