From 3992346add01639672719462b6d6e1f1595fe2d8 Mon Sep 17 00:00:00 2001
From: Mark Shannon <mark@hotpy.org>
Date: Mon, 28 Jan 2019 12:57:18 +0000
Subject: [PATCH] Python: Fix up mutating-descriptor query to only flag
 mutation when they occur during descriptor protocol.

---
 change-notes/1.20/analysis-python.md          |  1 +
 python/ql/src/Classes/MutatingDescriptor.ql   |  9 ++++++--
 .../descriptors/MutatingDescriptor.expected   |  1 +
 .../query-tests/Classes/descriptors/test.py   | 21 ++++++++++++++-----
 4 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/change-notes/1.20/analysis-python.md b/change-notes/1.20/analysis-python.md
index 978e6a0c30d..e4ff53a37d8 100644
--- a/change-notes/1.20/analysis-python.md
+++ b/change-notes/1.20/analysis-python.md
@@ -20,6 +20,7 @@ Removes false positives seen when using Python 3.6, but not when using earlier v
 
  | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| 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. |
 
diff --git a/python/ql/src/Classes/MutatingDescriptor.ql b/python/ql/src/Classes/MutatingDescriptor.ql
index 328c1fe86ab..65659526f57 100644
--- a/python/ql/src/Classes/MutatingDescriptor.ql
+++ b/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()
     )
diff --git a/python/ql/test/query-tests/Classes/descriptors/MutatingDescriptor.expected b/python/ql/test/query-tests/Classes/descriptors/MutatingDescriptor.expected
index ae4d733f7ad..4e69aab2f53 100644
--- a/python/ql/test/query-tests/Classes/descriptors/MutatingDescriptor.expected
+++ b/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 |
diff --git a/python/ql/test/query-tests/Classes/descriptors/test.py b/python/ql/test/query-tests/Classes/descriptors/test.py
index ba8da34839e..180ade50e36 100644
--- a/python/ql/test/query-tests/Classes/descriptors/test.py
+++ b/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)
\ No newline at end of file
+        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