Skip to content

Commit c516aa6

Browse files
yoffyoff
committed
ruby: refine query for uninitialised local variables
- there are places where uninitialised reads are intentional - there are also some places where they are impossible
1 parent 1ca25b2 commit c516aa6

5 files changed

Lines changed: 160 additions & 46 deletions

File tree

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
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).
8+
Consider the following example:
9+
</p>
10+
11+
<sample src="examples/UninitializedLocal.rb" />
12+
13+
<p>
14+
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.
15+
</p>
16+
17+
</overview>
18+
<recommendation>
19+
20+
<p>
21+
Ensure that you check the control and data flow in the method carefully.
22+
Check that the variable reference is spelled correctly, perhaps the variable has been renamed and the reference needs to be updated.
23+
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>.
24+
</p>
25+
26+
</recommendation>
27+
28+
<references>
29+
30+
31+
<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
32+
33+
34+
35+
</references>
36+
</qhelp>

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

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,71 @@
55
* @kind problem
66
* @problem.severity error
77
* @id rb/uninitialized-local-variable
8-
* @tags reliability
8+
* @tags quality
9+
* reliability
910
* correctness
10-
* @precision low
11+
* @precision high
1112
*/
1213

1314
import codeql.ruby.AST
1415
import codeql.ruby.dataflow.SSA
16+
private import codeql.ruby.dataflow.internal.DataFlowPublic
17+
import codeql.ruby.controlflow.internal.Guards as Guards
18+
import codeql.ruby.controlflow.CfgNodes
19+
20+
predicate isInBooleanContext(Expr e) {
21+
e = any(ConditionalExpr c).getCondition()
22+
or
23+
e = any(ConditionalLoop l).getCondition()
24+
or
25+
e = any(LogicalAndExpr n).getAnOperand()
26+
or
27+
e = any(LogicalOrExpr n).getAnOperand()
28+
or
29+
e = any(NotExpr n).getOperand()
30+
}
31+
32+
predicate isGuarded(LocalVariableReadAccess read) {
33+
exists(AstCfgNode guard, boolean branch |
34+
Guards::guardControlsBlock(guard, read.getAControlFlowNode().getBasicBlock(), branch)
35+
|
36+
// guard is `var`
37+
guard.getAstNode() = read.getVariable().getAnAccess() and
38+
branch = true
39+
or
40+
// guard is `!var`
41+
guard.getAstNode().(NotExpr).getOperand() = read.getVariable().getAnAccess() and
42+
branch = false
43+
or
44+
// guard is `var.nil?`
45+
exists(MethodCall c | guard.getAstNode() = c |
46+
c.getReceiver() = read.getVariable().getAnAccess() and
47+
c.getMethodName() = "nil?"
48+
) and
49+
branch = false
50+
or
51+
// guard is `!var.nil?`
52+
exists(MethodCall c | guard.getAstNode().(NotExpr).getOperand() = c |
53+
c.getReceiver() = read.getVariable().getAnAccess() and
54+
c.getMethodName() = "nil?"
55+
) and
56+
branch = true
57+
)
58+
}
59+
60+
predicate isNilChecked(LocalVariableReadAccess read) {
61+
exists(MethodCall c | c.getReceiver() = read |
62+
c.getMethodName() = "nil?"
63+
or
64+
c.isSafeNavigation()
65+
)
66+
}
1567

1668
class RelevantLocalVariableReadAccess extends LocalVariableReadAccess {
1769
RelevantLocalVariableReadAccess() {
18-
not exists(MethodCall c |
19-
c.getReceiver() = this and
20-
c.getMethodName() = "nil?"
21-
)
70+
not isInBooleanContext(this) and
71+
not isNilChecked(this) and
72+
not isGuarded(this)
2273
}
2374
}
2475

