Merge pull request #836 from markshannon/python-mutating-descriptor

Python: Fix up mutating-descriptor query

Author: taus-semmle

change-notes/1.20/analysis-python.md

@@ -24,6 +24,7 @@ Removes false positives seen when using Python 3.6, but not when using earlier v
  | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Comparison using is when operands support \_\_eq\_\_ (`py/comparison-using-is`) | Fewer false positive results | Results where one of the objects being compared is an enum member are no longer reported. |
+| Mutation of descriptor in \_\_get\_\_ or \_\_set\_\_ method (`py/mutable-descriptor`) | Fewer false positive results | Results where the mutation does not occur when calling one of the  `__get__`, `__set__` or `__delete__` methods are no longer reported. |
 | Unused import (`py/unused-import`) | Fewer false positive results | Results where the imported module is used in a `doctest` string are no longer reported. |
 | Unused import (`py/unused-import`) | Fewer false positive results | Results where the imported module is used in a type-hint comment are no longer reported. |
 

python/ql/src/Classes/MutatingDescriptor.ql

@@ -14,8 +14,13 @@ import python
 
 predicate mutates_descriptor(ClassObject cls, SelfAttributeStore s) {
     cls.isDescriptorType() and
-    exists(PyFunctionObject f |
-        cls.lookupAttribute(_) = f and 
+    exists(PyFunctionObject f, PyFunctionObject get_set |
+        exists(string name |
+            cls.lookupAttribute(name) = get_set |
+            name = "__get__" or name = "__set__" or name = "__delete__"
+        ) and
+        cls.lookupAttribute(_) = f and
+        get_set.getACallee*() = f and
         not f.getName() = "__init__" and
         s.getScope() = f.getFunction()
     )

python/ql/test/query-tests/Classes/descriptors/MutatingDescriptor.expected

@@ -1 +1,2 @@
 | test.py:10:9:10:19 | Attribute | Mutation of descriptor $@ object may lead to action-at-a-distance effects or race conditions for properties. | test.py:3:1:3:33 | class MutatingDescriptor | MutatingDescriptor |
+| test.py:25:9:25:19 | Attribute | Mutation of descriptor $@ object may lead to action-at-a-distance effects or race conditions for properties. | test.py:3:1:3:33 | class MutatingDescriptor | MutatingDescriptor |

python/ql/test/query-tests/Classes/descriptors/test.py

@@ -1,14 +1,25 @@
 
 #This is prone to strange side effects and race conditions.
 class MutatingDescriptor(object):
-    
+
     def __init__(self, func):
         self.my_func = func
-        
+
     def __get__(self, obj, obj_type):
-        #Modified state is visible to all instances of C that might call "show".
+        #Modified state is visible to all instances.
         self.my_obj = obj
         return self
-        
+
     def __call__(self, *args):
-        return self.my_func(self.my_obj, *args)
+        return self.my_func(self.my_obj, *args)
+
+    #Not call from __get__, __set__ or __delete__
+    def ok(self, func):
+        self.my_func = func
+
+    def __set__(self, obj, value):
+        self.not_ok(value)
+
+    def not_ok(self, value):
+        #Modified state is visible to all instances.
+        self.my_obj = value