Skip to content

Python: Fix globals() == locals() FP #6688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion python/ql/src/Statements/ModificationOfLocals.ql
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,11 @@ predicate modification_of_locals(ControlFlowNode f) {
}

from AstNode a, ControlFlowNode f
where modification_of_locals(f) and a = f.getNode()
where
modification_of_locals(f) and
a = f.getNode() and
// in module level scope `locals() == globals()`
// see https://docs.python.org/3/library/functions.html#locals
// FP report in https://github.com/github/codeql/issues/6674
not a.getScope() instanceof ModuleScope
select a, "Modification of the locals() dictionary will have no effect on the local variables."
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| test.py:109:5:109:8 | cond | Parenthesized condition in 'if' statement. |
| test.py:112:8:112:11 | cond | Parenthesized condition in 'while' statement. |
| test.py:115:9:115:12 | test | Parenthesized test in 'assert' statement. |
| test.py:118:13:118:13 | x | Parenthesized value in 'return' statement. |
| test.py:115:5:115:8 | cond | Parenthesized condition in 'if' statement. |
| test.py:118:8:118:11 | cond | Parenthesized condition in 'while' statement. |
| test.py:121:9:121:12 | test | Parenthesized test in 'assert' statement. |
| test.py:124:13:124:13 | x | Parenthesized value in 'return' statement. |
Original file line number Diff line number Diff line change
@@ -1 +1 @@
| test.py:162:9:162:17 | Attribute() | Instance of context-manager class $@ is closed in a finally block. Consider using 'with' statement. | test.py:145:1:145:17 | class CM | CM |
| test.py:168:9:168:17 | Attribute() | Instance of context-manager class $@ is closed in a finally block. Consider using 'with' statement. | test.py:151:1:151:17 | class CM | CM |
25 changes: 15 additions & 10 deletions python/ql/test/query-tests/Statements/general/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def return_in_finally(seq, x):
finally:
return 1
return 0

#Break in loop in finally
#This is OK
def return_in_loop_in_finally(f, seq):
Expand All @@ -27,7 +27,7 @@ def return_in_loop_in_finally(f, seq):
finally:
for i in seq:
break

#But this is not
def return_in_loop_in_finally(f, seq):
try:
Expand All @@ -49,7 +49,7 @@ def __init__(self):

for x in NonIterator():
do_something(x)

#None in for loop

def dodgy_iter(x):
Expand Down Expand Up @@ -91,8 +91,8 @@ class D(dict): pass





def modification_of_locals():
x = 0
locals()['x'] = 1
Expand All @@ -104,6 +104,12 @@ def modification_of_locals():
return x


globals()['foo'] = 42 # OK
# in module-level scope `locals() == globals()`
# FP report from https://github.com/github/codeql/issues/6674
locals()['foo'] = 43 # technically OK


#C-style things

if (cond):
Expand All @@ -128,7 +134,7 @@ def __get__(self, instance, instance_type):
return self.getter(instance_type)

class WithClassProperty(object):

@classproperty
def x(self):
return [0]
Expand All @@ -143,13 +149,13 @@ def x(self):
#Should use context mamager

class CM(object):

def __enter__(self):
pass

def __exit__(self, ex, cls, tb):
pass

def write(self, data):
pass

Expand All @@ -168,4 +174,3 @@ def assert_ok(seq):
# False positive. ODASA-8042. Fixed in PR #2401.
class false_positive:
e = (x for x in [])