mirror of
https://github.com/github/codeql.git
synced 2026-04-25 16:55:19 +02:00
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.
This commit is contained in:
@@ -12,6 +12,8 @@ Consider the following example:
|
||||
|
||||
<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.
|
||||
In fact, the last access to <code>m</code> is a method call, and the value of the local variable is <code>nil</code>,
|
||||
so this will raise a <code>NoMethodError</code>.
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
@@ -19,8 +21,7 @@ This will generate an alert on the last access to <code>m</code>, where it is no
|
||||
|
||||
<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>.
|
||||
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.
|
||||
</p>
|
||||
|
||||
</recommendation>
|
||||
@@ -28,8 +29,8 @@ Another possibility is that an exception may be raised before the variable is as
|
||||
<references>
|
||||
|
||||
|
||||
<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
|
||||
|
||||
<li>https://www.rubyguides.com/: <a href="https://www.rubyguides.com/2018/01/ruby-nil/">Nil</a>.</li>
|
||||
<li>https://ruby-doc.org/: <a href="https://ruby-doc.org/core-2.6.5/NoMethodError.html">NoMethodError</a>.</li>
|
||||
|
||||
|
||||
</references>
|
||||
|
||||
@@ -77,7 +77,8 @@ class RelevantLocalVariableReadAccess extends LocalVariableReadAccess instanceof
|
||||
RelevantLocalVariableReadAccess() {
|
||||
not isInBooleanContext(this) and
|
||||
not isNilChecked(this) and
|
||||
not isGuarded(this)
|
||||
not isGuarded(this) and
|
||||
this = any(MethodCall m).getReceiver()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -5,10 +5,10 @@ end
|
||||
def foo
|
||||
m # calls m above
|
||||
if false
|
||||
m = 0
|
||||
m = "0"
|
||||
m # reads local variable m
|
||||
else
|
||||
end
|
||||
m # reads uninitialized local variable m, `nil`
|
||||
m.strip # reads uninitialized local variable m, `nil`, and crashes
|
||||
m2 # undefined local variable or method 'm2' for main (NameError)
|
||||
end
|
||||
@@ -5,57 +5,57 @@ end
|
||||
def foo
|
||||
m # calls m above
|
||||
if false
|
||||
m = 0
|
||||
m = "0"
|
||||
m # reads local variable m
|
||||
else
|
||||
end
|
||||
m #$ Alert
|
||||
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
|
||||
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
|
||||
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
|
||||
if !(a = "3") or a # OK - a is assigned in the previous conjunct
|
||||
a.strip
|
||||
end
|
||||
if false
|
||||
b = 0
|
||||
b = "0"
|
||||
end
|
||||
b.nil?
|
||||
b || 0 # OK
|
||||
b&.m # OK - safe navigation
|
||||
b if b # 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
|
||||
c = "0"
|
||||
end
|
||||
unless c
|
||||
return
|
||||
end
|
||||
c # OK - given above unless
|
||||
c.strip # OK - given above unless
|
||||
|
||||
if false
|
||||
d = 0
|
||||
d = "0"
|
||||
end
|
||||
if (d.nil?)
|
||||
return
|
||||
end
|
||||
d # OK - given above check
|
||||
d.strip # OK - given above check
|
||||
|
||||
if false
|
||||
e = 0
|
||||
e = "0"
|
||||
end
|
||||
unless (!e.nil?)
|
||||
return
|
||||
end
|
||||
e # OK - given above unless
|
||||
e.strip # OK - given above unless
|
||||
end
|
||||
|
||||
def test_loop
|
||||
@@ -66,12 +66,12 @@ def test_loop
|
||||
set_a
|
||||
end
|
||||
end until a # OK
|
||||
a # OK - given previous until
|
||||
a.strip # OK - given previous until
|
||||
end
|
||||
|
||||
def test_for
|
||||
for i in 0..10 # OK - since 0..10 cannot raise
|
||||
i
|
||||
for i in ["foo", "bar"] # OK - since 0..10 cannot raise
|
||||
puts i.strip
|
||||
end
|
||||
i #$ SPURIOUS: Alert
|
||||
i.strip #$ SPURIOUS: Alert
|
||||
end
|
||||
Reference in New Issue
Block a user