‎ruby/ql/src/queries/variables/examples/UninitializedLocal.rb‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
def m
2+
puts "m"
3+
end
4+
5+
def foo
6+
m # calls m above
7+
if false
8+
m = 0
9+
m # reads local variable m
10+
else
11+
end
12+
m # reads uninitialized local variable m, `nil`
13+
m2 # undefined local variable or method 'm2' for main (NameError)
14+
end

‎ruby/ql/test/query-tests/variables/UninitializedLocal/UninitializedLocal.expected‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,3 @@
1-
| UninitializedLocal.rb:12:3:12:3 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:7:8:7 | m | m |
2-
| UninitializedLocal.rb:17:16:17:16 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:17:7:17:7 | a | a |
3-
| UninitializedLocal.rb:30:3:30:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
4-
| UninitializedLocal.rb:31:3:31:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
5-
| UninitializedLocal.rb:32:3:32:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
6-
| UninitializedLocal.rb:32:8:32:8 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
7-
| UninitializedLocal.rb:33:3:33:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
8-
| UninitializedLocal.rb:33:14:33:14 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
9-
| UninitializedLocal.rb:33:20:33:20 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
10-
| UninitializedLocal.rb:34:3:34:3 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
11-
| UninitializedLocal.rb:34:15:34:15 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
12-
| UninitializedLocal.rb:34:21:34:21 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:7:27:7 | b | b |
13-
| UninitializedLocal.rb:44:13:44:13 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:40:11:40:11 | a | a |
14-
| UninitializedLocal.rb:45:3:45:3 | a | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:40:11:40:11 | a | a |
1+
| UninitializedLocal.rb:12:5:12:5 | m | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:8:9:8:9 | m | m |
2+
| UninitializedLocal.rb:34:5:34:5 | b | Local variable $@ may be used before it is initialized. | UninitializedLocal.rb:27:9:27:9 | b | b |
3+
| 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‎

Lines changed: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -14,33 +14,57 @@ def foo
1414
end
1515

1616
def test_guards
17-
if (a = 3 && a) #$ Alert
18-
a
19-
end
20-
if (a = 3) && a # OK - a is assigned in the previous conjunct
21-
a
22-
end
23-
if !(a = 3) or a # OK - a is assigned in the previous conjunct
24-
a
25-
end
26-
if false
27-
b = 0
28-
end
29-
b.nil?
30-
b || 0 #$ SPURIOUS: Alert
31-
b&.m #$ SPURIOUS: Alert
32-
b if b #$ SPURIOUS: Alert
33-
b.close if b && !b.closed #$ SPURIOUS: Alert
34-
b.blowup if b || !b.blownup #$ Alert
17+
if (a = 3 && a) # OK - a is in a Boolean context
18+
a
19+
end
20+
if (a = 3) && a # OK - a is assigned in the previous conjunct
21+
a
22+
end
23+
if !(a = 3) or a # OK - a is assigned in the previous conjunct
24+
a
25+
end
26+
if false
27+
b = 0
28+
end
29+
b.nil?
30+
b || 0 # OK
31+
b&.m # OK - safe navigation
32+
b if b # OK
33+
b.close if b && !b.closed # OK
34+
b.blowup if b || !b.blownup #$ Alert
35+
36+
if false
37+
c = 0
38+
end
39+
unless c
40+
return
41+
end
42+
c # OK - given above unless
43+
44+
if false
45+
d = 0
46+
end
47+
if (d.nil?)
48+
return
49+
end
50+
d # OK - given above check
51+
52+
if false
53+
e = 0
54+
end
55+
unless (!e.nil?)
56+
return
57+
end
58+
e # OK - given above unless
3559
end
3660

3761
def test_loop
38-
begin
39-
if false
40-
a = 0
41-
else
42-
set_a
43-
end
44-
end until a #$ SPURIOUS: Alert
45-
a #$ SPURIOUS: Alert
62+
begin
63+
if false
64+
a = 0
65+
else
66+
set_a
67+
end
68+
end until a # OK
69+
a # OK - given previous until
4670
end

0 commit comments

Comments
 (0)