Skip to content

Commit 930f1b5

Browse files
yoffyoff
authored
Merge pull request github#15397 from github/tausbn/python-fix-deepcopy-mutable-default-fp
Python: Fix `deepcopy` mutable default FP
2 parents 67a8639 + 96b1b8e commit 930f1b5

10 files changed

+115
-118
lines changed

‎python/ql/src/semmle/python/functions/ModificationOfParameterWithDefault.qll

+5
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
private import python
1010
import semmle.python.dataflow.new.DataFlow
11+
private import semmle.python.dataflow.new.internal.TaintTrackingPrivate as TTP
1112

1213
/**
1314
* Provides a data-flow configuration for detecting modifications of a parameters default value.
@@ -69,6 +70,10 @@ module ModificationOfParameterWithDefault {
6970
// if we are tracking a empty default, then it is ok to modify non-empty values,
7071
// so our tracking ends at those.
7172
state = false and node instanceof MustBeNonEmpty
73+
or
74+
// the target of a copy step is (presumably) a different object, and hence modifications of
75+
// this object no longer matter for the purposes of this query.
76+
TTP::copyStep(_, node) and state in [true, false]
7277
}
7378
}
7479

‎python/ql/src/semmle/python/functions/ModificationOfParameterWithDefaultCustomizations.qll

+5-1
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,11 @@ module ModificationOfParameterWithDefault {
7575
class MutableDefaultValue extends Source {
7676
boolean nonEmpty;
7777

78-
MutableDefaultValue() { nonEmpty = mutableDefaultValue(this.asCfgNode().(NameNode).getNode()) }
78+
MutableDefaultValue() {
79+
nonEmpty = mutableDefaultValue(this.asCfgNode().(NameNode).getNode()) and
80+
// Ignore sources inside the standard library. These are unlikely to be true positives.
81+
exists(this.getLocation().getFile().getRelativePath())
82+
}
7983

8084
override boolean isNonEmpty() { result = nonEmpty }
8185
}

‎python/ql/test/query-tests/Functions/ModificationOfParameterWithDefault/ModificationOfParameterWithDefault.expected

+32
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,18 @@ edges
2828
| test.py:131:23:131:23 | ControlFlowNode for l | test.py:135:9:135:9 | ControlFlowNode for l |
2929
| test.py:138:15:138:15 | ControlFlowNode for l | test.py:140:9:140:9 | ControlFlowNode for l |
3030
| test.py:145:23:145:23 | ControlFlowNode for l | test.py:147:9:147:9 | ControlFlowNode for l |
31+
| test.py:153:25:153:25 | ControlFlowNode for x | test.py:154:5:154:5 | ControlFlowNode for x |
32+
| test.py:156:21:156:21 | ControlFlowNode for x | test.py:157:5:157:5 | ControlFlowNode for x |
33+
| test.py:159:27:159:27 | ControlFlowNode for y | test.py:160:25:160:25 | ControlFlowNode for y |
34+
| test.py:159:27:159:27 | ControlFlowNode for y | test.py:161:21:161:21 | ControlFlowNode for y |
35+
| test.py:160:25:160:25 | ControlFlowNode for y | test.py:153:25:153:25 | ControlFlowNode for x |
36+
| test.py:161:21:161:21 | ControlFlowNode for y | test.py:156:21:156:21 | ControlFlowNode for x |
37+
| test.py:181:28:181:28 | ControlFlowNode for x | test.py:185:9:185:9 | ControlFlowNode for x |
38+
| test.py:181:28:181:28 | ControlFlowNode for x | test.py:187:9:187:9 | ControlFlowNode for x |
39+
| test.py:194:18:194:18 | ControlFlowNode for x | test.py:195:28:195:28 | ControlFlowNode for x |
40+
| test.py:195:28:195:28 | ControlFlowNode for x | test.py:181:28:181:28 | ControlFlowNode for x |
41+
| test.py:197:18:197:18 | ControlFlowNode for x | test.py:198:28:198:28 | ControlFlowNode for x |
42+
| test.py:198:28:198:28 | ControlFlowNode for x | test.py:181:28:181:28 | ControlFlowNode for x |
3143
nodes
3244
| test.py:2:12:2:12 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
3345
| test.py:3:5:3:5 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
@@ -81,6 +93,20 @@ nodes
8193
| test.py:140:9:140:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
8294
| test.py:145:23:145:23 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
8395
| test.py:147:9:147:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
96+
| test.py:153:25:153:25 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
97+
| test.py:154:5:154:5 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
98+
| test.py:156:21:156:21 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
99+
| test.py:157:5:157:5 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
100+
| test.py:159:27:159:27 | ControlFlowNode for y | semmle.label | ControlFlowNode for y |
101+
| test.py:160:25:160:25 | ControlFlowNode for y | semmle.label | ControlFlowNode for y |
102+
| test.py:161:21:161:21 | ControlFlowNode for y | semmle.label | ControlFlowNode for y |
103+
| test.py:181:28:181:28 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
104+
| test.py:185:9:185:9 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
105+
| test.py:187:9:187:9 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
106+
| test.py:194:18:194:18 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
107+
| test.py:195:28:195:28 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
108+
| test.py:197:18:197:18 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
109+
| test.py:198:28:198:28 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
84110
subpaths
85111
#select
86112
| test.py:3:5:3:5 | ControlFlowNode for l | test.py:2:12:2:12 | ControlFlowNode for l | test.py:3:5:3:5 | ControlFlowNode for l | This expression mutates a $@. | test.py:2:12:2:12 | ControlFlowNode for l | default value |
@@ -106,3 +132,9 @@ subpaths
106132
| test.py:135:9:135:9 | ControlFlowNode for l | test.py:131:23:131:23 | ControlFlowNode for l | test.py:135:9:135:9 | ControlFlowNode for l | This expression mutates a $@. | test.py:131:23:131:23 | ControlFlowNode for l | default value |
107133
| test.py:140:9:140:9 | ControlFlowNode for l | test.py:138:15:138:15 | ControlFlowNode for l | test.py:140:9:140:9 | ControlFlowNode for l | This expression mutates a $@. | test.py:138:15:138:15 | ControlFlowNode for l | default value |
108134
| test.py:147:9:147:9 | ControlFlowNode for l | test.py:145:23:145:23 | ControlFlowNode for l | test.py:147:9:147:9 | ControlFlowNode for l | This expression mutates a $@. | test.py:145:23:145:23 | ControlFlowNode for l | default value |
135+
| test.py:154:5:154:5 | ControlFlowNode for x | test.py:159:27:159:27 | ControlFlowNode for y | test.py:154:5:154:5 | ControlFlowNode for x | This expression mutates a $@. | test.py:159:27:159:27 | ControlFlowNode for y | default value |
136+
| test.py:157:5:157:5 | ControlFlowNode for x | test.py:159:27:159:27 | ControlFlowNode for y | test.py:157:5:157:5 | ControlFlowNode for x | This expression mutates a $@. | test.py:159:27:159:27 | ControlFlowNode for y | default value |
137+
| test.py:185:9:185:9 | ControlFlowNode for x | test.py:194:18:194:18 | ControlFlowNode for x | test.py:185:9:185:9 | ControlFlowNode for x | This expression mutates a $@. | test.py:194:18:194:18 | ControlFlowNode for x | default value |
138+
| test.py:185:9:185:9 | ControlFlowNode for x | test.py:197:18:197:18 | ControlFlowNode for x | test.py:185:9:185:9 | ControlFlowNode for x | This expression mutates a $@. | test.py:197:18:197:18 | ControlFlowNode for x | default value |
139+
| test.py:187:9:187:9 | ControlFlowNode for x | test.py:194:18:194:18 | ControlFlowNode for x | test.py:187:9:187:9 | ControlFlowNode for x | This expression mutates a $@. | test.py:194:18:194:18 | ControlFlowNode for x | default value |
140+
| test.py:187:9:187:9 | ControlFlowNode for x | test.py:197:18:197:18 | ControlFlowNode for x | test.py:187:9:187:9 | ControlFlowNode for x | This expression mutates a $@. | test.py:197:18:197:18 | ControlFlowNode for x | default value |

‎python/ql/test/query-tests/Functions/ModificationOfParameterWithDefault/options

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
semmle-extractor-options: --max-import-depth=2 --dont-split-graph

‎python/ql/test/query-tests/Functions/ModificationOfParameterWithDefault/test.py

+68
Original file line numberDiff line numberDiff line change
@@ -148,3 +148,71 @@ def sanitizer_negated(l = [1]):
148148
else:
149149
l.append(1)
150150
return l
151+
152+
# indirect modification of parameter with default
153+
def aug_assign_argument(x):
154+
x += ['x'] #$ modification=x
155+
156+
def mutate_argument(x):
157+
x.append('x') #$ modification=x
158+
159+
def indirect_modification(y = []):
160+
aug_assign_argument(y)
161+
mutate_argument(y)
162+
163+
def guarded_modification(z=[]):
164+
if z:
165+
z.append(0)
166+
return z
167+
168+
# This function causes a discrepancy between the
169+
# Python 2 and 3 versions of the analysis.
170+
# We comment it out until we have resoved the issue.
171+
#
172+
# def issue1143(expr, param=[]):
173+
# if not param:
174+
# return result
175+
# for i in param:
176+
# param.remove(i) # Mutation here
177+
178+
179+
# Type guarding of modification of parameter with default:
180+
181+
def do_stuff_based_on_type(x):
182+
if isinstance(x, str):
183+
x = x.split()
184+
elif isinstance(x, dict):
185+
x.setdefault('foo', 'bar') #$ modification=x
186+
elif isinstance(x, list):
187+
x.append(5) #$ modification=x
188+
elif isinstance(x, tuple):
189+
x = x.unknown_method()
190+
191+
def str_default(x="hello world"):
192+
do_stuff_based_on_type(x)
193+
194+
def dict_default(x={'baz':'quux'}):
195+
do_stuff_based_on_type(x)
196+
197+
def list_default(x=[1,2,3,4]):
198+
do_stuff_based_on_type(x)
199+
200+
def tuple_default(x=(1,2)):
201+
do_stuff_based_on_type(x)
202+
203+
# Modification of parameter with default (safe method)
204+
205+
def safe_method(x=[]):
206+
return x.count(42)
207+
208+
# Use of deepcopy:
209+
210+
from copy import deepcopy
211+
212+
def flow_from_within_deepcopy_fp():
213+
y = deepcopy([])
214+
y.append(1)
215+
216+
def flow_through_deepcopy_fp(x=[]):
217+
y = deepcopy(x)
218+
y.append(1)

‎python/ql/test/query-tests/Functions/general/DeprecatedSliceMethod.expected

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
| functions_test.py:99:5:99:40 | Function DeprecatedSliceMethods.__getslice__ | __getslice__ method has been deprecated since Python 2.0. |
2-
| functions_test.py:102:5:102:47 | Function DeprecatedSliceMethods.__setslice__ | __setslice__ method has been deprecated since Python 2.0. |
3-
| functions_test.py:105:5:105:40 | Function DeprecatedSliceMethods.__delslice__ | __delslice__ method has been deprecated since Python 2.0. |
1+
| functions_test.py:95:5:95:40 | Function DeprecatedSliceMethods.__getslice__ | __getslice__ method has been deprecated since Python 2.0. |
2+
| functions_test.py:98:5:98:47 | Function DeprecatedSliceMethods.__setslice__ | __setslice__ method has been deprecated since Python 2.0. |
3+
| functions_test.py:101:5:101:40 | Function DeprecatedSliceMethods.__delslice__ | __delslice__ method has been deprecated since Python 2.0. |

‎python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected

-44
This file was deleted.

‎python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.qlref

-1
This file was deleted.

‎python/ql/test/query-tests/Functions/general/functions_test.py

-68
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,6 @@ def ok3(x):
3636
def ok4(x = []):
3737
return len(x)
3838

39-
def mpd(x = []):
40-
x.append("x")
41-
42-
4339
def use_implicit_return_value(arg):
4440
x = do_nothing()
4541
return call_non_callable(arg)
@@ -128,12 +124,6 @@ def mutli_return(arg):
128124
else:
129125
return do_nothing()
130126

131-
#Modification of parameter with default
132-
133-
def augassign(x = []):
134-
x += ["x"]
135-
136-
137127
#ODASA 3658
138128
from sys import exit
139129
#Consistent returns
@@ -144,61 +134,3 @@ def ok5():
144134
except ValueError as e:
145135
print(e)
146136
exit(EXIT_ERROR)
147-
148-
149-
150-
# indirect modification of parameter with default
151-
def aug_assign_argument(x):
152-
x += ['x']
153-
154-
def mutate_argument(x):
155-
x.append('x')
156-
157-
def indirect_modification(y = []):
158-
aug_assign_argument(y)
159-
mutate_argument(y)
160-
161-
def guarded_modification(z=[]):
162-
if z:
163-
z.append(0)
164-
return z
165-
166-
# This function causes a discrepancy between the
167-
# Python 2 and 3 versions of the analysis.
168-
# We comment it out until we have resoved the issue.
169-
#
170-
# def issue1143(expr, param=[]):
171-
# if not param:
172-
# return result
173-
# for i in param:
174-
# param.remove(i) # Mutation here
175-
176-
177-
# Type guarding of modification of parameter with default:
178-
179-
def do_stuff_based_on_type(x):
180-
if isinstance(x, str):
181-
x = x.split()
182-
elif isinstance(x, dict):
183-
x.setdefault('foo', 'bar')
184-
elif isinstance(x, list):
185-
x.append(5)
186-
elif isinstance(x, tuple):
187-
x = x.unknown_method()
188-
189-
def str_default(x="hello world"):
190-
do_stuff_based_on_type(x)
191-
192-
def dict_default(x={'baz':'quux'}):
193-
do_stuff_based_on_type(x)
194-
195-
def list_default(x=[1,2,3,4]):
196-
do_stuff_based_on_type(x)
197-
198-
def tuple_default(x=(1,2)):
199-
do_stuff_based_on_type(x)
200-
201-
# Modification of parameter with default (safe method)
202-
203-
def safe_method(x=[]):
204-
return x.count(42)

‎python/ql/test/query-tests/Functions/general/options

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
semmle-extractor-options: --max-import-depth=1 --dont-split-graph
1+
semmle-extractor-options: --max-import-depth=2 --dont-split-graph

0 commit comments

Comments
 (0)