Merge pull request #15397 from github/tausbn/python-fix-deepcopy-muta…

…ble-default-fp

Python: Fix `deepcopy` mutable default FP

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

@@ -8,6 +8,7 @@
 
 private import python
 import semmle.python.dataflow.new.DataFlow
+private import semmle.python.dataflow.new.internal.TaintTrackingPrivate as TTP
 
 /**
  * Provides a data-flow configuration for detecting modifications of a parameters default value.
@@ -69,6 +70,10 @@ module ModificationOfParameterWithDefault {
       // if we are tracking a empty default, then it is ok to modify non-empty values,
       // so our tracking ends at those.
       state = false and node instanceof MustBeNonEmpty
+      or
+      // the target of a copy step is (presumably) a different object, and hence modifications of
+      // this object no longer matter for the purposes of this query.
+      TTP::copyStep(_, node) and state in [true, false]
     }
   }
 

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

@@ -75,7 +75,11 @@ module ModificationOfParameterWithDefault {
   class MutableDefaultValue extends Source {
     boolean nonEmpty;
 
-    MutableDefaultValue() { nonEmpty = mutableDefaultValue(this.asCfgNode().(NameNode).getNode()) }
+    MutableDefaultValue() {
+      nonEmpty = mutableDefaultValue(this.asCfgNode().(NameNode).getNode()) and
+      // Ignore sources inside the standard library. These are unlikely to be true positives.
+      exists(this.getLocation().getFile().getRelativePath())
+    }
 
     override boolean isNonEmpty() { result = nonEmpty }
   }

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

@@ -28,6 +28,18 @@ edges
 | test.py:131:23:131:23 | ControlFlowNode for l | test.py:135:9:135:9 | ControlFlowNode for l |
 | test.py:138:15:138:15 | ControlFlowNode for l | test.py:140:9:140:9 | ControlFlowNode for l |
 | test.py:145:23:145:23 | ControlFlowNode for l | test.py:147:9:147:9 | ControlFlowNode for l |
+| test.py:153:25:153:25 | ControlFlowNode for x | test.py:154:5:154:5 | ControlFlowNode for x |
+| test.py:156:21:156:21 | ControlFlowNode for x | test.py:157:5:157:5 | ControlFlowNode for x |
+| test.py:159:27:159:27 | ControlFlowNode for y | test.py:160:25:160:25 | ControlFlowNode for y |
+| test.py:159:27:159:27 | ControlFlowNode for y | test.py:161:21:161:21 | ControlFlowNode for y |
+| test.py:160:25:160:25 | ControlFlowNode for y | test.py:153:25:153:25 | ControlFlowNode for x |
+| test.py:161:21:161:21 | ControlFlowNode for y | test.py:156:21:156:21 | ControlFlowNode for x |
+| test.py:181:28:181:28 | ControlFlowNode for x | test.py:185:9:185:9 | ControlFlowNode for x |
+| test.py:181:28:181:28 | ControlFlowNode for x | test.py:187:9:187:9 | ControlFlowNode for x |
+| test.py:194:18:194:18 | ControlFlowNode for x | test.py:195:28:195:28 | ControlFlowNode for x |
+| test.py:195:28:195:28 | ControlFlowNode for x | test.py:181:28:181:28 | ControlFlowNode for x |
+| test.py:197:18:197:18 | ControlFlowNode for x | test.py:198:28:198:28 | ControlFlowNode for x |
+| test.py:198:28:198:28 | ControlFlowNode for x | test.py:181:28:181:28 | ControlFlowNode for x |
 nodes
 | test.py:2:12:2:12 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:3:5:3:5 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
@@ -81,6 +93,20 @@ nodes
 | test.py:140:9:140:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:145:23:145:23 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
 | test.py:147:9:147:9 | ControlFlowNode for l | semmle.label | ControlFlowNode for l |
+| test.py:153:25:153:25 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:154:5:154:5 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:156:21:156:21 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:157:5:157:5 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:159:27:159:27 | ControlFlowNode for y | semmle.label | ControlFlowNode for y |
+| test.py:160:25:160:25 | ControlFlowNode for y | semmle.label | ControlFlowNode for y |
+| test.py:161:21:161:21 | ControlFlowNode for y | semmle.label | ControlFlowNode for y |
+| test.py:181:28:181:28 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:185:9:185:9 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:187:9:187:9 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:194:18:194:18 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:195:28:195:28 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:197:18:197:18 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| test.py:198:28:198:28 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
 subpaths
 #select
 | 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
 | 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 |
 | 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 |
 | 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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

@@ -0,0 +1 @@
+semmle-extractor-options: --max-import-depth=2 --dont-split-graph

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

@@ -148,3 +148,71 @@ def sanitizer_negated(l = [1]):
     else:
         l.append(1)
     return l
+
+# indirect modification of parameter with default
+def aug_assign_argument(x):
+    x += ['x'] #$ modification=x
+
+def mutate_argument(x):
+    x.append('x') #$ modification=x
+
+def indirect_modification(y = []):
+    aug_assign_argument(y)
+    mutate_argument(y)
+
+def guarded_modification(z=[]):
+    if z:
+        z.append(0)
+    return z
+
+# This function causes a discrepancy between the
+# Python 2 and 3 versions of the analysis.
+# We comment it out until we have resoved the issue.
+#
+# def issue1143(expr, param=[]):
+#     if not param:
+#         return result
+#     for i in param:
+#         param.remove(i) # Mutation here
+
+
+# Type guarding of modification of parameter with default:
+
+def do_stuff_based_on_type(x):
+    if isinstance(x, str):
+        x = x.split()
+    elif isinstance(x, dict):
+        x.setdefault('foo', 'bar') #$ modification=x
+    elif isinstance(x, list):
+        x.append(5) #$ modification=x
+    elif isinstance(x, tuple):
+        x = x.unknown_method()
+
+def str_default(x="hello world"):
+    do_stuff_based_on_type(x)
+
+def dict_default(x={'baz':'quux'}):
+    do_stuff_based_on_type(x)
+
+def list_default(x=[1,2,3,4]):
+    do_stuff_based_on_type(x)
+
+def tuple_default(x=(1,2)):
+    do_stuff_based_on_type(x)
+
+# Modification of parameter with default (safe method)
+
+def safe_method(x=[]):
+    return x.count(42)
+
+# Use of deepcopy:
+
+from copy import deepcopy
+
+def flow_from_within_deepcopy_fp():
+    y = deepcopy([])
+    y.append(1)
+
+def flow_through_deepcopy_fp(x=[]):
+    y = deepcopy(x)
+    y.append(1)

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

@@ -1,3 +1,3 @@
-| functions_test.py:99:5:99:40 | Function DeprecatedSliceMethods.__getslice__ | __getslice__ method has been deprecated since Python 2.0. |
-| functions_test.py:102:5:102:47 | Function DeprecatedSliceMethods.__setslice__ | __setslice__ method has been deprecated since Python 2.0. |
-| functions_test.py:105:5:105:40 | Function DeprecatedSliceMethods.__delslice__ | __delslice__ method has been deprecated since Python 2.0. |
+| functions_test.py:95:5:95:40 | Function DeprecatedSliceMethods.__getslice__ | __getslice__ method has been deprecated since Python 2.0. |
+| functions_test.py:98:5:98:47 | Function DeprecatedSliceMethods.__setslice__ | __setslice__ method has been deprecated since Python 2.0. |
+| 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/functions_test.py

@@ -36,10 +36,6 @@ def ok3(x):
 def ok4(x = []):
     return len(x)
 
-def mpd(x = []):
-    x.append("x")
-
-
 def use_implicit_return_value(arg):
     x = do_nothing()
     return call_non_callable(arg)
@@ -128,12 +124,6 @@ def mutli_return(arg):
     else:
         return do_nothing()
 
-#Modification of parameter with default
-
-def augassign(x = []):
-    x += ["x"]
-
-
 #ODASA 3658
 from sys import exit
 #Consistent returns
@@ -144,61 +134,3 @@ def ok5():
     except ValueError as e:
         print(e)
         exit(EXIT_ERROR)
-
-
-
-# indirect modification of parameter with default
-def aug_assign_argument(x):
-    x += ['x']
-
-def mutate_argument(x):
-    x.append('x')
-
-def indirect_modification(y = []):
-    aug_assign_argument(y)
-    mutate_argument(y)
-
-def guarded_modification(z=[]):
-    if z:
-        z.append(0)
-    return z
-
-# This function causes a discrepancy between the
-# Python 2 and 3 versions of the analysis.
-# We comment it out until we have resoved the issue.
-#
-# def issue1143(expr, param=[]):
-#     if not param:
-#         return result
-#     for i in param:
-#         param.remove(i) # Mutation here
-
-
-# Type guarding of modification of parameter with default:
-
-def do_stuff_based_on_type(x):
-    if isinstance(x, str):
-        x = x.split()
-    elif isinstance(x, dict):
-        x.setdefault('foo', 'bar')
-    elif isinstance(x, list):
-        x.append(5)
-    elif isinstance(x, tuple):
-        x = x.unknown_method()
-
-def str_default(x="hello world"):
-    do_stuff_based_on_type(x)
-
-def dict_default(x={'baz':'quux'}):
-    do_stuff_based_on_type(x)
-
-def list_default(x=[1,2,3,4]):
-    do_stuff_based_on_type(x)
-
-def tuple_default(x=(1,2)):
-    do_stuff_based_on_type(x)
-
-# Modification of parameter with default (safe method)
-
-def safe_method(x=[]):
-    return x.count(42)

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

@@ -1 +1 @@
-semmle-extractor-options: --max-import-depth=1 --dont-split-graph
+semmle-extractor-options: --max-import-depth=2 --dont-split-graph