8000 ruby: only report on method calls · github/codeql@2477233 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2477233

Browse files
yoffyoff
committed
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.
1 parent b641d5f commit 2477233

File tree

4 files changed

+30
-28
lines changed

4 files changed

+30
-28
lines changed

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

Lines changed: 5 additions & 4 deletions
10000
Original file line numberDiff line numberDiff line change
@@ -12,24 +12,25 @@ Consider the following example:
1212

1313
<p>
1414
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+
In fact, the last access to <code>m</code> is a method call, and the value of the local variable is <code>nil</code>,
16+
so this will raise a <code>NoMethodError</code>.
1517
</p>
1618

1719
</overview>
1820
<recommendation>
1921

2022
<p>
2123
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+
Add a check for <code>nil</code> before the read, or rewrite the code to ensure that the variable is always initialized before being read.
2425
</p>
2526

2627
</recommendation>
2728

2829
<references>
2930

3031

31-
<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
32-
32+
<li>https://www.rubyguides.com/: <a href="https://www.rubyguides.com/2018/01/ruby-nil/">Nil</a>.</li>
33+
<li>https://ruby-doc.org/: <a href="https://ruby-doc.org/core-2.6.5/NoMethodError.html">NoMethodError</a>.</li>
3334

3435

3536
</references>

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ class RelevantLocalVariableReadAccess extends LocalVariableReadAccess instanceof
7777
RelevantLocalVariableReadAccess() {
7878
not isInBooleanContext(this) and
7979
not isNilChecked(this) and
80-
not isGuarded(this)
80+
not isGuarded(this) and
81+
this = any(MethodCall m).getReceiver()
8182
}
8283
}
8384

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ def m
55
def foo
66
m # calls m above
77
if false
8-
m = 0
8+
m = "0"
99
m # reads local variable m
1010
else
1111
end
12-
m # reads uninitialized local variable m, `nil`
12+
m.strip # reads uninitialized local variable m, `nil`, and crashes
1313
m2 # undefined local variable or method 'm2' for main (NameError)
1414
end

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

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,57 +5,57 @@ def m
55
def foo
66
m # calls m above
77
if false
8-
m = 0
8+
m = "0"
99
m # reads local variable m
1010
else
1111
end
12-
m #$ Alert
12+
m.strip #$ Alert
1313
m2 # undefined local variable or method 'm2' for main (NameError)
1414
end
1515

1616
def test_guards
17-
if (a = 3 && a) # OK - a is in a Boolean context
18-
a
17+
if (a = "3" && a) # OK - a is in a Boolean context
18+
a.strip
1919
end
20-
if (a = 3) && a # OK - a is assigned in the previous conjunct
21-
a
20+
if (a = "3") && a # OK - a is assigned in the previous conjunct
21+
a.strip
2222
end
23-
if !(a = 3) or a # OK - a is assigned in the previous conjunct
24-
a
23+
if !(a = "3") or a # OK - a is assigned in the previous conjunct
24+
a.strip
2525
end
2626
if false
27-
b = 0
27+
b = "0"
2828
end
2929
b.nil?
3030
b || 0 # OK
31-
b&.m # OK - safe navigation
32-
b if b # OK
31+
b&.strip # OK - safe navigation
32+
b.strip if b # OK
3333
b.close if b && !b.closed # OK
3434
b.blowup if b || !b.blownup #$ Alert
3535

3636
if false
37-
c = 0
37+
c = "0"
3838
end
3939
unless c
4040
return
4141
end
42-
c # OK - given above unless
42+
c.strip # OK - given above unless
4343

4444
if false
45-
d = 0
45+
d = "0"
4646
end
4747
if (d.nil?)
4848
return
4949
end
50-
d # OK - given above check
50+
d.strip # OK - given above check
5151

5252
if false
53-
e = 0
53+
e = "0"
5454
end
5555
unless (!e.nil?)
5656
return
5757
end
58-
e # OK - given above unless
58+
e.strip # OK - given above unless
5959
end
6060

6161
def test_loop
@@ -66,12 +66,12 @@ def test_loop
6666
set_a
6767
end
6868
end until a # OK
69-
a # OK - given previous until
69+
a.strip # OK - given previous until
7070
end
7171

7272
def test_for
73-
for i in 0..10 # OK - since 0..10 cannot raise
74-
i
73+
for i in ["foo", "bar"] # OK - since 0..10 cannot raise
74+
puts i.strip
7575
end
76-
i #$ SPURIOUS: Alert
76+
i.strip #$ SPURIOUS: Alert
7777
end

0 commit comments

Comments
 (0)
0